Re: Table AM Interface Enhancements

Hi, Alexander!

On Wed, 20 Mar 2024 at 09:22, Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com> wrote:

> 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