On Tue, Oct 1, 2024 at 9:48 AM shveta malik <[email protected]> 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
