From: Nikolay Shaplov <dhyan@nataraj.su>
To: jian he <jian.universality@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Yura Sokolov <y.sokolov@postgrespro.ru>
Cc: 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> (raw)
In-Reply-To: <CACJufxFSkfwQR0tPCgzQQjFw5CNxYsFFi7X_gFuPyRacrqKOPQ@mail.gmail.com>
References: <202312081259.2pwkb25emowf@alvherre.pgsql>
<2312843.ElGaqSPkdT@thinkpad-pgpro>
<CACJufxFSkfwQR0tPCgzQQjFw5CNxYsFFi7X_gFuPyRacrqKOPQ@mail.gmail.com>
В письме от понедельник, 3 июня 2024 г. 10:52:02 MSK пользователь jian he
написал:
Hi!
Thank you for giving attention to my patch, and sorry, it takes me some time
to deal with such things.
By the way, what is your further intentions concerning this patch? Should I
add you as a reviewer, or you are just passing by? Or add you as second
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
short. I do not know what else I can say there without diving deep into
implementation details. Do you have any ideas what can be added there?
> some places you use "Spec Set", other places you use "spec set"
...
> 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
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 what
is what. Hope it will make things more clear.
If not me know and we will try to find out what comments should be added
>
> + 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)
>
> 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
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)));
.....
> 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.
Fixing them better go as a separate patch I guess...
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
[text/x-patch] v8-0001-New-options-engine.patch (0B, ../2726217.uZKlY2gecq@thinkpad-pgpro/2-v8-0001-New-options-engine.patch)
download
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: pgsql-hackers@postgresql.org
Cc: dhyan@nataraj.su, jian.universality@gmail.com, smithpb2250@gmail.com, alvherre@alvh.no-ip.org, y.sokolov@postgrespro.ru, michael@paquier.xyz
Subject: Re: [PATCH] New [relation] option engine
In-Reply-To: <2726217.uZKlY2gecq@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 DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox