Re: Table AM Interface Enhancements

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

>