From: Nikolay Shaplov <dhyan@nataraj.su>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@lists.postgresql.org,
 Michael Paquier <michael@paquier.xyz>, Amit Langote <amitlangote09@gmail.com>
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: <202205151225.jbalr77h46jc@alvherre.pgsql>
References: <202205151225.jbalr77h46jc@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart3198980.n4Qna3SLz4";
 micalg="pgp-sha512"; protocol="application/pgp-signature"
Archived-At: 
 <https://www.postgresql.org/message-id/3242859.cHiyl0VpJ2%40thinkpad-pgpro>
Precedence: bulk

--nextPart3198980.n4Qna3SLz4
Content-Type: multipart/mixed; boundary="nextPart5904915.V09D27m8HI"; protected-headers="v1"
Content-Transfer-Encoding: 7Bit
From: Nikolay Shaplov <dhyan@nataraj.su>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>, Amit Langote <amitlangote09@gmail.com>
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: <202205151225.jbalr77h46jc@alvherre.pgsql>
References: <202205151225.jbalr77h46jc@alvherre.pgsql>

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"