From: Nikolay Shaplov <dhyan@nataraj.su>
To: jian he <jian.universality@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>,
 Yura Sokolov <y.sokolov@postgrespro.ru>, pgsql-hackers@lists.postgresql.org,
 Michael Paquier <michael@paquier.xyz>
Subject: Re: [PATCH] New [relation] option engine
Date: Sun, 16 Jun 2024 17:07:21 +0300
Message-ID: <2726217.uZKlY2gecq@thinkpad-pgpro>
Organization: Postgres Professional
In-Reply-To: 
 <CACJufxFSkfwQR0tPCgzQQjFw5CNxYsFFi7X_gFuPyRacrqKOPQ@mail.gmail.com>
References: 
 <202312081259.2pwkb25emowf@alvherre.pgsql>
 <2312843.ElGaqSPkdT@thinkpad-pgpro>
 <CACJufxFSkfwQR0tPCgzQQjFw5CNxYsFFi7X_gFuPyRacrqKOPQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart2852682.AiC22s8V5E";
 micalg="pgp-sha512"; protocol="application/pgp-signature"
Archived-At: 
 <https://www.postgresql.org/message-id/2726217.uZKlY2gecq%40thinkpad-pgpro>
Precedence: bulk

--nextPart2852682.AiC22s8V5E
Content-Type: multipart/mixed; boundary="nextPart1901634.1xdlsreqCQ";
 protected-headers="v1"
Content-Transfer-Encoding: 7Bit
From: Nikolay Shaplov <dhyan@nataraj.su>
To: jian he <jian.universality@gmail.com>
Subject: Re: [PATCH] New [relation] option engine
Date: Sun, 16 Jun 2024 17:07:21 +0300
Message-ID: <2726217.uZKlY2gecq@thinkpad-pgpro>
Organization: Postgres Professional
MIME-Version: 1.0

This is a multi-part message in MIME format.

--nextPart1901634.1xdlsreqCQ
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=BF=D0=BE=D0=BD=
=D0=B5=D0=B4=D0=B5=D0=BB=D1=8C=D0=BD=D0=B8=D0=BA, 3 =D0=B8=D1=8E=D0=BD=D1=
=8F 2024 =D0=B3. 10:52:02 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 jian he=20
=D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB:

Hi!

Thank you for giving attention to my patch, and sorry, it takes me some tim=
e=20
to deal with such things.

By the way, what is your further intentions concerning this patch? Should I=
=20
add you as a reviewer,  or you are just passing by? Or add you as second=20
author, if you are planning to suggest big changes

So I tried to fix all issues you've pointed out:

> you've changed the  `typedef struct IndexAmRoutine`
> then we also need to update doc/src/sgml/indexam.sgml.

Oh. You are right. I've fixed that. The description I've created is quite=20
short. I do not know what else I can say there without diving deep into=20
implementation details. Do you have any ideas what can be added there?

> some places you use "Spec Set", other places you use "spec set"
=2E..
> here the comments, you used "spec_set".
> maybe we can make it more consistent?

This sounds reasonable. I've changed it to Spec Set and Options Spec Set=20
wherever I found alternative spelling.

> typedef enum option_value_status
> {
> OPTION_VALUE_STATUS_EMPTY, /* Option was just initialized */
> OPTION_VALUE_STATUS_RAW, /* Option just came from syntax analyzer in
> * has name, and raw (unparsed) value */
> OPTION_VALUE_STATUS_PARSED, /* Option was parsed and has link to catalog
> * entry and proper value */
> OPTION_VALUE_STATUS_FOR_RESET, /* This option came from ALTER xxx RESET */
> } option_value_status;

> overall I am not sure about the usage of option_value_status.
> using some real example demo different option_value_status usage would be
> great.

I've added explanatory comment before option_value_status enum explaining w=
hat=20
is what. Hope it will make things more clear.=20

If not me know and we will try to find out what comments should be added

>=20
> + errmsg("RESET must not include values for parameters")));
> the error message seems not so good?
eh... It does seem to be not good, but I've copied it from original code
https://github.com/postgres/postgres/blob/
00ac25a3c365004821e819653c3307acd3294818/src/backend/access/common/
reloptions.c#L1231

I would not dare changing it. I've already changed to many things here.

> you declared as:
> options_spec_set *get_stdrd_relopt_spec_set(bool is_for_toast);
> but you used as:
> options_spec_set * get_stdrd_relopt_spec_set(bool is_heap)
>=20
> maybe we refactor the usage as
> `options_spec_set * get_stdrd_relopt_spec_set(bool is_for_toast)`
My bad.
I've changed is_for_toast to is_heap once, and I guess I've missed function=
=20
declaration... Fixed it to be consistent.

> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("value %s out of bounds for option \"%s\"",
> + value, option->gen->name),
> + errdetail("Valid values are between \"%d\" and \"%d\".",
> +   optint->min, optint->max)));
=2E....
> I think the above two, change errdetail to errhint would be more
> appropriate.

Same as above... I've just copied error messages from the original code.=20
=46ixing them better go as a separate patch I guess...


=2D-=20
Nikolay Shaplov aka Nataraj
=46uzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

--nextPart1901634.1xdlsreqCQ
Content-Disposition: attachment; filename="v8-0001-New-options-engine.patch"
Content-Transfer-Encoding: 7Bit
Content-Type: text/x-patch; charset="unicode-2-0-utf-8";
 name="v8-0001-New-options-engine.patch"