Re: Table AM Interface Enhancements

Hi, Pavel!

Thank you for your feedback.  The revised patch set is attached.

I found that vacuum.c has a lot of heap-specific code.  Thus, it
should be OK for analyze.c to keep heap-specific code.  Therefore, I
rolled back the movement of functions between files.  That leads to a
smaller patch, easier to review.

On Wed, Mar 27, 2024 at 2:52 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>> 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.

I found that the function name acquire_sample_rows is referenced in
quite several places in the source code.  So, I would prefer to save
the old name to keep the changes minimal.

src/backend/access/heap/vacuumlazy.c:            * match what analyze.c's acquire_sample_rows() does, otherwise VACUUM
src/backend/access/heap/vacuumlazy.c:            * The logic here is a bit simpler than acquire_sample_rows(), as
src/backend/access/heap/vacuumlazy.c:                            * what acquire_sample_rows() does.
src/backend/access/heap/vacuumlazy.c:                            * acquire_sample_rows() does, so be consistent.
src/backend/access/heap/vacuumlazy.c:            * acquire_sample_rows() will recognize the same LP_DEAD items as dead
src/backend/commands/analyze.c:static int       acquire_sample_rows(Relation onerel, int elevel,
src/backend/commands/analyze.c:         acquirefunc = acquire_sample_rows;
src/backend/commands/analyze.c: * acquire_sample_rows -- acquire a random sample of rows from the table
src/backend/commands/analyze.c:acquire_sample_rows(Relation onerel, int elevel,
src/backend/commands/analyze.c: * This has the same API as acquire_sample_rows, except that rows are
src/backend/commands/analyze.c:                 acquirefunc = acquire_sample_rows;

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

Ok!

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

With the patch, this method doesn't have usages outside of table am.
I don't think we should keep a method, which doesn't have clear
external usage patterns.  But I agree that starting a scan with
heap_beginscan() and ending with table_endscan() is not correct.  Now
ending this scan with heap_endscan().

> Also, a comment about it was introduced in v5:
>
> src/backend/access/heap/heapam_handler.c: * with table_beginscan_analyze()

Corrected.

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

Fixed.

>> 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

Fixed.

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

This is minor redundancy.  I prefer to keep it.  This makes it obvious
that patch just moved this code.