Re: Conflict Detection and Resolution

On Tue, Oct 1, 2024 at 9:48 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>

I have started reviewing patch002, it is WIP, but please find initial
set of comments:

1.
ExecSimpleRelationInsert():
+ /* Check for conflict and return to caller for resolution if found */
+ if (resolver != CR_ERROR &&
+ has_conflicting_tuple(estate, resultRelInfo, &(*conflictslot),
+ CT_INSERT_EXISTS, resolver, slot, subid,
+ apply_remote))

Why are we calling has_conflicting_tuple only if the resolver is not
'ERROR '? Is it for optimization to avoid pre-scan for ERROR cases? If
so, please add a comment.

2)
has_conflicting_tuple():
+ /*
+ * Return if any conflict is found other than one with 'ERROR'
+ * resolver configured. In case of 'ERROR' resolver, emit error here;
+ * otherwise return to caller for resolutions.
+ */
+ if (FindConflictTuple(resultRelInfo, estate, uniqueidx, slot,
+ &(*conflictslot)))

has_conflicting_tuple() is called only from ExecSimpleRelationInsert()
when the resolver of 'insert_exists' is not 'ERROR', then why do we
have the above comment in has_conflicting_tuple()?

3)
Since has_conflicting_tuple() is only called for insert_exists
conflict, better to name it as 'has_insert_conflicting_tuple' or
'find_insert_conflicting_tuple'. My preference is the second one,
similar to FindConflictTuple().

4)
We can have an ASSERT in has_conflicting_tuple() that conflict_type is
only insert_exists.

5)
has_conflicting_tuple():
+ }
+ return false;
+}

we can have a blank line before returning.

6)
Existing has_conflicting_tuple header comment:
+/*
+ * Check all the unique indexes for conflicts and return true if found.
+ * If the configured resolver is in favour of apply, give the conflicted
+ * tuple information in conflictslot.
+ */

Suggestion:
/*
* Check the unique indexes for conflicts. Return true on finding the
first conflict itself.
*
* If the configured resolver is in favour of apply, give the conflicted
* tuple information in conflictslot.
*/

<A change in first line and then a blank line.>

7)
Can we please rearrange 'has_conflicting_tuple' arguments. First
non-pointers, then single pointers and then double pointers.

Oid subid, ConflictType type, ConflictResolver resolver, bool
apply_remote, ResultRelInfo *resultRelInfo, EState *estate,
TupleTableSlot *slot, TupleTableSlot **conflictslot

8)
Now since we are doing a pre-scan of indexes before the actual
table-insert, this existing comment needs some change. Also we need to
mention why we are scanning again when we have done pre-scan already.

/*
* Checks the conflict indexes to fetch the
conflicting local tuple
* and reports the conflict. We perform this check
here, instead of
* performing an additional index scan before the
actual insertion and
* reporting the conflict if any conflicting tuples
are found. This is
* to avoid the overhead of executing the extra scan
for each INSERT
* operation, ....
*/

thanks
Shveta