Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions

В письме от пятница, 4 июня 2021 г. 3:19:09 MSK пользователь Jeff Davis
написал:

> > Moving reloptions to AM code is the goal I am slowly moving to. I've
> > started
> > some time ago with big patch
> > https://commitfest.postgresql.org/14/992/ and
> > have been told to split it into smaller parts. So I did, and this
> > patch is
> > last step that cleans options related things up, and then actual
> > moving can be
> > done.
>
> Thank you for working on this.
Welcome!
Sorry for slow reply, I am on summer vacations now, no chance for fast replies
now :-)

> Can you outline the plan for moving these options to the table AM to
> make sure this patch is a step in the right direction?

Yes, I can. First you can see the whole patch, the way it was several years
ago: https://commitfest.postgresql.org/15/992/, reloptions8a.diff
The things I would say is accurate for postgres ~11, it may have been changed
since I last payed attention to them.

So, there are three general places where options can be stored:
1. Global boolRelOpts, intRelOpts, realRelOpts, stringRelOpts in src/backend/
access/common/reloptions.c for in-core access methods.

2. custom_options array of accessable via add_bool_reloption,
add_int_reloption, add_real_reloption, add_string_reloption for access methods
from contribs. (See reloptions.c too)

3. And also each access method has an array of relopt_parse_elt[] that is
also about reloptions.

1 and 2 are technically arrays of relopt_get, and store information what kind
of options do we have.

3 is array of relopt_parse_elt[] that store information how options should be
stored into binary representation.

My idea was to merge relopt_get and relopt_parse_elt into one structure (in my
patch it is "option_definition_basic"), each access method, that needs options,
should have a set of option_definition_basic for their options (called
option_definition_basic in my patch, may be should be renamed)

and this set of option_definitions is the only data that is needed to parse
option into binary representation.

So in access method instead of am_option function we will have
amrelopt_catalog function that returns "options defenition set" for this am,
and this definition set is used by option parser to parse options.

So, it my explanation is not quite clear, please ask questions, I will try to
answer them.

> I was trying to work through this problem as well[1], and there are a
> few complications.
>
> * Which options apply to any relation (of any table AM), and which
> apply to only heaps? As far as I can tell, the only one that seems
> heap-specific is "fillfactor".

From my point of view, each relation kind has it's own set of options.
The fact, that almost every kind has a fillfactor is just a coincidence.
If we try to do some optimization here, we will be buried under the complexity
of it soon. So they are _different_ options just having the same name.

> * Toast tables can be any AM, as well, so if we accept new reloptions
> for a custom AM, we also need to accept options for toast tables of
> that AM.

When I wrote this patch, AM was introduced only to index relations.
I do not how it is implemented for heap now, but there should be some logic in
it. If toast tables has it's own AM, then option definition set should be
stored there, and we should find a way to work with it, somehow.

> * Implementation-wise, the bytea representation of the options is not
> very easy to extend. Should we have a new text field in the catalog to
> hold the custom options?

I am not really understanding this question.

Normally all options can be well represented as binary structure stored at
bytea. I see no problem here. If we need some unusual behaviour, we can use
string option with custom validation function. This should cover almost all
needs I can imagine.

=======

So it you are interested in having better option implementation, and has no
ideas of your own, I would suggest to revive my patch and try to commit it.
What I need first of all is a reviewer. Testing and coauthoring will also be
apriciated.

My original big patch, I gave you link to, have been split into several parts.
The last minor part, that should be commit in advance, and have not been
commit yet is https://commitfest.postgresql.org/33/2370/
If you join as a reviewer this would be splendid! :-)

--
Nikolay Shaplov
dhyan(at)nataraj(dot)su (e-mail, jabber)
@dhyan:nataraj.su (matrix)