Thread: [HACKERS] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM

(Please see Attribution notice at the bottom of the letter)

Hi! Some time ago I've send a proposal, about removing all am-related code 
from src/backend/access/common/reloptions.c and move it into somewhere inside 
am code. It was in 
https://www.postgresql.org/message-id/4636505.Bu9AKW1Kzc%40nataraj-amd64
thread.

Now I have a patch that is ready

Before explaining what have been done, I need to define some concepts, so it 
would be easy to speak about them in the further explanation.

Reloptions values in postgres actually exists in four representations: 
DefList*, TextArray[], Bytea and so called Values.

- DefList representation is a list of DefElem, it is the representation in 
which reloptions comes from syntax analyzer, when relation is created or 
altered, and also this representation that is needed for pg_dump to dump a 
create statement that creates this relation.

- TextArray[] representation is a way, how reloptions are stored in 
pg_catalog. It is TEXT[] attribute in the pg_catalog table and from the poinf 
o view of postgres code it is Datum array of TEXT Datums, when read into the 
memory

-  Bytea (or I will also call it Binary) representation, is a representation 
with which relation code really works. It has C-structure inside in which 
fixed-length options are sored, some space is reserved for storing varlen 
values after the structure, and certainly it has BYTEA header in front of all 
of it. It is cached, it is fast to access. It is a good stuff.

- Values (or in some cases RawValues) -- internal representation of option 
parser. Technically it is List of option_value structures (old name for it is 
relopt_value). This representation is used for parsing and validating options. 
Actually all the convertations between representations listed above are dove 
through Values representation. Values are called RawValues when Values List 
were already created, but it was not parsed yet, and all option values are in 
raw state: represented as a text.


DefList and TextArray representations are converted in both ways(always 
through Values representation): we need DefList -> TextArray to create or 
change entry in the pg_catalog, and need TextArray -> DefList for pg_dump

Bytea representation is converted from TextArray representation (also through 
Values representation). When relation code, tries to get cached 
Bytea representation for certain relation, and none is actually cached, then 
cache code calls extractRelOptions function that fetches entry from pg_catalog 
for that relation, gets reloptions  TextArray[], converts it into Bytea and 
cache it.


Each reloption has it's own definition. This definitions tells how this option 
should be parsed, validated and converted. Before my patch this information 
were stored in relopt_gen and relopt_parse_elt structures. In my patch all 
this information were moved into option_definition_basic structure (that is 
actually old relopt_gen + relopt_parse_elt)

The set of options definition I would call a options catalog. Before my patch 
there was one global options catalog (actually it was divided into parts by 
data types and has another part for dynamically added options, but 
nevertheless, it was global). After my patch there would be a separate options 
catalog for each relation type. For AM-relation this catalog would be 
available via AM-handler. for other relation types it would be still kept in 
reloption.c



Now I am ready to tell what actually have been done in this patch:

1. I moved options definitions from global lists from reloptions.c 
[type name]RelOpts to lists that are kept inside AM code. One list per AM.  
heap options, toast options and options for all other relation types that are 
not accessible through  AccessMethod, all were kept inside reloptions.c, but 
now it is one list for each relation type, not a global one

2. Each AccessMethod had amoptions function that was responsible for 
converting reloptions from TextArray[] representation into Bytea 
representaions. This function used functions from reloptions.c and also had
an array of relopt_parse_elt that defines how parsed reloption data should be 
packed inside Bytea chunk. I've moved data from relopt_parse_elt array into 
option definitions that are stored in the catalog (since catalog are  now 
defined inside the AccessMethod we have access to a structure of Bytea 
representation at the place where we define option catalog)

3. Speaking of amoptions function, I've completely removed it, since we do not 
need it. For developers of Access Methods who still want to do some custom 
action with just parsed data, I've added postprocess_fun to option catalog. If 
this function is defined, it is called by convertation function right after 
Bytea representation were created. In postprocess_fun developer can do some 
custom validation, or change just parsed data. This feature is used in bloom 
index.

4. Instead of amoptions function I've added amrelopt_catalog function to the 
Access Method. This function returns option definition catalog for an Access 
Method. The catalog is used for processing options.

5. As before, relation cache calls extractRelOptions when it needs options 
that were not previously cached.
Before my patch, it created Bytea representations for non-AM relations, and 
called amoptions AM function to get Bytea representation for AM relation.
In by patch, it now gets option catalog, (using local functions for non-AM 
relations, and  using amrelopt_catalog function for AM-relations), and then 
use this catalog to convert options from TextArray into Bytea representation.

6. I've added some more featues: 

- Now you can set OPTION_DEFINITION_FLAG_FORBID_ALTER flag in definition of the 
option, and then postgres will refuse to change option value using ALTER 
command. In many indexes, some options are used only for index creation. After 
this it's value is saved in MetaPage, and used from there. Nevertheless user 
is allowed to change option value, though it affects nothing, one need to 
recreate index to really change such value. Now using this flag we can prevent 
user from getting an illusion that he successfully changed option value.

- After applying my patch if you try to set toast. option for table that does 
not have toast, you will get an error. Before postgres will accept toast 
option for a table without a toast, but this value will be lost. This is bad 
behavior, so now postgres will throw an error in this case.

- I've noticed that all string relation options in postgres are technically 
enum options. So I've added enum option type. This will save some bytes of the 
memory and some tacts of the CPU. I also left string option type (through it 
is not used now). A. Korotkov said that it might be needed later for storing 
patterns, xml/json paths or something like that.

- Added some more tests that will trigger more options code. Though I am 
dreaming about more advanced test system, that will allow to check that 
certain value is received somewhere deep in the code.

7. And now the most controversial change. I've got rid of  StdRdOptions 
structure, in which options for heap, toast, nbtree, hash and spgist were 
stored in Bytea representation. In indexes only fillfactor value were actually 
used from the whole big structure. So I've created a separate structure for 
each relation type. HeapOptions and ToastOptions are very similar to each 
other. So common part of creating these catalogs were moved to 
add_autovacuum_options function that are called while creating each catalog.

Now to the controversial part: in src/include/utils/rel.h had a lot of 
Relation* macroses that used StdRdOptions structure. I've changed them into 
View* Heap* and Toast* analogues, and changed the code to use these macroses 
instead of Relation* one. But this part of code I least sure of. I'd like to 
ask experienced developers to double check it.

8. I've moved all options-abstract code into options.c and options.h file, and 
left in reloptions.c and reloptions.h only the code that concerns relation
options. Actually the main idea of this patch was to get this abstract code in 
order to use it for custom attribute options later. All the rest just a side 
effects :-)

