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.