Horton

Re: Table AM Interface Enhancements

Pavel Borisov <pashkin.elfe@gmail.com>

View thread View on postgr.es
From: Pavel Borisov <pashkin.elfe@gmail.com>
To: Japin Li <japinli@hotmail.com>
Cc: Alexander Korotkov <aekorotkov@gmail.com>, Mark Dilger <mark.dilger@enterprisedb.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-03-29T14:07:44Z
Lists: pgsql-hackers 
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

>

Commits

  1. Remove extra comment at TableAmRoutine.scan_analyze_next_block

  2. revert: Generalize relation analyze in table AM interface

  3. Revert: Allow table AM to store complex data structures in rd_amcache

  4. Revert: Allow table AM tuple_insert() method to return the different slot

  5. Revert: Allow locking updated tuples in tuple_update() and tuple_delete()

  6. Revert: Let table AM insertion methods control index insertion

  7. Revert: Custom reloptions for table AM

  8. Provide a way block-level table AMs could re-use acquire_sample_rows()

  9. Custom reloptions for table AM

  10. Use streaming I/O in ANALYZE.

  11. Revert "Custom reloptions for table AM"

  12. Let table AM insertion methods control index insertion

  13. Generalize relation analyze in table AM interface

  14. Improve error message for tts_(virtual|minimal)_is_current_xact_tuple

  15. Add comments on some MinimalTupleSlots methods usage

  16. Add TupleTableSlotOps.is_current_xact_tuple() method

  17. Allow table AM tuple_insert() method to return the different slot

  18. Allow table AM to store complex data structures in rd_amcache