So, before adding this to commitfest I want somebody to give a general look at 
it, if the code is OK.

Alvaro Herrera, you once said that you can review the patch...

The patch is available in the attachment.  It can be applied to current master

You can also see latest version on my github 
https://github.com/dhyannataraj/postgres/tree/reloption_fix
at the reloption_fix branch.


ATTRIBUTION NOTICE: I wrote this patch, when I was an  employee in Postgres 
Professional company. So this patch should be attributed as patch from 
Postgres Pro (or something like that). For some reasons I did not manage to 
commit this patch while I left that job. But I got a permission to commit it 
even if I left the company.
So my contribution to this patch as a independent developer was final code 
cleanup, and writing some more comments. All the rest was a work of Postgres 
Pro employee Nikolay Shaplov.

-- 
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

В письме от 6 февраля 2017 23:30:03 пользователь Nikolay Shaplov написал:

I've rebased the patch, so it can be applied to current master. See
attachment.

Alvaro in private letter told that he will review the patch some time later.
So I am waiting.

Also added this patch to commitfest:
https://commitfest.postgresql.org/13/992/


> (Please see Attribution notice at the bottom of the letter)
>
> Hi! Some time ago I've send a proposal, about removing all am-related code
> from src/backend/access/common/reloptions.c and move it into somewhere
> inside am code. It was in
> https://www.postgresql.org/message-id/4636505.Bu9AKW1Kzc%40nataraj-amd64
> thread.
>
> Now I have a patch that is ready
>
> Before explaining what have been done, I need to define some concepts, so it
> would be easy to speak about them in the further explanation.
>
> Reloptions values in postgres actually exists in four representations:
> DefList*, TextArray[], Bytea and so called Values.
>
> - DefList representation is a list of DefElem, it is the representation in
> which reloptions comes from syntax analyzer, when relation is created or
> altered, and also this representation that is needed for pg_dump to dump a
> create statement that creates this relation.
>
> - TextArray[] representation is a way, how reloptions are stored in
> pg_catalog. It is TEXT[] attribute in the pg_catalog table and from the
> poinf o view of postgres code it is Datum array of TEXT Datums, when read
> into the memory
>
> -  Bytea (or I will also call it Binary) representation, is a representation
> with which relation code really works. It has C-structure inside in which
> fixed-length options are sored, some space is reserved for storing varlen
> values after the structure, and certainly it has BYTEA header in front of
> all of it. It is cached, it is fast to access. It is a good stuff.
>
> - Values (or in some cases RawValues) -- internal representation of option
> parser. Technically it is List of option_value structures (old name for it
> is relopt_value). This representation is used for parsing and validating
> options. Actually all the convertations between representations listed
> above are dove through Values representation. Values are called RawValues
> when Values List were already created, but it was not parsed yet, and all
> option values are in raw state: represented as a text.
>
>
> DefList and TextArray representations are converted in both ways(always
> through Values representation): we need DefList -> TextArray to create or
> change entry in the pg_catalog, and need TextArray -> DefList for pg_dump
>
> Bytea representation is converted from TextArray representation (also
> through Values representation). When relation code, tries to get cached
> Bytea representation for certain relation, and none is actually cached, then
> cache code calls extractRelOptions function that fetches entry from
> pg_catalog for that relation, gets reloptions  TextArray[], converts it
> into Bytea and cache it.
>
>
> Each reloption has it's own definition. This definitions tells how this
> option should be parsed, validated and converted. Before my patch this
> information were stored in relopt_gen and relopt_parse_elt structures. In
> my patch all this information were moved into option_definition_basic
> structure (that is actually old relopt_gen + relopt_parse_elt)
>
> The set of options definition I would call a options catalog. Before my
> patch there was one global options catalog (actually it was divided into
> parts by data types and has another part for dynamically added options, but
> nevertheless, it was global). After my patch there would be a separate
> options catalog for each relation type. For AM-relation this catalog would
> be available via AM-handler. for other relation types it would be still
> kept in reloption.c
>
>
>
> Now I am ready to tell what actually have been done in this patch:
>
> 1. I moved options definitions from global lists from reloptions.c
> [type name]RelOpts to lists that are kept inside AM code. One list per AM.
> heap options, toast options and options for all other relation types that
> are not accessible through  AccessMethod, all were kept inside
> reloptions.c, but now it is one list for each relation type, not a global
> one
>
> 2. Each AccessMethod had amoptions function that was responsible for
> converting reloptions from TextArray[] representation into Bytea
> representaions. This function used functions from reloptions.c and also had
> an array of relopt_parse_elt that defines how parsed reloption data should
> be packed inside Bytea chunk. I've moved data from relopt_parse_elt array
> into option definitions that are stored in the catalog (since catalog are
> now defined inside the AccessMethod we have access to a structure of Bytea
> representation at the place where we define option catalog)
>
> 3. Speaking of amoptions function, I've completely removed it, since we do
> not need it. For developers of Access Methods who still want to do some
> custom action with just parsed data, I've added postprocess_fun to option
> catalog. If this function is defined, it is called by convertation function
> right after Bytea representation were created. In postprocess_fun developer
> can do some custom validation, or change just parsed data. This feature is
> used in bloom index.
>
> 4. Instead of amoptions function I've added amrelopt_catalog function to the
> Access Method. This function returns option definition catalog for an
> Access Method. The catalog is used for processing options.
>
> 5. As before, relation cache calls extractRelOptions when it needs options
> that were not previously cached.
> Before my patch, it created Bytea representations for non-AM relations, and
> called amoptions AM function to get Bytea representation for AM relation.
> In by patch, it now gets option catalog, (using local functions for non-AM
> relations, and  using amrelopt_catalog function for AM-relations), and then
> use this catalog to convert options from TextArray into Bytea
> representation.
>
> 6. I've added some more featues:
>
> - Now you can set OPTION_DEFINITION_FLAG_FORBID_ALTER flag in definition of
> the option, and then postgres will refuse to change option value using
> ALTER command. In many indexes, some options are used only for index
> creation. After this it's value is saved in MetaPage, and used from there.
> Nevertheless user is allowed to change option value, though it affects
> nothing, one need to recreate index to really change such value. Now using
> this flag we can prevent user from getting an illusion that he successfully
> changed option value.
>
> - After applying my patch if you try to set toast. option for table that
> does not have toast, you will get an error. Before postgres will accept
> toast option for a table without a toast, but this value will be lost. This
> is bad behavior, so now postgres will throw an error in this case.
>
> - I've noticed that all string relation options in postgres are technically
> enum options. So I've added enum option type. This will save some bytes of
> the memory and some tacts of the CPU. I also left string option type
> (through it is not used now). A. Korotkov said that it might be needed
> later for storing patterns, xml/json paths or something like that.
>
> - Added some more tests that will trigger more options code. Though I am
> dreaming about more advanced test system, that will allow to check that
> certain value is received somewhere deep in the code.
>
> 7. And now the most controversial change. I've got rid of  StdRdOptions
> structure, in which options for heap, toast, nbtree, hash and spgist were
> stored in Bytea representation. In indexes only fillfactor value were
> actually used from the whole big structure. So I've created a separate
> structure for each relation type. HeapOptions and ToastOptions are very
> similar to each other. So common part of creating these catalogs were moved
> to
> add_autovacuum_options function that are called while creating each catalog.
>
> Now to the controversial part: in src/include/utils/rel.h had a lot of
> Relation* macroses that used StdRdOptions structure. I've changed them into
> View* Heap* and Toast* analogues, and changed the code to use these macroses
> instead of Relation* one. But this part of code I least sure of. I'd like
> to ask experienced developers to double check it.
>
> 8. I've moved all options-abstract code into options.c and options.h file,
> and left in reloptions.c and reloptions.h only the code that concerns
> relation options. Actually the main idea of this patch was to get this
> abstract code in order to use it for custom attribute options later. All
> the rest just a side effects :-)
>
> So, before adding this to commitfest I want somebody to give a general look
> at it, if the code is OK.
>
> Alvaro Herrera, you once said that you can review the patch...
>
> The patch is available in the attachment.  It can be applied to current
> master
>
> You can also see latest version on my github
> https://github.com/dhyannataraj/postgres/tree/reloption_fix
> at the reloption_fix branch.
>
>
> ATTRIBUTION NOTICE: I wrote this patch, when I was an  employee in Postgres
> Professional company. So this patch should be attributed as patch from
> Postgres Pro (or something like that). For some reasons I did not manage to
> commit this patch while I left that job. But I got a permission to commit it
> even if I left the company.
> So my contribution to this patch as a independent developer was final code
> cleanup, and writing some more comments. All the rest was a work of Postgres
> Pro employee Nikolay Shaplov.

--
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I've rebased the patch again.

Alvaro, I am waiting for you! ;-)

> I've rebased the patch, so it can be applied to current master. See
> attachment.
> 
> Alvaro in private letter told that he will review the patch some time later.
> So I am waiting.
> 
> Also added this patch to commitfest:
> https://commitfest.postgresql.org/13/992/
> 
> > (Please see Attribution notice at the bottom of the letter)
> > 
> > Hi! Some time ago I've send a proposal, about removing all am-related code
> > from src/backend/access/common/reloptions.c and move it into somewhere
> > inside am code. It was in
> > https://www.postgresql.org/message-id/4636505.Bu9AKW1Kzc%40nataraj-amd64
> > thread.
> > 
> > Now I have a patch that is ready
> > 
> > Before explaining what have been done, I need to define some concepts, so
> > it would be easy to speak about them in the further explanation.
> > 
> > Reloptions values in postgres actually exists in four representations:
> > DefList*, TextArray[], Bytea and so called Values.
> > 
> > - DefList representation is a list of DefElem, it is the representation in
> > which reloptions comes from syntax analyzer, when relation is created or
> > altered, and also this representation that is needed for pg_dump to dump a
> > create statement that creates this relation.
> > 
> > - TextArray[] representation is a way, how reloptions are stored in
> > pg_catalog. It is TEXT[] attribute in the pg_catalog table and from the
> > poinf o view of postgres code it is Datum array of TEXT Datums, when read
> > into the memory
> > 
> > -  Bytea (or I will also call it Binary) representation, is a
> > representation with which relation code really works. It has C-structure
> > inside in which fixed-length options are sored, some space is reserved
> > for storing varlen values after the structure, and certainly it has BYTEA
> > header in front of all of it. It is cached, it is fast to access. It is a
> > good stuff.
> > 
> > - Values (or in some cases RawValues) -- internal representation of option
> > parser. Technically it is List of option_value structures (old name for it
> > is relopt_value). This representation is used for parsing and validating
> > options. Actually all the convertations between representations listed
> > above are dove through Values representation. Values are called RawValues
> > when Values List were already created, but it was not parsed yet, and all
> > option values are in raw state: represented as a text.
> > 
> > 
> > DefList and TextArray representations are converted in both ways(always
> > through Values representation): we need DefList -> TextArray to create or
> > change entry in the pg_catalog, and need TextArray -> DefList for pg_dump
> > 
> > Bytea representation is converted from TextArray representation (also
> > through Values representation). When relation code, tries to get cached
> > Bytea representation for certain relation, and none is actually cached,
> > then cache code calls extractRelOptions function that fetches entry from
> > pg_catalog for that relation, gets reloptions  TextArray[], converts it
> > into Bytea and cache it.
> > 
> > 
> > Each reloption has it's own definition. This definitions tells how this
> > option should be parsed, validated and converted. Before my patch this
> > information were stored in relopt_gen and relopt_parse_elt structures. In
> > my patch all this information were moved into option_definition_basic
> > structure (that is actually old relopt_gen + relopt_parse_elt)
> > 
> > The set of options definition I would call a options catalog. Before my
> > patch there was one global options catalog (actually it was divided into
> > parts by data types and has another part for dynamically added options,
> > but
> > nevertheless, it was global). After my patch there would be a separate
> > options catalog for each relation type. For AM-relation this catalog would
> > be available via AM-handler. for other relation types it would be still
> > kept in reloption.c
> > 
> > 
> > 
> > Now I am ready to tell what actually have been done in this patch:
> > 
> > 1. I moved options definitions from global lists from reloptions.c
> > [type name]RelOpts to lists that are kept inside AM code. One list per AM.
> > heap options, toast options and options for all other relation types that
> > are not accessible through  AccessMethod, all were kept inside
> > reloptions.c, but now it is one list for each relation type, not a global
> > one
> > 
> > 2. Each AccessMethod had amoptions function that was responsible for
> > converting reloptions from TextArray[] representation into Bytea
> > representaions. This function used functions from reloptions.c and also
> > had
> > an array of relopt_parse_elt that defines how parsed reloption data should
> > be packed inside Bytea chunk. I've moved data from relopt_parse_elt array
> > into option definitions that are stored in the catalog (since catalog are
> > now defined inside the AccessMethod we have access to a structure of Bytea
> > representation at the place where we define option catalog)
> > 
> > 3. Speaking of amoptions function, I've completely removed it, since we do
> > not need it. For developers of Access Methods who still want to do some
> > custom action with just parsed data, I've added postprocess_fun to option
> > catalog. If this function is defined, it is called by convertation
> > function
> > right after Bytea representation were created. In postprocess_fun
> > developer
> > can do some custom validation, or change just parsed data. This feature is
> > used in bloom index.
> > 
> > 4. Instead of amoptions function I've added amrelopt_catalog function to
> > the Access Method. This function returns option definition catalog for an
> > Access Method. The catalog is used for processing options.
> > 
> > 5. As before, relation cache calls extractRelOptions when it needs options
> > that were not previously cached.
> > Before my patch, it created Bytea representations for non-AM relations,
> > and
> > called amoptions AM function to get Bytea representation for AM relation.
> > In by patch, it now gets option catalog, (using local functions for non-AM
> > relations, and  using amrelopt_catalog function for AM-relations), and
> > then
> > use this catalog to convert options from TextArray into Bytea
> > representation.
> > 
> > 6. I've added some more featues:
> > 
> > - Now you can set OPTION_DEFINITION_FLAG_FORBID_ALTER flag in definition
> > of
> > the option, and then postgres will refuse to change option value using
> > ALTER command. In many indexes, some options are used only for index
> > creation. After this it's value is saved in MetaPage, and used from there.
> > Nevertheless user is allowed to change option value, though it affects
> > nothing, one need to recreate index to really change such value. Now using
> > this flag we can prevent user from getting an illusion that he
> > successfully
> > changed option value.
> > 
> > - After applying my patch if you try to set toast. option for table that
> > does not have toast, you will get an error. Before postgres will accept
> > toast option for a table without a toast, but this value will be lost.
> > This
> > is bad behavior, so now postgres will throw an error in this case.
> > 
> > - I've noticed that all string relation options in postgres are
> > technically
> > enum options. So I've added enum option type. This will save some bytes of
> > the memory and some tacts of the CPU. I also left string option type
> > (through it is not used now). A. Korotkov said that it might be needed
> > later for storing patterns, xml/json paths or something like that.
> > 
> > - Added some more tests that will trigger more options code. Though I am
> > dreaming about more advanced test system, that will allow to check that
> > certain value is received somewhere deep in the code.
> > 
> > 7. And now the most controversial change. I've got rid of  StdRdOptions
> > structure, in which options for heap, toast, nbtree, hash and spgist were
> > stored in Bytea representation. In indexes only fillfactor value were
> > actually used from the whole big structure. So I've created a separate
> > structure for each relation type. HeapOptions and ToastOptions are very
> > similar to each other. So common part of creating these catalogs were
> > moved
> > to
> > add_autovacuum_options function that are called while creating each
> > catalog.
> > 
> > Now to the controversial part: in src/include/utils/rel.h had a lot of
> > Relation* macroses that used StdRdOptions structure. I've changed them
> > into
> > View* Heap* and Toast* analogues, and changed the code to use these
> > macroses instead of Relation* one. But this part of code I least sure of.
> > I'd like to ask experienced developers to double check it.
> > 
> > 8. I've moved all options-abstract code into options.c and options.h file,
> > and left in reloptions.c and reloptions.h only the code that concerns
> > relation options. Actually the main idea of this patch was to get this
> > abstract code in order to use it for custom attribute options later. All
> > the rest just a side effects :-)
> > 
> > So, before adding this to commitfest I want somebody to give a general
> > look
> > at it, if the code is OK.
> > 
> > Alvaro Herrera, you once said that you can review the patch...
> > 
> > The patch is available in the attachment.  It can be applied to current
> > master
> > 
> > You can also see latest version on my github
> > https://github.com/dhyannataraj/postgres/tree/reloption_fix
> > at the reloption_fix branch.
> > 
> > 
> > ATTRIBUTION NOTICE: I wrote this patch, when I was an  employee in
> > Postgres
> > Professional company. So this patch should be attributed as patch from
> > Postgres Pro (or something like that). For some reasons I did not manage
> > to
> > commit this patch while I left that job. But I got a permission to commit
> > it even if I left the company.
> > So my contribution to this patch as a independent developer was final code
> > cleanup, and writing some more comments. All the rest was a work of
> > Postgres Pro employee Nikolay Shaplov.

