[PATCH] New [relation] option engine
Hi!
I'd like to introduce a patch that reworks options processing.
This patch detaches code for options processing and validating from the code
that uses these options. This will allow to reuse same code in any part of
postgres that may need options. Currently there are three ways of defining
options, and this code is scattered along the source tree, that leads to a
problems: code is hard to understand, hard to maintain, I saw several options
added or changed, and and in one of two cases, there was one or another
mistake done because of code unclearity.
General idea:
There is an object OptionSpec that describes how single option should be
parsed, validated, stored into bytea, etc.
OptionSpecSet is a list of OptionSpecs, that describes all possible options
for certain object (e.g. options for nbtree index relation)
All options related functions that are not depended on context were moved to
src/backend/access/common/options.c. These functions receives OptionSpecSet
(or OptionSpec) and option data that should be processed. And OptionSpecSet
should contain all information that is needed for proper processing.
Options from the perspective of the option engine can exist in four
representation:
- defList - the way they came from SQL parser
- TEXT[] - the way they are stored in pg_class or similar place
- Bytea - the way they stored in C-structure, for caching and using in
postgres code that uses these options
- Value List - is an internal representation that is used while parsing,
validating and converting between representations
See code comments for more info.
There are functions that is used for conversion from one representation to
another:
- optionsDefListToRawValues : defList -> Values
- optionsTextArrayToDefList :TEXT[] -> defList
- optionsTextArrayToRawValues : TEXT[] -> Values
- optionsValuesToTextArray: Values -> TEXT[]
- optionsValuesToBytea: Values -> Bytea
This functions are called from meta-functions that is used when postgres
receives an SQL command for creating or updating options:
- optionsDefListToTextArray - when options are created
- optionsUpdateTexArrayWithDefList - when option are updated.
They also trigger validation while processing.
There are also functions for SpecSet processing:
- allocateOptionsSpecSet
- optionsSpecSetAddBool
- optionsSpecSetAddBool
- optionsSpecSetAddReal
- optionsSpecSetAddEnum
- optionsSpecSetAddString
For index access methods "amoptions" member function that preformed option
processing, were replaced with "amreloptspecset" member function that provided
an SpecSet for reloptions for this AM, so caller can trigger option processing
himself.
For other relation types options have been processing by "fixed" functions, so
these processing were replaced with "fixed" SpecSet providers + processing
using that SpecSet. Later on these should be moved to access method the same
way it is done in indexes. I plan to do it after this patch is commit.
As for local options, that is used of opclass options, I kept all current API,
but made this API a wrapper around new option engine. Local options should be
moved to unified option engine API later on. I hope to do it too.
This patch does not change any of postgres behaviour (even if this behaviour
is not quite right). The only change is text of the warning for unexisting
option in toast namespace. But I hope this change is for better.
The only problem I am not sure how to solve is an obtaining a LockMode.
To get a LockMode for option , you need a SpecSet. For indexes we get SpecSets
via AccessMethod. To get access for AccessMethod, you need relation C-
structure. To get relation structure you need open relation with NoLock mode.
But if I do it, I get Assert in relation_open. (There were no such Assert when
I started this work, it appeared later).
All code related to this issue is marked with FIXME comments. I've commented
that Assert out, but not quite sure I was right. Here I need a help of more
experienced members of community.
This quite a big patch. Unfortunately it can't be split into smaller parts.
I also suggest to consider this patch as an implementation of general idea,
that works well. I have ideas in mind to make it better, but it will be
infinite improvement process that will never lead to final commit. So if the
concept is good and implementation is good enough, I suggest to commit it, and
make it better later on by smaller patches over it. If it is not good enough,
let me know, I will try to make it good.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v01.difftext/x-patch; charset=UTF-8; name=new_options_take_two_v01.diffDownload+2624-2359
Hi,
On 2022-02-14 00:43:36 +0300, Nikolay Shaplov wrote:
I'd like to introduce a patch that reworks options processing.
This doesn't apply anymore: http://cfbot.cputube.org/patch_37_3536.log
Given that this patch has been submitted just to the last CF and that there's
been no action on it, I don't see this going into 15. Therefore I'd like to
move it to the next CF?
Greetings,
Andres Freund
В письме от вторник, 22 марта 2022 г. 03:46:10 MSK пользователь Andres Freund
написал:
I'd like to introduce a patch that reworks options processing.
This doesn't apply anymore: http://cfbot.cputube.org/patch_37_3536.log
Thank you for sending this notice!
I've updated the patch to include new changes, that have been made in postgres
recently.
Given that this patch has been submitted just to the last CF and that
there's been no action on it, I don't see this going into 15.
Yes it is not to be added in 15 for sure.
Therefore I'd like to move it to the next CF?
The only reason it can be kept in current CF, is to keep it in front of the
eyes of potential reviewers for one more week. So it is up to you, if you
consider it does not worth it, you can move it now. No great harm would be
done then.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v01a.difftext/x-patch; charset=UTF-8; name=new_options_take_two_v01a.diffDownload+2629-2370
A minor fix to get rid of maybe-uninitialized warnings.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v01b.difftext/x-patch; charset=UTF-8; name=new_options_take_two_v01b.diffDownload+2631-2371
В письме от воскресенье, 27 марта 2022 г. 15:19:41 MSK пользователь Nikolay
Shaplov написал:
A minor fix to get rid of maybe-uninitialized warnings.
Same patch, but properly rebased to current master head.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v01c.difftext/x-patch; charset=utf-8; name=new_options_take_two_v01c.diffDownload+2631-2368
В письме от суббота, 16 апреля 2022 г. 17:15:34 MSK пользователь Nikolay
Shaplov написал:
Changed NoLock to AccessShareLock. This will remove last FIXME from the patch.
As more experienced pg-developers told me, you can call relation_open with
AccessShareLock, on a table you are going to work with, any time you like,
without being ashamed about it.
But this lock is visible when you have prepared transaction, so have to update
twophase test of test_decoding extension.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v02.difftext/x-patch; charset=UTF-8; name=new_options_take_two_v02.diffDownload+2626-2366
В письме от среда, 4 мая 2022 г. 18:15:51 MSK пользователь Nikolay Shaplov
написал:
Rebased patch, so it can be applied to current master again.
Added static keyword to enum option items definition as it have been done in
09cd33f4
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v02a.difftext/x-patch; charset=UTF-8; name=new_options_take_two_v02a.diffDownload+2626-2366
I'm sorry if you've already said this elsewhere, but can you please
state what is the *intention* of this patchset? If it's a pure
refactoring (but I don't think it is), then it's a net loss, because
after pgindent it summarizes as:
58 files changed, 2714 insertions(+), 2368 deletions(-)
so we end up 500+ lines worse than the initial story. However, I
suspect that's not the final situation, since I saw a comment somewhere
that opclass options have to be rewritten to modify this mechanism, and
I suspect that will remove a few lines. And you maybe have a more
ambitious goal. But what is it?
Please pgindent your code for the next submission, making sure to add
your new typedef(s) to typedefs.list so that it doesn't generate stupid
spaces. After pgindenting you'll notice the argument lists of some
functions look bad (cf. commit c4f113e8fef9). Please fix that too.
I notice that you kept the commentary about lock levels in the place
where they were previously defined. This is not good. Please move each
explanation next to the place where each option is defined.
For next time, please use "git format-patch" for submission, and write a
tentative commit message. The committer may or may not use your
proposed commit message, but with it they will know what you're trying
to achieve.
The translatability marker for detailmsg for enums is wrong AFAICT. You
need gettext_noop() around the strings themselves IIRC. I think you
need to get rid of the _() call around the variable that receives that
value and use errdetail() instead of errdetail_internal(), to avoid
double-translating it; but I'm not 100% sure. Please experiment with
"make update-po" until you get the messages in the .po file.
You don't need braces around single-statement blocks.
Thanks
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"World domination is proceeding according to plan" (Andrew Morton)
В письме от воскресенье, 15 мая 2022 г. 15:25:47 MSK пользователь Alvaro
Herrera написал:
I'm sorry if you've already said this elsewhere, but can you please
state what is the *intention* of this patchset? If it's a pure
refactoring (but I don't think it is), then it's a net loss, because
after pgindent it summarizes as:58 files changed, 2714 insertions(+), 2368 deletions(-)
so we end up 500+ lines worse than the initial story. However, I
suspect that's not the final situation, since I saw a comment somewhere
that opclass options have to be rewritten to modify this mechanism, and
I suspect that will remove a few lines. And you maybe have a more
ambitious goal. But what is it?
My initial goal was to make options code reusable for any types of options
(not only reloptions). While working with this goal I came to conclusion that
I have to create completely new option engine that will be used anywhere
options (name=value) is needed. This will solve following problems:
- provide unified API for options definition. Currently postgres have core-AM
options, contrib-AM options and local options for opclasses, each have their
own way to define options. This patch will allow to use one API for them all
(for opclasses it is still WIP)
- Currently core-AM options are partly defined in reloptions.c and partly in AM
code. This is error prone. This patch fixes that.
- For indexes option definition is moved into AM code, where they should be.
For heap it should be moved into AM code later.
- There is no difference for core-AM indexes, and contrib-AM indexes options.
They use same API.
I also tried to write detailed commit message as you've suggested. There my
goals is described in more detailed way.
Please pgindent your code for the next submission, making sure to add
your new typedef(s) to typedefs.list so that it doesn't generate stupid
spaces. After pgindenting you'll notice the argument lists of some
functions look bad (cf. commit c4f113e8fef9). Please fix that too.
I've tried to pgindent. Hope I did it well. I've manually edited all code
lines (not string consts) that were longer then 80 characters, afterwards.
Hope it was right decision
I notice that you kept the commentary about lock levels in the place
where they were previously defined. This is not good. Please move each
explanation next to the place where each option is defined.
You are right. Tried to find better place for it.
I also noticed that I've missed updating initial comment for reloptions.c.
Will update it this week, meanwhile will send a patch version without changing
that comment, in order not to slow anything down.
For next time, please use "git format-patch" for submission, and write a
tentative commit message. The committer may or may not use your
proposed commit message, but with it they will know what you're trying
to achieve.
Done.
The translatability marker for detailmsg for enums is wrong AFAICT. You
need gettext_noop() around the strings themselves IIRC. I think you
need to get rid of the _() call around the variable that receives that
value and use errdetail() instead of errdetail_internal(), to avoid
double-translating it; but I'm not 100% sure. Please experiment with
"make update-po" until you get the messages in the .po file.
That part of code was not written by me. It was added while enum options were
commit. Then I've just copied it to this patch. I do not quite understand how
does it works. But I can say that update-po works well for enum detailmsg, and
we actually have gettext_noop(), but it is used while calling
optionsSpecSetAddEnum, not when error message is actually printed. But I guess
it do the trick.
You don't need braces around single-statement blocks.
Tried to remove all I've found.
Thanks
Thank you for answering.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v03.patchtext/x-patch; charset=UTF-8; name=new_options_take_two_v03.patchDownload+2637-2395
forbid_realloc is only tested in an assert. There needs to be an "if"
test for it somewhere (suppose some extension author uses this API and
only runs it in assert-disabled environment; they'll never know they
made a mistake). But do we really need this option? Why do we need a
hardcoded limit in the number of options?
In allocateOptionsSpecSet there's a new error message with a typo
"grater" which should be "greater". But I think the message is
confusingly worded. Maybe a better wording is "the value of parameter
XXX may not be greater than YYY".
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
В письме от среда, 18 мая 2022 г. 11:10:08 MSK пользователь Alvaro Herrera
написал:
forbid_realloc is only tested in an assert. There needs to be an "if"
test for it somewhere (suppose some extension author uses this API and
only runs it in assert-disabled environment; they'll never know they
made a mistake). But do we really need this option? Why do we need a
hardcoded limit in the number of options?
General idea was that it is better to allocate as many option_spec items as we
are going to use. It is optional, so if you do not want to allocate exact
number of options, then realloc will be used, when we adding one more item,
and all allocated items are used.
But when you explicitly specify number of items, it is better not to forget to
++ it when you add extra option in the code. That was the purpose of
forbid_realloc: to remind. It worked well for, while working with the patch
several options were added in the upstream, and this assert reminded me that I
should also allocate extra item.
If it is run in production without asserts, it is also no big deal, we will
just have another realloc.
But you are right, variable name forbid_realloc is misleading. It does not
really forbid anything, so I changed it to assert_on_realloc, so the name
tells what this flag really do.
In allocateOptionsSpecSet there's a new error message with a typo
"grater" which should be "greater". But I think the message is
confusingly worded. Maybe a better wording is "the value of parameter
XXX may not be greater than YYY".
This error message is really from bloom index. And here I was not as careful
and watchful as I should, because this error message is from the check that
was not there in original code. And this patch should not change behaviour at
all. So I removed this check completely, and will submit it later.
My original patch has a bunch of changes like that. I've removed them all, but
missed one in the contrib... :-(
Thank you for pointing to it.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v03a.patchtext/x-patch; charset=UTF-8; name=new_options_take_two_v03a.patchDownload+2622-2395
On Mon, 2022-02-14 at 00:43 +0300, Nikolay Shaplov wrote:
For index access methods "amoptions" member function that preformed
option
processing, were replaced with "amreloptspecset" member function that
provided
an SpecSet for reloptions for this AM, so caller can trigger option
processing
himself.
What about table access methods? There have been a couple attempts to
allow custom reloptions for table AMs. Does this patch help that use
case?
Regards,
Jeff Davis
В письме от понедельник, 11 июля 2022 г. 23:03:55 MSK пользователь Jeff Davis
написал:
For index access methods "amoptions" member function that preformed
option
processing, were replaced with "amreloptspecset" member function that
provided
an SpecSet for reloptions for this AM, so caller can trigger option
processing
himself.What about table access methods? There have been a couple attempts to
allow custom reloptions for table AMs. Does this patch help that use
case?
This patch does not add custom reloptions for table AM. It is already huge
enough, with no extra functionality. But new option engine will make adding
custom options for table AM more easy task, as main goal of this patch is to
simplify adding options everywhere they needed. And yes, adding custom table
AM options is one of my next goals, as soon as this patch is commit.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
В письме от вторник, 12 июля 2022 г. 07:30:40 MSK пользователь Nikolay Shaplov
написал:
What about table access methods? There have been a couple attempts to
allow custom reloptions for table AMs. Does this patch help that use
case?This patch does not add custom reloptions for table AM. It is already huge
enough, with no extra functionality. But new option engine will make adding
custom options for table AM more easy task, as main goal of this patch is to
simplify adding options everywhere they needed. And yes, adding custom
table AM options is one of my next goals, as soon as this patch is commit.
And here goes rebased version of the patch, that can be applied to current
master.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v03b.patchtext/x-patch; charset=UTF-8; name=new_options_take_two_v03b.patchDownload+2619-2390
2022年7月12日(火) 13:47 Nikolay Shaplov <dhyan@nataraj.su>:
В письме от вторник, 12 июля 2022 г. 07:30:40 MSK пользователь Nikolay Shaplov
написал:What about table access methods? There have been a couple attempts to
allow custom reloptions for table AMs. Does this patch help that use
case?This patch does not add custom reloptions for table AM. It is already huge
enough, with no extra functionality. But new option engine will make adding
custom options for table AM more easy task, as main goal of this patch is to
simplify adding options everywhere they needed. And yes, adding custom
table AM options is one of my next goals, as soon as this patch is commit.And here goes rebased version of the patch, that can be applied to current
master.
Hi
cfbot reports the patch no longer applies. As CommitFest 2022-11 is
currently underway, this would be an excellent time to update the patch.
Thanks
Ian Barwick
В письме от пятница, 4 ноября 2022 г. 03:47:09 MSK пользователь Ian Lawrence
Barwick написал:
cfbot reports the patch no longer applies. As CommitFest 2022-11 is
currently underway, this would be an excellent time to update the patch.
Oups! I should have done it before...
Fixed
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v03c.patchtext/x-patch; charset=UTF-8; name=new_options_take_two_v03c.patchDownload+2620-2390
В письме от пятница, 4 ноября 2022 г. 21:06:38 MSK пользователь Nikolay
Shaplov написал:
В письме от пятница, 4 ноября 2022 г. 03:47:09 MSK пользователь Ian Lawrence
Barwick написал:cfbot reports the patch no longer applies. As CommitFest 2022-11 is
currently underway, this would be an excellent time to update the patch.Oups! I should have done it before...
Fixed
Trying to fix meson build
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v03d.patchtext/x-patch; charset=UTF-8; name=new_options_take_two_v03d.patchDownload+2621-2390
В письме от пятница, 4 ноября 2022 г. 22:30:06 MSK пользователь Nikolay
Shaplov написал:
cfbot reports the patch no longer applies. As CommitFest 2022-11 is
currently underway, this would be an excellent time to update the patch.Oups! I should have done it before...
FixedTrying to fix meson build
Trying to fix compiler warnings.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v03e.patchtext/x-patch; charset=UTF-8; name=new_options_take_two_v03e.patchDownload+2621-2390
В письме от воскресенье, 6 ноября 2022 г. 19:22:09 MSK пользователь Nikolay
Shaplov написал:
cfbot reports the patch no longer applies. As CommitFest 2022-11 is
currently underway, this would be an excellent time to update the
patch.Oups! I should have done it before...
FixedTrying to fix meson build
Trying to fix compiler warnings.
Patched rebased. Imported changes from 4f981df8 commit (Report a more useful
error for reloptions on a partitioned table)
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
new_options_take_two_v03f.patchtext/x-patch; charset=UTF-8; name=new_options_take_two_v03f.patchDownload+2591-2337
On Sun, 20 Nov 2022 at 11:42, Nikolay Shaplov <dhyan@nataraj.su> wrote:
В письме от воскресенье, 6 ноября 2022 г. 19:22:09 MSK пользователь Nikolay
Shaplov написал:cfbot reports the patch no longer applies. As CommitFest 2022-11 is
currently underway, this would be an excellent time to update the
patch.Oups! I should have done it before...
FixedTrying to fix meson build
Trying to fix compiler warnings.
Patched rebased. Imported changes from 4f981df8 commit (Report a more useful
error for reloptions on a partitioned table)
The patch does not apply on top of HEAD as in [1]http://cfbot.cputube.org/patch_41_3536.log, please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
92957ed98c5c565362ce665266132a7f08f6b0c0 ===
=== applying patch ./new_options_take_two_v03f.patch
patching file src/include/access/reloptions.h
Hunk #1 FAILED at 1.
1 out of 4 hunks FAILED -- saving rejects to file
src/include/access/reloptions.h.rej
[1]: http://cfbot.cputube.org/patch_41_3536.log
Regards,
Vignesh
