Re: [PATCH] New [relation] option engine

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