-- 
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

В письме от 12 марта 2017 22:29:30 пользователь Nikolay Shaplov написал:

Another rebase.

> I've rebased the patch again.
>
> Alvaro, I am waiting for you! ;-)
>
> > I've rebased the patch, so it can be applied to current master. See
> > attachment.
> >
> > Alvaro in private letter told that he will review the patch some time
> > later. So I am waiting.
> >
> > Also added this patch to commitfest:
> > https://commitfest.postgresql.org/13/992/
> >
> > > (Please see Attribution notice at the bottom of the letter)
> > >
> > > Hi! Some time ago I've send a proposal, about removing all am-related
> > > code
> > > from src/backend/access/common/reloptions.c and move it into somewhere
> > > inside am code. It was in
> > > https://www.postgresql.org/message-id/4636505.Bu9AKW1Kzc%40nataraj-amd64
> > > thread.
> > >
> > > Now I have a patch that is ready
> > >
> > > Before explaining what have been done, I need to define some concepts,
> > > so
> > > it would be easy to speak about them in the further explanation.
> > >
> > > Reloptions values in postgres actually exists in four representations:
> > > DefList*, TextArray[], Bytea and so called Values.
> > >
> > > - DefList representation is a list of DefElem, it is the representation
> > > in
> > > which reloptions comes from syntax analyzer, when relation is created or
> > > altered, and also this representation that is needed for pg_dump to dump
> > > a
> > > create statement that creates this relation.
> > >
> > > - TextArray[] representation is a way, how reloptions are stored in
> > > pg_catalog. It is TEXT[] attribute in the pg_catalog table and from the
> > > poinf o view of postgres code it is Datum array of TEXT Datums, when
> > > read
> > > into the memory
> > >
> > > -  Bytea (or I will also call it Binary) representation, is a
> > > representation with which relation code really works. It has C-structure
> > > inside in which fixed-length options are sored, some space is reserved
> > > for storing varlen values after the structure, and certainly it has
> > > BYTEA
> > > header in front of all of it. It is cached, it is fast to access. It is
> > > a
> > > good stuff.
> > >
> > > - Values (or in some cases RawValues) -- internal representation of
> > > option
> > > parser. Technically it is List of option_value structures (old name for
> > > it
> > > is relopt_value). This representation is used for parsing and validating
> > > options. Actually all the convertations between representations listed
> > > above are dove through Values representation. Values are called
> > > RawValues
> > > when Values List were already created, but it was not parsed yet, and
> > > all
> > > option values are in raw state: represented as a text.
> > >
> > >
> > > DefList and TextArray representations are converted in both ways(always
> > > through Values representation): we need DefList -> TextArray to create
> > > or
> > > change entry in the pg_catalog, and need TextArray -> DefList for
> > > pg_dump
> > >
> > > Bytea representation is converted from TextArray representation (also
> > > through Values representation). When relation code, tries to get cached
> > > Bytea representation for certain relation, and none is actually cached,
> > > then cache code calls extractRelOptions function that fetches entry from
> > > pg_catalog for that relation, gets reloptions  TextArray[], converts it
> > > into Bytea and cache it.
> > >
> > >
> > > Each reloption has it's own definition. This definitions tells how this
> > > option should be parsed, validated and converted. Before my patch this
> > > information were stored in relopt_gen and relopt_parse_elt structures.
> > > In
> > > my patch all this information were moved into option_definition_basic
> > > structure (that is actually old relopt_gen + relopt_parse_elt)
> > >
> > > The set of options definition I would call a options catalog. Before my
> > > patch there was one global options catalog (actually it was divided into
> > > parts by data types and has another part for dynamically added options,
> > > but
> > > nevertheless, it was global). After my patch there would be a separate
> > > options catalog for each relation type. For AM-relation this catalog
> > > would
> > > be available via AM-handler. for other relation types it would be still
> > > kept in reloption.c
> > >
> > >
> > >
> > > Now I am ready to tell what actually have been done in this patch:
> > >
> > > 1. I moved options definitions from global lists from reloptions.c
> > > [type name]RelOpts to lists that are kept inside AM code. One list per
> > > AM.
> > > heap options, toast options and options for all other relation types
> > > that
> > > are not accessible through  AccessMethod, all were kept inside
> > > reloptions.c, but now it is one list for each relation type, not a
> > > global
> > > one
> > >
> > > 2. Each AccessMethod had amoptions function that was responsible for
> > > converting reloptions from TextArray[] representation into Bytea
> > > representaions. This function used functions from reloptions.c and also
> > > had
> > > an array of relopt_parse_elt that defines how parsed reloption data
> > > should
> > > be packed inside Bytea chunk. I've moved data from relopt_parse_elt
> > > array
> > > into option definitions that are stored in the catalog (since catalog
> > > are
> > > now defined inside the AccessMethod we have access to a structure of
> > > Bytea
> > > representation at the place where we define option catalog)
> > >
> > > 3. Speaking of amoptions function, I've completely removed it, since we
> > > do
> > > not need it. For developers of Access Methods who still want to do some
> > > custom action with just parsed data, I've added postprocess_fun to
> > > option
> > > catalog. If this function is defined, it is called by convertation
> > > function
> > > right after Bytea representation were created. In postprocess_fun
> > > developer
> > > can do some custom validation, or change just parsed data. This feature
> > > is
> > > used in bloom index.
> > >
> > > 4. Instead of amoptions function I've added amrelopt_catalog function to
> > > the Access Method. This function returns option definition catalog for
> > > an
> > > Access Method. The catalog is used for processing options.
> > >
> > > 5. As before, relation cache calls extractRelOptions when it needs
> > > options
> > > that were not previously cached.
> > > Before my patch, it created Bytea representations for non-AM relations,
> > > and
> > > called amoptions AM function to get Bytea representation for AM
> > > relation.
> > > In by patch, it now gets option catalog, (using local functions for
> > > non-AM
> > > relations, and  using amrelopt_catalog function for AM-relations), and
> > > then
> > > use this catalog to convert options from TextArray into Bytea
> > > representation.
> > >
> > > 6. I've added some more featues:
> > >
> > > - Now you can set OPTION_DEFINITION_FLAG_FORBID_ALTER flag in definition
> > > of
> > > the option, and then postgres will refuse to change option value using
> > > ALTER command. In many indexes, some options are used only for index
> > > creation. After this it's value is saved in MetaPage, and used from
> > > there.
> > > Nevertheless user is allowed to change option value, though it affects
> > > nothing, one need to recreate index to really change such value. Now
> > > using
> > > this flag we can prevent user from getting an illusion that he
> > > successfully
> > > changed option value.
> > >
> > > - After applying my patch if you try to set toast. option for table that
> > > does not have toast, you will get an error. Before postgres will accept
> > > toast option for a table without a toast, but this value will be lost.
> > > This
> > > is bad behavior, so now postgres will throw an error in this case.
> > >
> > > - I've noticed that all string relation options in postgres are
> > > technically
> > > enum options. So I've added enum option type. This will save some bytes
> > > of
> > > the memory and some tacts of the CPU. I also left string option type
> > > (through it is not used now). A. Korotkov said that it might be needed
> > > later for storing patterns, xml/json paths or something like that.
> > >
> > > - Added some more tests that will trigger more options code. Though I am
> > > dreaming about more advanced test system, that will allow to check that
> > > certain value is received somewhere deep in the code.
> > >
> > > 7. And now the most controversial change. I've got rid of  StdRdOptions
> > > structure, in which options for heap, toast, nbtree, hash and spgist
> > > were
> > > stored in Bytea representation. In indexes only fillfactor value were
> > > actually used from the whole big structure. So I've created a separate
> > > structure for each relation type. HeapOptions and ToastOptions are very
> > > similar to each other. So common part of creating these catalogs were
> > > moved
> > > to
> > > add_autovacuum_options function that are called while creating each
> > > catalog.
> > >
> > > Now to the controversial part: in src/include/utils/rel.h had a lot of
> > > Relation* macroses that used StdRdOptions structure. I've changed them
> > > into
> > > View* Heap* and Toast* analogues, and changed the code to use these
> > > macroses instead of Relation* one. But this part of code I least sure
> > > of.
> > > I'd like to ask experienced developers to double check it.
> > >
> > > 8. I've moved all options-abstract code into options.c and options.h
> > > file,
> > > and left in reloptions.c and reloptions.h only the code that concerns
> > > relation options. Actually the main idea of this patch was to get this
> > > abstract code in order to use it for custom attribute options later. All
> > > the rest just a side effects :-)
> > >
> > > So, before adding this to commitfest I want somebody to give a general
> > > look
> > > at it, if the code is OK.
> > >
> > > Alvaro Herrera, you once said that you can review the patch...
> > >
> > > The patch is available in the attachment.  It can be applied to current
> > > master
> > >
> > > You can also see latest version on my github
> > > https://github.com/dhyannataraj/postgres/tree/reloption_fix
> > > at the reloption_fix branch.
> > >
> > >
> > > ATTRIBUTION NOTICE: I wrote this patch, when I was an  employee in
> > > Postgres
> > > Professional company. So this patch should be attributed as patch from
> > > Postgres Pro (or something like that). For some reasons I did not manage
> > > to
> > > commit this patch while I left that job. But I got a permission to
> > > commit
> > > it even if I left the company.
> > > So my contribution to this patch as a independent developer was final
> > > code
> > > cleanup, and writing some more comments. All the rest was a work of
> > > Postgres Pro employee Nikolay Shaplov.

