public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nikolay Shaplov <[email protected]>
To: Alvaro Herrera <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: [email protected]
Cc: Michael Paquier <[email protected]>
Cc: Amit Langote <[email protected]>
Subject: Re: [PATCH] New [relation] option engine
Date: Tue, 17 May 2022 15:09:55 +0300
Message-ID: <3242859.cHiyl0VpJ2@thinkpad-pgpro> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
--nextPart3198980.n4Qna3SLz4
Content-Type: multipart/mixed; boundary="nextPart5904915.V09D27m8HI"; protected-headers="v1"
Content-Transfer-Encoding: 7Bit
From: Nikolay Shaplov <[email protected]>
To: Alvaro Herrera <[email protected]>
Cc: Andres Freund <[email protected]>, [email protected], Michael Paquier <[email protected]>, Amit Langote <[email protected]>
Subject: Re: [PATCH] New [relation] option engine
Date: Tue, 17 May 2022 15:09:55 +0300
Message-ID: <3242859.cHiyl0VpJ2@thinkpad-pgpro>
Organization: Postgres Professional
In-Reply-To: <[email protected]>
References: <[email protected]>
This is a multi-part message in MIME format.
--nextPart5904915.V09D27m8HI
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"
=D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 =D0=B2=D0=BE=D1=81=
=D0=BA=D1=80=D0=B5=D1=81=D0=B5=D0=BD=D1=8C=D0=B5, 15 =D0=BC=D0=B0=D1=8F 202=
2 =D0=B3. 15:25:47 MSK =D0=BF=D0=BE=D0=BB=D1=8C=D0=B7=D0=BE=D0=B2=D0=B0=D1=
=82=D0=B5=D0=BB=D1=8C Alvaro=20
Herrera =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB:
> 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:
>=20
> 58 files changed, 2714 insertions(+), 2368 deletions(-)
>=20
> 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=
=20
(not only reloptions). While working with this goal I came to conclusion th=
at=20
I have to create completely new option engine that will be used anywhere=20
options (name=3Dvalue) is needed. This will solve following problems:
=2D provide unified API for options definition. Currently postgres have cor=
e-AM =20
options, contrib-AM options and local options for opclasses, each have thei=
r=20
own way to define options. This patch will allow to use one API for them al=
l=20
(for opclasses it is still WIP)
=2D Currently core-AM options are partly defined in reloptions.c and partly=
in AM=20
code. This is error prone. This patch fixes that.
=2D For indexes option definition is moved into AM code, where they should =
be.=20
=46or heap it should be moved into AM code later.=20
=2D There is no difference for core-AM indexes, and contrib-AM indexes opti=
ons.=20
They use same API.
I also tried to write detailed commit message as you've suggested. There my=
=20
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=20
lines (not string consts) that were longer then 80 characters, afterwards.=
=20
Hope it was right decision=20
> 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 chang=
ing=20
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 we=
re=20
commit. Then I've just copied it to this patch. I do not quite understand h=
ow=20
does it works. But I can say that update-po works well for enum detailmsg, =
and=20
we actually have gettext_noop(), but it is used while calling=20
optionsSpecSetAddEnum, not when error message is actually printed. But I gu=
ess=20
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.=20
=2D-=20
Nikolay Shaplov aka Nataraj
=46uzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
--nextPart5904915.V09D27m8HI
Content-Disposition: attachment; filename="new_options_take_two_v03.patch"
Content-Transfer-Encoding: 7Bit
Content-Type: text/x-patch; charset="UTF-8"; name="new_options_take_two_v03.patch"
view thread (20+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] New [relation] option engine
In-Reply-To: <3242859.cHiyl0VpJ2@thinkpad-pgpro>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox