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.
>

+1 for the idea.

>
> 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');
>

This will work for CREATE TABLE as well I guess as normal relation storage
parameter works now right?

> 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.
>

IMHO, if the user is changing the access method for the table then it
should be fine to throw an error if there are some parameters which are not
supported by the new AM. So that user can take a calculative call and
first remove those storage options before changing the AM.

I have a few comments on the patch, mostly cosmetics.

1.
+ Assert(routine->taboptions != NULL);

Why AM is not allowed to register the NULL function, if NULL is registered
that means the AM
does not support any of the storage parameters.
2.
@@ -1358,6 +1358,7 @@ untransformRelOptions(Datum options)
return result;
}

+
/*
* Extract and parse reloptions from a pg_class tuple.
*

Unwanted hunk (added extra line)

3.
+ * Parse options for heaps, views and toast tables. This is
+ * implementation of relOptions for access method heapam.
*/

Better to say access method heap instead of heapam.
4.
+ * Parse options for tables.
+ *
+ * taboptions tables AM's option parser function
+ * reloptions options as text[] datum
+ * validate error flag

Function header comment formatting is not proper, it also has uneven
spacing between words.
5.
-extract_autovac_opts(HeapTuple tup, TupleDesc pg_class_desc)
+extract_autovac_opts(HeapTuple tup, TupleDesc pg_class_desc,
+ reloptions_function taboptions)

Indentation is not proper, run pgindent on this.

5.
>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.

Wrap these long commit message lines at 80 characters.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com