Re: Reorganize GUC structs

> 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.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/