--
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I gave this patch a quick skim.  At first I was confused by the term
"catalog"; I thought it meant we stored options in a system table.  But
that's not what is meant at all; instead, what we do is build these
"catalogs" in memory.  Maybe a different term could have been used, but
I'm not sure it's stricly necessary.  I wouldn't really bother.

I'm confused by the "no ALTER, no lock" rule.  Does it mean that if
"ALTER..SET" is forbidden?  Because I noticed that brin's
pages_per_range is marked as such, but we do allow that option to change
with ALTER..SET, so there's at least one bug there, and I would be
surprised if there aren't any others.

Please make sure to mark functions as static (e.g. bringetreloptcatalog).

Why not pass ->struct_size and ->postprocess_fun (which I'd rename it as
->postprocess_fn, more in line with our naming style) as a parameter to
allocateOptionsCatalog?  Also, to avoid repalloc() in most cases (and to
avoid pallocing more options that you're going to need in a bunch of
cases, perhaps that function should the number of times you expect to
call AddItems for that catalog (since you do it immediately afterwards
in all cases I can see), and allocate that number.  If later another
item arrives, then repalloc using the same code you already have in
AddItems().

Something is wrong with leading whitespace in many places; either you
added too many tabs, or the wrong number spaces; not sure which but
visually it's clearly wrong.  ... Actually there are whitespace-style
violations in several places; please fix using pgindent (after adding
any new typedefs your defining to typedefs.list).

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

By the way, it would be very good if you could review some patches, too.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Copying Fabrízio Mello here, who spent some time trying to work on
reloptions too.  He may have something to say about the new
functionality that this patch provides.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

В письме от 17 марта 2017 14:21:26 пользователь Alvaro Herrera написал:
> I gave this patch a quick skim.
Thanks!

> At first I was confused by the term
> "catalog"; I thought it meant we stored options in a system table.  But
> that's not what is meant at all; instead, what we do is build these
> "catalogs" in memory.  Maybe a different term could have been used, but
> I'm not sure it's stricly necessary.  I wouldn't really bother.
Yes "catalog" is quite confusing. I did not find better name while I was
writing the code, so I take first one, that came into my mind.
If you, or somebody else, have better idea how to call this sets of options
definitions I will gladly rename it, as catalog is a bad name. May be
OptionsDefSet instead of OptionsCatalog?


> I'm confused by the "no ALTER, no lock" rule.  Does it mean that if
> "ALTER..SET" is forbidden?  Because I noticed that brin's
> pages_per_range is marked as such, but we do allow that option to change
> with ALTER..SET, so there's at least one bug there, and I would be
> surprised if there aren't any others.

If you grep, for example, gist index code for "buffering_mode" option, you will
see, that this option is used only in gistbuild.c. There it is written into
the meta page, and afterwards, value from meta page is used, and one from
options, is just ignored.
Nowdays you can successfully alter this value, but this will not affect
anything until index is recreated... I thought it is very confusing behavior
and decided that we should just forbid such alters.


> Please make sure to mark functions as static (e.g. bringetreloptcatalog).
Ok. Will do.

> Why not pass ->struct_size and ->postprocess_fun (which I'd rename it as
> ->postprocess_fn, more in line with our naming style) as a parameter to
> allocateOptionsCatalog?
struct_size -- very good idea!
postprocess_fn -- now it is rarely used. In most cases it is NULL. May be it
would be ok to set it afterwards in that rare cases when it is needed.

> Also, to avoid repalloc() in most cases (and to
> avoid pallocing more options that you're going to need in a bunch of
> cases, perhaps that function should the number of times you expect to
> call AddItems for that catalog (since you do it immediately afterwards
> in all cases I can see), and allocate that number.  If later another
> item arrives, then repalloc using the same code you already have in
> AddItems().
I've copied this code from reloptions code for custom options. Without much
thinking about it.

If I would think about it now: we always know how many options we will have.
So we can just pass this number to palloc and assert if somebody adds more
options then expected... What do yo think about it.


> Something is wrong with leading whitespace in many places; either you
> added too many tabs, or the wrong number spaces; not sure which but
> visually it's clearly wrong.  ... Actually there are whitespace-style
> violations in several places; please fix using pgindent (after adding
> any new typedefs your defining to typedefs.list).
I will run pgindent on my code.



--
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.

Nikolay Shaplov wrote:

> If I would think about it now: we always know how many options we will have. 
> So we can just pass this number to palloc and assert if somebody adds more 
> options then expected... What do yo think about it.

I think we need to preserve the ability to add custom options, because
extensions may want to do that.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

В письме от 23 марта 2017 16:14:58 пользователь Fabrízio de Royes Mello
написал:
> On Thu, Mar 23, 2017 at 3:58 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
>
> wrote:
> > Copying Fabrízio Mello here, who spent some time trying to work on
> > reloptions too.  He may have something to say about the new
> > functionality that this patch provides.
>
> Thanks Álvaro, I'll look the patch and try to help in some way.
Thank you, that would be great!

--
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.

В письме от 26 марта 2017 15:02:12 Вы написали:
> Nikolay Shaplov wrote:
> > If I would think about it now: we always know how many options we will
> > have. So we can just pass this number to palloc and assert if somebody
> > adds more options then expected... What do yo think about it.
>
> I think we need to preserve the ability to add custom options, because
> extensions may want to do that.
Ok. At least this will not do any harm :-)

--
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.

В письме от 17 марта 2017 14:21:26 пользователь Alvaro Herrera написал:

> Please make sure to mark functions as static (e.g. bringetreloptcatalog).
I am a bit confused here:

For brin and nbtree this is a good idea: brin.c and nbtree.c has AM-handler
inside it, and  there are other static functions there. Adding one more static
function here seems to be quite logical.

For gin, gist and spgist, authors seems to use [index_name]_private.h files to
hide internal functions from outside code. In ginutil.c and spgutils.c, where
AM-handler is located, there is no static functions at all...  gist.c has, but
I think I should write similar code for all *_private.h indexes.

So I think it wold be good to hide catalog function via *_pricate.h include
file for these three indexes.

hash.c is quite a mess...
There is no hash_private.h, AM-handles is located in hash.c, that has "This
file contains only the public interface routines." comment at the beginning,
and there is no static functions inside. I do not know what is the right way
to hide hashgetreloptcatalog function here...

What would you advise?


--
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.

В письме от 26 марта 2017 15:02:12 пользователь Alvaro Herrera написал:
> Nikolay Shaplov wrote:
> > If I would think about it now: we always know how many options we will
> > have. So we can just pass this number to palloc and assert if somebody
> > adds more options then expected... What do yo think about it.
>
> I think we need to preserve the ability to add custom options, because
> extensions may want to do that.

I've been thinking about it for a while... I think this might lead to reducing
the quality of the code...

For example: There was 10 options for some relation type. I decided to add one
more option, but i did not ++ the number of options for
allocateOptionsCatalog. So space for 10 options were allocated, and then when
11th option is added, optionCatalogAddItem would allocate space for ten more
options, and nine of them would not be used. And nobody will notice it.

So, I see here four solutions:

0. Leave it as it was. (We both do not like it)
1. Forbid dynamic number of options (you do not like it)
2. Allow dynamic number of options only for special cases, and in all other
cases make them strict, and asserting if option number is wrong. This can be
done in two ways:
2a. Add strict boolean flag, that tells if allow to add more options or not
2b. Do not add any flags, but if number of items is specified, then process
number of items in strict mode. If it is set to -1, then do as it is done now,
totally dynamically.

I would prefer 2b, if you sure that somebody will need dynamic number of
options.

PS. I hardly believe that there can be dynamic number of options, as this
options later are mapped into C-structure that is quite static. No use case
comes into my mind, where I would like to have dynamic number of options, not
knowing at build time, how many of them there would be.



--
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.

Hi!

Here is a new version of the patch.

I've run pgindent on all my code.

I've declared static all functions I can.
 
I've moved size of Bytea to the arguments of allocateOptionsCatalog, and I 
also pass expected number of catalog items there. If it is positive, it will 
be treated as strict number of items. If it is -1, then new items will be 
dynamically reparroced when needed.

-- 
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.

Hi Nikolay,

On 2017-03-29 15:51:43 +0300, Nikolay Shaplov wrote:
> Here is a new version of the patch.

Thanks for the new version of the patch.  As this commitfest is running
out of time, and as the patch has been submitted late in the v10
development cycle, I've moved this patch to the next commitfest.

Regards,

Andres

В письме от 3 апреля 2017 11:31:28 пользователь Andres Freund написал:

> > Here is a new version of the patch.
>
> Thanks for the new version of the patch.  As this commitfest is running
> out of time, and as the patch has been submitted late in the v10
> development cycle, I've moved this patch to the next commitfest.
Alas...

But I am quite patient, so here is the same patch reabesed on current master,
And it also includes all changes that have been done to reloptions since
previous one. (autosummarize for brin and no options for partitioned tables)


--
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

В письме от 19 апреля 2017 21:31:42 пользователь Nikolay Shaplov написал:

I rebased the patch again, after code reindent.

Still waiting for review.

--
Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)

В письме от 25 июня 2017 21:05:49 пользователь Nikolay Shaplov написал:

I've just made sure that patch is still applyable to the current master.

And I am still waiting for reviews :-)


--
Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)

Nikolay Shaplov wrote:
> В письме от 25 июня 2017 21:05:49 пользователь Nikolay Shaplov написал:
> 
> I've just made sure that patch is still applyable to the current master.
> 
> And I am still waiting for reviews :-)

I see that this patch adds a few tests for reloptions, for instance in
bloom.  I think we should split this in at least two commits, one that
adds those tests before the functionality change, so that they can be
committed in advance, verify that the buildfarm likes it with the
current code, and verify the coverage.

I also noticed that your patch adds an error message that didn't exist
previously, 

+               ereport(ERROR,
+                       (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                      errmsg("col%i should not be grater than length", i)));

this seems a bit troublesome, because of the "col%i" thing and also
because of the typo.  I wonder if this means you've changed the way
sanity checks work here.

The new checks around toast table creation look like they belong to a
completely independent patch also ... the error message there goes
against message style guidelines also.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал:

> > I've just made sure that patch is still applyable to the current master.
> >
> > And I am still waiting for reviews :-)
>
> I see that this patch adds a few tests for reloptions, for instance in
> bloom.  I think we should split this in at least two commits, one that
> adds those tests before the functionality change, so that they can be
> committed in advance, verify that the buildfarm likes it with the
> current code, and verify the coverage.
This sounds as a good idea. I created such patch and started a new thread for
it https://www.postgresql.org/message-id/2615372.orqtEn8VGB%40x200m (I will
add that thread to commitfest as soon as I understand what test should be left
there)

