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

В письме от 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
/messages/by-id/4636505.Bu9AKW1Kzc@nataraj-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.

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
/messages/by-id/4636505.Bu9AKW1Kzc@nataraj-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.

В письме от 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
/messages/by-id/4636505.Bu9AKW1Kzc@nataraj-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.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

Regards,

--
Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

В письме от 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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

В письме от 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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

В письме от 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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

В письме от 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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Nikolay Shaplov wrote:

В письме от 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.

I am just saying that if there is a function used only in one
compilation unit (.c file) then let's make sure its prototype says
"static".

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.

Sure.

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?

Leave it where it is (hashutil.c), no static marker. We may want to
move things around later, but that's not your patch's responsibility.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

В письме от 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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

В письме от 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.

В письме от 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)