Re: Table AM Interface Enhancements

On Fri, Mar 22, 2024 at 6:56 AM Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com> wrote:
>
> On Fri, 22 Mar 2024 at 08:51, Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)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