> On Oct 3, 2025, at 14:55, Peter Eisentraut <[email protected]> wrote:
>
>
> The patches in the series are arranged so that they can be considered and
> applied incrementally.
> <v1-0001-Modernize-some-for-loops.patch><v1-0002-Add-some-const-qualifiers.patch><v1-0003-Change-reset_extra-into-a-config_generic-common-f.patch><v1-0004-Use-designated-initializers-for-guc_tables.patch><v1-0005-Change-config_generic.vartype-to-be-initialized-a.patch><v1-0006-Reorganize-GUC-structs.patch><v1-0007-Sort-guc_parameters.dat-alphabetically-by-name.patch><v1-0008-Enforce-alphabetical-order-in-guc_parameters.dat.patch>
0001 - looks good to me. Basically it only moves for loop’s loop variable type
definition into for(), it isn’t tied to rest commits, I guess it can be pushed
independently.
0002 - also looks good. It just add “const” where possible. I think it can be
pushed together with 0001.
0003 - also looks good. It moves “reset_extra” from individual typed config
structure to “config_generic”, which dramatically eliminates unnecessary
switch-cases. Just a small comment:
```
@@ -6244,29 +6217,11 @@ RestoreGUCState(void *gucstate)
switch (gconf->vartype)
{
case PGC_BOOL:
- {
- struct config_bool *conf = (struct
config_bool *) gconf;
-
- if (conf->reset_extra &&
conf->reset_extra != gconf->extra)
- guc_free(conf->reset_extra);
- break;
- }
case PGC_INT:
- {
- struct config_int *conf = (struct
config_int *) gconf;
-
- if (conf->reset_extra &&
conf->reset_extra != gconf->extra)
- guc_free(conf->reset_extra);
- break;
- }
case PGC_REAL:
- {
- struct config_real *conf = (struct
config_real *) gconf;
-
- if (conf->reset_extra &&
conf->reset_extra != gconf->extra)
- guc_free(conf->reset_extra);
- break;
- }
+ case PGC_ENUM:
+ /* no need to do anything */
+ break;
case PGC_STRING:
{
struct config_string *conf = (struct
config_string *) gconf;
@@ -6274,19 +6229,11 @@ RestoreGUCState(void *gucstate)
guc_free(*conf->variable);
if (conf->reset_val && conf->reset_val
!= *conf->variable)
guc_free(conf->reset_val);
- if (conf->reset_extra &&
conf->reset_extra != gconf->extra)
- guc_free(conf->reset_extra);
- break;
- }
- case PGC_ENUM:
- {
- struct config_enum *conf = (struct
config_enum *) gconf;
-
- if (conf->reset_extra &&
conf->reset_extra != gconf->extra)
- guc_free(conf->reset_extra);
break;
}
}
```
Do we still need this switch-case? As only PGC_STRING needs to do something,
why not just do:
```
If (gconf-vartype == PGC_STRING)
{
…
}
if (gconf->reset_extra && gconf->reset_extra != gconf->extra)
guc_free(gconf->reset_extra);
```
0004 - Also looks good to me. But I am not an expert of perl programming, so I
cannot comment much.
0005 - Also looks good to me. This is a small change. It updates the perl
script to set vartype so that vartype gets set at compile time, which further
simplifies the c code.
0006 - Comes to the most interesting part of this patch. It moves all typed
config into “config_generic” as a unnamed union, then replaces typed config
with config_generic everywhere. Though this commit touches a lot of lines of
code, but all changes are straightforward. Also looks good to me.
0007 needs a rebase. I guess you may split 0007 and 0008 to a separate patch
once 0001-0006 are pushed. As they only re-order GUC variables without real
logic change, so they can be quickly reviewed and pushed, otherwise it would be
painful where every commit the touches GUC would lead to a rebase.
0008 - I don’t review it as 0007 needs a rebase.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
