Hi, Alexander!
For 0007:
Code inside
+heapam_reloptions(char relkind, Datum reloptions, bool validate)
+{
+ if (relkind == RELKIND_RELATION ||
+ relkind == RELKIND_TOASTVALUE ||
+ relkind == RELKIND_MATVIEW)
+ return heap_reloptions(relkind, reloptions, validate);
+
+ return NULL;
looks redundant to what is done inside heap_reloptions(). Was this on purpose? Is it possible to leave only "return heap_reloptions()" ?
This looks like a duplicate:
src/include/access/reloptions.h:extern bytea *index_reloptions(amoptions_function amoptions, Datum reloptions,
src/include/access/tableam.h:extern bytea *index_reloptions(amoptions_function amoptions, Datum reloptions,
Otherwise the patch looks good and doing what it's proposed to do.
For patch 0006:
The change for analyze is in the same style as for previous table am extensibility patches.
table_scan_analyze_next_tuple/table_scan_analyze_next_block existing extensibility is dropped in favour of more general method table_relation_analyze. I haven't found existing extensions on a GitHub that use these table am's, so probably it's quite ok to remove the extensibility that didn't get any traction for many years.
The patch contains a big block of code copy-paste. I've checked that the code is the same with only function name replacement in favor to using table am instead of heap am. I'd propose restoring the static functions declaration in the head of the file, which was removed in the patch and place heapam_acquire_sample_rows() above compare_rows() to make functions copied as the whole code block. This is for better patch look only, not a principal change.
-static int acquire_sample_rows(Relation onerel, int elevel,
- HeapTuple *rows, int targrows,
- double *totalrows, double *totaldeadrows);
-static int compare_rows(const void *a, const void *b, void *arg)
May it also be a better place than vacuum.h for
typedef int (*AcquireSampleRowsFunc) ? Maybe sampling.h ?
The other patch that I'd like to review is 0012:
For a
typedef enum RowRefType
I think some comments would be useful to avoid confusion about the changes like
- newrc->allMarkTypes = (1 << newrc->markType);
+ newrc->allRefTypes = (1 << refType);
Also I think the semantical difference between ROW_REF_COPY and ROW_MARK_COPY is better to be mentioned in the comments and/or commit message. This may include a description of assigning different reftypes in parse_relation.c
In a comment there is a small confusion between markType and refType:
* The parent's allRefTypes field gets the OR of (1<<refType) across all
* its children (this definition allows children to use different markTypes).
Both patches look good to me and are ready, though they may need minimal comments/cosmetic work.
Regards,
Pavel Borisov