Horton

Re: Table AM Interface Enhancements

Pavel Borisov <pashkin.elfe@gmail.com>

View thread View on postgr.es
From: Pavel Borisov <pashkin.elfe@gmail.com>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Japin Li <japinli@hotmail.com>, Mark Dilger <mark.dilger@enterprisedb.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-03-21T05:36:14Z
Lists: pgsql-hackers 
Hi, Alexander!


On Wed, 20 Mar 2024 at 09:22, Pavel Borisov <pashkin.elfe@gmail.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

Commits

  1. Remove extra comment at TableAmRoutine.scan_analyze_next_block

  2. revert: Generalize relation analyze in table AM interface

  3. Revert: Allow table AM to store complex data structures in rd_amcache

  4. Revert: Allow table AM tuple_insert() method to return the different slot

  5. Revert: Allow locking updated tuples in tuple_update() and tuple_delete()

  6. Revert: Let table AM insertion methods control index insertion

  7. Revert: Custom reloptions for table AM

  8. Provide a way block-level table AMs could re-use acquire_sample_rows()

  9. Custom reloptions for table AM

  10. Use streaming I/O in ANALYZE.

  11. Revert "Custom reloptions for table AM"

  12. Let table AM insertion methods control index insertion

  13. Generalize relation analyze in table AM interface

  14. Improve error message for tts_(virtual|minimal)_is_current_xact_tuple

  15. Add comments on some MinimalTupleSlots methods usage

  16. Add TupleTableSlotOps.is_current_xact_tuple() method

  17. Allow table AM tuple_insert() method to return the different slot

  18. Allow table AM to store complex data structures in rd_amcache