Horton

Re: Table AM Interface Enhancements

Alexander Korotkov <aekorotkov@gmail.com>

View thread View on postgr.es
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Pavel Borisov <pashkin.elfe@gmail.com>
Cc: Japin Li <japinli@hotmail.com>, Mark Dilger <mark.dilger@enterprisedb.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-03-26T23:22:35Z
Lists: pgsql-hackers

Attachments

On Fri, Mar 22, 2024 at 6:56 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>
> On Fri, 22 Mar 2024 at 08:51, Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>>
>> Hi, Alexander!
>>
>> Thank you for working on this patchset and pushing some of these patches!
>>
>> I tried to write comments for tts_minimal_is_current_xact_tuple() and tts_minimal_getsomeattrs() for them to be the same as for the same functions for heap and virtual tuple slots, as I proposed above in the thread. (tts_minimal_getsysattr is not introduced by the current patchset, but anyway)
>>
>> Meanwhile I found that (never appearing) error message for tts_minimal_is_current_xact_tuple needs to be corrected. Please see the patch in the attachment.
>>
> I need to correct myself: it's for  tts_minimal_getsysattr() not tts_minimal_getsomeattrs()

Pushed.

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().

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.

0007 (was 0012) – This patch doesn't make much sense if not removing
ROW_MARK_COPY.  What an oversight by me!  I managed to remove
ROW_MARK_COPY so that tests passed.  Added a lot of comments and made
other improvements.  But the problem is that I didn't manage to
research all the consequences of this patch to FDW.  And I think there
are open design questions.  In particular how should ROW_REF_COPY work
with row marks other than ROW_MARK_REFERENCE and should it work at
all?  This would require some consensus, and it doesn't seem feasible
to achieve before FF.  So, I think this is not a subject for v17.

Other patches are without changes.

------
Regards,
Alexander Korotkov

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