> I also noticed that your patch adds an error message that didn't exist
> previously,
>
> +               ereport(ERROR,
> +                       (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +                      errmsg("col%i should not be grater than length",
> i)));
>
> this seems a bit troublesome, because of the "col%i" thing
What the problem with col%i ?

> and also because of the typo.
Oh... I always had bad marks for all natural languages in school :-( ;-)

> I wonder if this means you've changed the way sanity checks work here.
This should not be like this (I hope). I will attentively look at this part of
code again, and explain what exactly I've done. (I did it a year ago and now
need some time to recall)

> The new checks around toast table creation look like they belong to a
> completely independent patch also ...
This sounds reasonable too. Will do it, it is possible, as far as I remember.

> the error message there goes against message style guidelines also.
Oh... these style guidelines... will pay attention to it too...

--
Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)

В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал:

Yet another git rebase. This patch can be applied to master branch again.

For this patch no review needed now, as I will offer part of it (one that
refuses to set toast reloptions when there is no TOAST table) as a separate
patch.

--
Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Oct 30, 2017 at 9:49 AM, Nikolay Shaplov <dhyan@nataraj.su> wrote:
> В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал:
>
> Yet another git rebase. This patch can be applied to master branch again.
>
> For this patch no review needed now, as I will offer part of it (one that
> refuses to set toast reloptions when there is no TOAST table) as a separate
> patch.

This patch needs a rebase, there are conflicts with HEAD.

