Обсуждение: [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

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

A minor fix to get rid of maybe-uninitialized warnings. 

-- 
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

В письме от воскресенье, 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

В письме от суббота, 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

В письме от среда, 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

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

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

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

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

В письме от пятница, 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

В письме от пятница, 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...
> > Fixed
>
> Trying to fix meson build
Trying to fix compiler warnings.

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

В письме от воскресенье, 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...
> > > Fixed
> >
> > Trying 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

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...
> > > > Fixed
> > >
> > > Trying 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], 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

On Tue, 3 Jan 2023 at 18:38, vignesh C <vignesh21@gmail.com> wrote:
>
> 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...
> > > > > Fixed
> > > >
> > > > Trying 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], 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

There has been no updates on this thread for some time, so this has
been switched as Returned with Feedback. Feel free to open it in the
next commitfest if you plan to continue on this.

Regards,
Vignesh

On 2023-Jan-31, vignesh C wrote:

> On Tue, 3 Jan 2023 at 18:38, vignesh C <vignesh21@gmail.com> wrote:

> > The patch does not apply on top of HEAD as in [1], 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
> 
> There has been no updates on this thread for some time, so this has
> been switched as Returned with Feedback. Feel free to open it in the
> next commitfest if you plan to continue on this.

Well, no feedback has been given, so I'm not sure this is a great
outcome.  In the interest of keeping it alive, I've rebased it.  It
turns out that the only conflict is with the 2022 -> 2023 copyright line
update.

I have not reviewed it.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El que vive para el futuro es un iluso, y el que vive para el pasado,
un imbécil" (Luis Adler, "Los tripulantes de la noche")

В письме от среда, 1 февраля 2023 г. 12:04:26 MSK пользователь Alvaro Herrera
написал:
> On 2023-Jan-31, vignesh C wrote:
> > On Tue, 3 Jan 2023 at 18:38, vignesh C <vignesh21@gmail.com> wrote:
> > > The patch does not apply on top of HEAD as in [1], 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
> >
> > There has been no updates on this thread for some time, so this has
> > been switched as Returned with Feedback. Feel free to open it in the
> > next commitfest if you plan to continue on this.
>
> Well, no feedback has been given, so I'm not sure this is a great
> outcome.  In the interest of keeping it alive, I've rebased it.  It
> turns out that the only conflict is with the 2022 -> 2023 copyright line
> update.
>
> I have not reviewed it.
Guys sorry, I am totally unable to do just anything this month... Do not have
spare resources to switch my attention to anything even this simple...

Hope, next month will be better... And I will try to do some more reviewing of
other patches...

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

On Wed, 1 Feb 2023 at 14:49, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2023-Jan-31, vignesh C wrote:
>
> > On Tue, 3 Jan 2023 at 18:38, vignesh C <vignesh21@gmail.com> wrote:
>
> > > The patch does not apply on top of HEAD as in [1], 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
> >
> > There has been no updates on this thread for some time, so this has
> > been switched as Returned with Feedback. Feel free to open it in the
> > next commitfest if you plan to continue on this.
>
> Well, no feedback has been given, so I'm not sure this is a great
> outcome.  In the interest of keeping it alive, I've rebased it.  It
> turns out that the only conflict is with the 2022 -> 2023 copyright line
> update.
>
> I have not reviewed it.

Since there was no response to rebase the patch, I was not sure if the
author was planning to continue. Anyways Nikolay Shaplov plans to work
on this soon, So I have added this to the next commitfest at [1].
[1] - https://commitfest.postgresql.org/41/3536/

Regards,
Vignesh

On 2023-Feb-02, vignesh C wrote:

> On Wed, 1 Feb 2023 at 14:49, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > Well, no feedback has been given, so I'm not sure this is a great
> > outcome.  In the interest of keeping it alive, I've rebased it.  It
> > turns out that the only conflict is with the 2022 -> 2023 copyright line
> > update.
> 
> Since there was no response to rebase the patch, I was not sure if the
> author was planning to continue. Anyways Nikolay Shaplov plans to work
> on this soon, So I have added this to the next commitfest at [1].

Thank you!

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"¿Cómo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germán Poo)

On Thu, Feb 2, 2023 at 10:03 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, 1 Feb 2023 at 14:49, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > On 2023-Jan-31, vignesh C wrote:
> >
> > > On Tue, 3 Jan 2023 at 18:38, vignesh C <vignesh21@gmail.com> wrote:

> > > There has been no updates on this thread for some time, so this has
> > > been switched as Returned with Feedback. Feel free to open it in the
> > > next commitfest if you plan to continue on this.
> >
> > Well, no feedback has been given, so I'm not sure this is a great
> > outcome.  In the interest of keeping it alive, I've rebased it.  It
> > turns out that the only conflict is with the 2022 -> 2023 copyright line
> > update.
> >
> > I have not reviewed it.
>
> Since there was no response to rebase the patch, I was not sure if the
> author was planning to continue. Anyways Nikolay Shaplov plans to work
> on this soon, So I have added this to the next commitfest at [1].
> [1] - https://commitfest.postgresql.org/41/3536/

Nine months have passed, and there hasn't been an update since last
time it was returned. It doesn't seem to have been productive to
resurrect it to the CF based on "someone plans to work on it soon".
I'm re-returning it with feedback.

Some time ago, there was a proposal to create a new category "lack of
interest", but that also seems to have gone nowhere because of...lack
of interest.

--
John Naylor

11.11.2023 12:33, John Naylor пишет:
> On Thu, Feb 2, 2023 at 10:03 AM vignesh C <vignesh21@gmail.com> wrote:
>>
>> On Wed, 1 Feb 2023 at 14:49, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>>>
>>> On 2023-Jan-31, vignesh C wrote:
>>>
>>>> On Tue, 3 Jan 2023 at 18:38, vignesh C <vignesh21@gmail.com> wrote:
> 
>>>> There has been no updates on this thread for some time, so this has
>>>> been switched as Returned with Feedback. Feel free to open it in the
>>>> next commitfest if you plan to continue on this.
>>>
>>> Well, no feedback has been given, so I'm not sure this is a great
>>> outcome.  In the interest of keeping it alive, I've rebased it.  It
>>> turns out that the only conflict is with the 2022 -> 2023 copyright line
>>> update.
>>>
>>> I have not reviewed it.
>>
>> Since there was no response to rebase the patch, I was not sure if the
>> author was planning to continue. Anyways Nikolay Shaplov plans to work
>> on this soon, So I have added this to the next commitfest at [1].
>> [1] - https://commitfest.postgresql.org/41/3536/
> 
> Nine months have passed, and there hasn't been an update since last
> time it was returned. It doesn't seem to have been productive to
> resurrect it to the CF based on "someone plans to work on it soon".
> I'm re-returning it with feedback.
> 
> Some time ago, there was a proposal to create a new category "lack of
> interest", but that also seems to have gone nowhere because of...lack
> of interest.

I've rebased patch, so it could be add to commitfest again.

------
Yura Sokolov

On Fri, Dec 08, 2023 at 08:10:29AM +0300, Yura Sokolov wrote:
> I've rebased patch, so it could be add to commitfest again.

This is a 270kB patch with quite a few changes, and a lot of code
moved around:

>  47 files changed, 2592 insertions(+), 2326 deletions(-)

Could it be possible to split that into more successive steps to ease
its review?
--
Michael

В письме от пятница, 8 декабря 2023 г. 08:59:41 MSK пользователь Michael
Paquier написал:

> > I've rebased patch, so it could be add to commitfest again.
>
> This is a 270kB patch with quite a few changes, and a lot of code
>
> moved around:
> >  47 files changed, 2592 insertions(+), 2326 deletions(-)
>
> Could it be possible to split that into more successive steps to ease
> its review?

