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.
