Re: Per-table storage parameters for TableAM/IndexAM extensions

On Wed, Dec 29, 2021 at 10:38 PM Sadhuprasad Patro <b(dot)sadhu(at)gmail(dot)com>
wrote:

> Hi,
>
> Currently all the storage options for a table are very much specific
> to the heap but a different AM might need some user defined AM
> specific parameters to help tune the AM. So here is a patch which
> provides an AM level routine so that instead of getting parameters
> validated using “heap_reloptions” it will call the registered AM
> routine.
>
>
This is a good idea. +1.

e.g:
> -- create a new access method and table using this access method
> CREATE ACCESS METHOD myam TYPE TABLE HANDLER <new_tableam_handler>;
>
> CREATE TABLE mytest (a int) USING myam ;
>
> --a new parameter is to set storage parameter for only myam as below
> ALTER TABLE mytest(squargle_size = '100MB');
>

I syntax here is, ALTER TABLE <table_name> SET ( attribute_option = value
);

> The user-defined parameters will have meaning only for the "myam",
> otherwise error will be thrown. Our relcache already allows the
> AM-specific cache to be stored for each relation.
>
> Open Question: When a user changes AM, then what should be the
> behavior for not supported storage options? Should we drop the options
> and go with only system storage options?
> Or throw an error, in which case the user has to clean the added
> parameters.
>

I think throwing an error makes more sense, so that the user can clean
that.

Here are a few quick cosmetic review comments:

1)

> @@ -1372,7 +1373,8 @@ untransformRelOptions(Datum options)
> */
> bytea *
> extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
> - amoptions_function amoptions)
> + amoptions_function amoptions,
> + reloptions_function taboptions)
>

Indentation has been changed and needs to be fixed.

2)

case RELKIND_MATVIEW:
> options = table_reloptions(taboptions, classForm->relkind,
> datum, false);
> break;
>

Going beyond line limit.

3)

diff --git a/src/backend/access/heap/heapam_handler.c
> b/src/backend/access/heap/heapam_handler.c
> index 9befe01..6324d7e 100644
> --- a/src/backend/access/heap/heapam_handler.c
> +++ b/src/backend/access/heap/heapam_handler.c
> @@ -2581,6 +2581,7 @@ static const TableAmRoutine heapam_methods = {
> .index_build_range_scan = heapam_index_build_range_scan,
> .index_validate_scan = heapam_index_validate_scan,
>
> + .taboptions = heap_reloptions,
>

Instead of taboptions can name this as relation_options to be in sink with
other members.

4)

@@ -2427,7 +2428,7 @@ do_autovacuum(void)
> */
> MemoryContextSwitchTo(AutovacMemCxt);
> tab = table_recheck_autovac(relid, table_toast_map, pg_class_desc,
> - effective_multixact_freeze_max_age);
> + classRel->rd_tableam->taboptions, effective_multixact_freeze_max_age);
> if (tab == NULL)
>

Split the another added parameter to function in the next line.

5)

Overall patch has many indentation issues, I would suggest running the
pgindent to fix those.

Regards
Rushabh Lathia
www.EnterpriseDB.com