-\set VERBOSITY terseCREATE INDEX bloomidx2 ON tst USING bloom (i, t) WITH (length=0);ERROR:  value 0 out of bounds for
option"length" 
+DETAIL:  Valid values are between "1" and "4096".
+CREATE INDEX bloomidx2 ON tst USING bloom (i, t) WITH (length=4097);
+ERROR:  value 4097 out of bounds for option "length"
+DETAIL:  Valid values are between "1" and "4096".
The basic set of relopt tests have been committed 4b95cc1. Why
insisting on them?

+   if (validate)
+       for (i = 0; i < INDEX_MAX_KEYS; i++)
+       {
The lack of brackets here is not project-like. You should use some for clarity.

There is still no API equivalent for add_int_reloption(),
add_real_reloption(), which is a problem as extension should be
allowed to still use that. Except if I am missing something you could
just have a compatibility API as for example optionCatalogAddItem()
and add_reloption() share really a lot.
   }
-   return lockmode;
[...]#include "utils/typcache.h"
-
+#include "utils/memutils.h"
+#include "utils/guc.h"
Much noise diffs.
LOCKMODE
-AlterTableGetRelOptionsLockLevel(List *defList)
+AlterTableGetRelOptionsLockLevel(Relation rel, List *defList){
This could just be static within tablecmds.c.

-#define RelationGetTargetPageFreeSpace(relation, defaultff) \
-   (BLCKSZ * (100 - RelationGetFillFactor(relation, defaultff)) / 100)
+#define HeapGetTargetPageFreeSpace(relation, defaultff) \
+   (BLCKSZ * (100 - HeapGetFillFactor(relation, defaultff)) / 100)
-1 for this kind of renames. OK, fillfactor cannot be set for toast
tables but that's not a reason to break current APIs and give more
noise to extension authors.

+/* optionsDefListToRawValues
+ *     Converts option values that came from syntax analyzer (DefList) into
+ *     Values List.
Should be careful about comment block format.

+       case RELKIND_PARTITIONED_TABLE:
+           catalog = NULL; /* No options for parted table for now */
+           break;
s/parted/partitioned/.

+   amrelopt_catalog_function amrelopt_catalog; /* can be NULL */
Or amoptionlist? Catalog is confusing and refers to system catalogs for example.

-   int         bufferingModeOffset;    /* use buffering build? */
+   int         buffering_mode; /* use buffering build? */
I would suggest avoiding unnecessary renames to keep the patch simple.
--
Michael

On Tue, Nov 14, 2017 at 4:44 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Oct 30, 2017 at 9:49 AM, Nikolay Shaplov <dhyan@nataraj.su> wrote:
>> В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал:
>>
>> Yet another git rebase. This patch can be applied to master branch again.
>>
>> For this patch no review needed now, as I will offer part of it (one that
>> refuses to set toast reloptions when there is no TOAST table) as a separate
>> patch.
>
> This patch needs a rebase, there are conflicts with HEAD.

Bumping the patch to next CF as no updates have happened for the last two weeks.

> There is still no API equivalent for add_int_reloption(),
> add_real_reloption(), which is a problem as extension should be
> allowed to still use that. Except if I am missing something you could
> just have a compatibility API as for example optionCatalogAddItem()
> and add_reloption() share really a lot.

This worries me.
--
Michael

On Tue, Nov 28, 2017 at 11:22 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Nov 14, 2017 at 4:44 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Mon, Oct 30, 2017 at 9:49 AM, Nikolay Shaplov <dhyan@nataraj.su> wrote:
>>> В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал:
>>>
>>> Yet another git rebase. This patch can be applied to master branch again.
>>>
>>> For this patch no review needed now, as I will offer part of it (one that
>>> refuses to set toast reloptions when there is no TOAST table) as a separate
>>> patch.
>>
>> This patch needs a rebase, there are conflicts with HEAD.
>
> Bumping the patch to next CF as no updates have happened for the last two weeks.

Er, returned with feedback.. This will be my last patch for the day.
--
Michael

В письме от 28 ноября 2017 11:23:38 пользователь Michael Paquier написал:
> On Tue, Nov 28, 2017 at 11:22 AM, Michael Paquier
>
> <michael.paquier@gmail.com> wrote:
> > On Tue, Nov 14, 2017 at 4:44 PM, Michael Paquier
> >
> > <michael.paquier@gmail.com> wrote:
> >> On Mon, Oct 30, 2017 at 9:49 AM, Nikolay Shaplov <dhyan@nataraj.su>
wrote:
> >>> В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera
> >>> написал:
> >>>
> >>> Yet another git rebase. This patch can be applied to master branch
> >>> again.
> >>>
> >>> For this patch no review needed now, as I will offer part of it (one
> >>> that
> >>> refuses to set toast reloptions when there is no TOAST table) as a
> >>> separate
> >>> patch.
> >>
> >> This patch needs a rebase, there are conflicts with HEAD.
> >
> > Bumping the patch to next CF as no updates have happened for the last two
> > weeks.
> Er, returned with feedback.. This will be my last patch for the day.
May be you are right. This patch should be split into smaller patches. I will
do it bit by bit whet I have time. But I see no reason for the big one to
remain on commitfest, as it is not for direct commit.

PS. I will do the rebase soon, this patch overlaps with two recent commits. So
this require very careful rebasing....


--
Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)

В письме от 28 ноября 2017 11:23:38 пользователь Michael Paquier написал:

> >> This patch needs a rebase, there are conflicts with HEAD.
> >
> > Bumping the patch to next CF as no updates have happened for the last two
> > weeks.
> Er, returned with feedback.. This will be my last patch for the day.
Just to have it up to date. This is a rebased version of the patch.

Will cut chunks from it and add them to commitfest, soon.

--
Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)

В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал:

> The new checks around toast table creation look like they belong to a
> completely independent patch also ... the error message there goes
> against message style guidelines also.

Offered toast relation checks as a separate patch:
https://www.postgresql.org/message-id/3413542.LWpeoxCNq5@x200m


--
Do code for fun.

В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал:

> I think we should split this in at least two commits,
Added another part for commit:
https://www.postgresql.org/message-id/43332102.S2V5pIjXRx@x200m
This one adds an enum relation option type.

--
Do code for fun.

В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал:
> I think we should split this in at least two commits,
I've added a third part of this patch to commitfest:
https://www.postgresql.org/message-id/flat/2083183.Rn7qOxG4Ov@x200m#2083183.Rn7qOxG4Ov@x200m

To finally commit the rest of the path (the main part of it at least) I need
this, and enum-options patches to be commited. Hope it will be done in march
and I will offer the last part in next commitfest.

--
Do code for fun.