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: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Japin Li <japinli@hotmail.com>, Mark Dilger <mark.dilger@enterprisedb.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-03-27T12:51:54Z
Lists: pgsql-hackers 
Hi, Alexander!

The revised rest of the patchset is attached.
> 0001 (was 0006) – I prefer the definition of AcquireSampleRowsFunc to
> stay in vacuum.h.  If we move it to sampling.h then we would have to
> add there includes to define Relation, HeapTuple etc.  I'd like to
> avoid this kind of change.  Also, I've deleted
> table_beginscan_analyze(), because it's only called from
> tableam-specific AcquireSampleRowsFunc.  Also I put some comments to
> heapam_scan_analyze_next_block() and heapam_scan_analyze_next_tuple()
> given that there are now no relevant comments for them in tableam.h.
> I've removed some redundancies from acquire_sample_rows().  And added
> comments to AcquireSampleRowsFunc based on what we have in FDW docs
> for this function.  Did some small edits as well.  As you suggested,
> turned back declarations for acquire_sample_rows() and compare_rows().
>

In my comment in the thread I was not thinking about returning the old name
acquire_sample_rows(), it was only about the declarations and the order of
functions to be one code block. To me heapam_acquire_sample_rows() looks
better for a name of heap implementation of *AcquireSampleRowsFunc(). I
suggest returning the name heapam_acquire_sample_rows() from v4. Sorry for
the confusion in this.

The changed type of static function that always returned true for heap
looks good to me:
static void heapam_scan_analyze_next_block

The same is for removing the comparison of always true "block_accepted" in
(heapam_)acquire_sample_rows()

Removing table_beginscan_analyze and call scan_begin() is not in the same
style as other table_beginscan_* functions. Though this is not a change in
functionality, I'd leave this part as it was in v4. Also, a comment about
it was introduced in v5:

src/backend/access/heap/heapam_handler.c: * with table_beginscan_analyze()

For comments I'd propose:
%s/In addition, store estimates/In addition, a function should store
estimates/g
%s/zerp/zero/g


> 0002 (was 0007) – I've turned the redundant "if", which you've pointed
> out, into an assert.  Also, added some comments, most notably comment
> for TableAmRoutine.reloptions based on the indexam docs.
>

%s/validate sthe/validates the/g

This seems not needed, it's already inited to InvalidOid before.
+else
+accessMethod = default_table_access_method;

+       accessMethodId = InvalidOid;

This code came from 374c7a22904. I don't insist on this simplification in a
patch 0002.

Overall both patches look good to me.

Regards,
Pavel Borisov.

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