Theoretically I can create patch with full options.c as it is in the patch
now, and use that code only in index AM, and keep reloption.c mostly
unchanged.

This will be total mess with two different options mechanisms working in the
same time, but this might be much more easy to review.  When we are done with
the first step, we can change the rest.
If this will help to finally include patch into postgres, I can do it. Will
that help you to review?


--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

On 2023-Dec-08, Nikolay Shaplov wrote:

> Theoretically I can create patch with full options.c as it is in the patch 
> now, and use that code only in index AM, and keep reloption.c mostly 
> unchanged.
> 
> This will be total mess with two different options mechanisms working in the 
> same time, but this might be much more easy to review.  When we are done with 
> the first step, we can change the rest.
> If this will help to finally include patch into postgres, I can do it. Will 
> that help you to review?

I don't think that's better, because we could create slight
inconsistencies between the code used for index AMs and the users of
reloptions.

I'm not seeing any reasonable way to split this patch in smaller ones.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"No necesitamos banderas
 No reconocemos fronteras"                  (Jorge González)

В письме от пятница, 8 декабря 2023 г. 15:59:09 MSK пользователь Alvaro
Herrera написал:

> > Theoretically I can create patch with full options.c as it is in the patch
> > now, and use that code only in index AM, and keep reloption.c mostly
> > unchanged.
> >
> > This will be total mess with two different options mechanisms working in
> > the same time, but this might be much more easy to review.  When we are
> > done with the first step, we can change the rest.
> > If this will help to finally include patch into postgres, I can do it.
> > Will
> > that help you to review?
>
> I don't think that's better, because we could create slight
> inconsistencies between the code used for index AMs and the users of
> reloptions.
I've written quite good regression tests for it, there should be no
inconsistency.

> I'm not seeing any reasonable way to split this patch in smaller ones.
Actually me neither. Not a good one anyway.

But if somebody really need it to be split, it can be done that way.

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

2024-01 Commitfest.

Hi, This patch has a CF status of "Needs Review" [1], but it seems
there were CFbot test failures last time it was run [2]. Please have a
look and post an updated version if necessary.

======
[1] https://commitfest.postgresql.org/46/4688/
[2] https://cirrus-ci.com/task/5066432363364352

Kind Regards,
Peter Smith.

В письме от понедельник, 22 января 2024 г. 08:24:13 MSK пользователь Peter
Smith написал:

Hi!

I've updated the patch, and it should work with modern master branch.

> 2024-01 Commitfest.
>
> Hi, This patch has a CF status of "Needs Review" [1], but it seems
> there were CFbot test failures last time it was run [2]. Please have a
> look and post an updated version if necessary.

Do we still miss notification facility for CFbot? May be somebody have added it
since the time I've checked last time?

>
> ======
> [1] https://commitfest.postgresql.org/46/4688/
> [2] https://cirrus-ci.com/task/5066432363364352
>
> Kind Regards,
> Peter Smith.


--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

В письме от четверг, 1 февраля 2024 г. 09:58:32 MSK пользователь Nikolay
Shaplov написал:

I' ve updated the patch again, since the output of the
src/test/modules/test_oat_hooks test have changed.
This test began to register namespace lookups, and since options internals
have been changed much, number of lookups have been changed too. So I just
fixed the expected file.

> Hi!
>
> I've updated the patch, and it should work with modern master branch.
>
> > 2024-01 Commitfest.
> >
> > Hi, This patch has a CF status of "Needs Review" [1], but it seems
> > there were CFbot test failures last time it was run [2]. Please have a
> > look and post an updated version if necessary.
>
> Do we still miss notification facility for CFbot? May be somebody have added
> it since the time I've checked last time?
>
> > ======
> > [1] https://commitfest.postgresql.org/46/4688/
> > [2] https://cirrus-ci.com/task/5066432363364352
> >
> > Kind Regards,
> > Peter Smith.


--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su