Re: Reorganize GUC structs - Mailing list pgsql-hackers
| From | Chao Li |
|---|---|
| Subject | Re: Reorganize GUC structs |
| Date | |
| Msg-id | 651EC9E6-446E-4999-96A9-6919EE6532DC@gmail.com Whole thread Raw |
| In response to | Reorganize GUC structs (Peter Eisentraut <peter@eisentraut.org>) |
| List | pgsql-hackers |
On Oct 3, 2025, at 14:55, Peter Eisentraut <peter@eisentraut.org> 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.
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
HighGo Software Co., Ltd.
https://www.highgo.com/
pgsql-hackers by date: