On Tue, 2022-01-18 at 22:44 +0530, Sadhuprasad Patro wrote:
> As of now, I have fixed the comments from Dilip & Rushabh and have
> done some more changes after internal testing and review. Please find
> the latest patch attached.
Hi,
Thank you for working on this! Some questions/comments:
At a high level, it seems there are some options that are common to all
tables, regardless of the AM. For instance, the vacuum/autovacuum
options. (Even if the AM doesn't require vacuum, then it needs to at
least be able to communicate that somehow.) I think parallel_workers
and user_catalog_table also fit into this category. That means we need
all of StdRdOptions to be the same, with the possible exception of
toast_tuple_target and/or fillfactor.
The current patch just leaves it up to the AM to return a bytea that
can be cast to StdRdOptions, which seems like a fragile API.
That makes me think that what we really want is to have *extra* options
for a table AM, not an entirely custom set. Do you agree?
If so, I suggest you refactor so that if validation doesn't recognize a
parameter, it calls a table AM method to validate it, and lets it in if
validation succeeds. That way all the stuff around StdRdOptions is
unchanged. When the table AM needs the parameter value, it can parse
pg_class.reloptions for itself and save it in rd_amcache.
Regards,
Jeff Davis