Re: [PATCH] New [relation] option engine - Mailing list pgsql-hackers

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