I've looked at patch 0003.
Generally, it does a similar thing as 0001 - it exposes a more generalized method tuple_insert_with_arbiter that encapsulates tuple_insert_speculative/tuple_complete_speculative and at the same time allows extensibility of this i.e. different implementation for custom table other than heap by the extensions. Though the code rearrangement is little bit more complicated, the patch is clear. It doesn't change the behavior for heap tables.
tuple_insert_speculative/tuple_complete_speculative are removed from table AM methods. I think it would not be hard for existing users of this to adapt to the changes.
Code block ExecCheckTupleVisible -- ExecCheckTIDVisible moved to heapam_handler.c I've checked, the code block unchanged except that ExecCheckTIDVisible now gets Relation from the caller instead of constructing it from ResultRelInfo.
Also two big code blocks are moved from ExecOnConflictUpdate and ExecInsert to a new method heapam_tuple_insert_with_arbiter. They correspond the old code with several minor modifications.
For ExecOnConflictUpdate comment need to be revised. This one is for shifted code:
> * Try to lock tuple for update as part of speculative insertion.
Probably it is worth to be moved to a comment for heapam_tuple_insert_with_arbiter.
For heapam_tuple_insert_with_arbiter both "return NULL" could be shifted level up into the end of the block:
>if (!ExecCheckIndexConstraints(resultRelInfo, slot, estate, &conflictTid,
>+ arbiterIndexes))
Also I'd add comment for heapam_tuple_insert_with_arbiter:
/* See comments for table_tuple_insert_with_arbiter() */
A comment to be corrected:
src/backend/access/heap/heapam.c: * implement table_tuple_insert_speculative()
As Jaipin said, I'd also propose removing "inline" from heapam_tuple_insert_with_arbiter.
More corrections for comments:
%s/If tuple doesn't violates/If tuple doesn't violate/g
%s/which comprises the list of/list, which comprises/g
%s/conflicting tuple gets locked/conflicting tuple should be locked/g
I think for better code look this could be removed:
>vlock:
> CHECK_FOR_INTERRUPTS();
together with CHECK_FOR_INTERRUPTS(); in heapam_tuple_insert_with_arbiter placed in the beginning of while loop.
Overall the patch looks good enough to me.
Regards,
Pavel
