Обсуждение: Reorganize GUC structs

This patch series does some reorganizing of the GUC table structs 
(config_generic, config_bool, etc.) to modernize and simplify things.  I 
suspect that some changes like these would have been desirable as the 
GUC code has evolved over time, but were avoided because of the burden 
of having to adjust all the static GUC tables at the same time.  Since 
these are now generated, this is no longer a problem.

The main change is that instead of having five separate structs, one for 
each type, with the generic part contained in each of them, flip it 
around and have one common struct, with the type-specific part has a 
subfield.

The very original GUC design had type-specific structs and
type-specific lists, and the membership in one of the lists defined
the type.  But now the structs themselves know the type (from the
.vartype field), and they are all loaded into a common hash table at
run time, this original separation no longer makes sense.  It creates
a bunch of inconsistencies in the code about whether the type-specific
or the generic struct is the primary struct, and a lot of casting in
between, which makes certain assumptions about the struct layouts.

After the change, all these casts are gone and all the data is
accessed via normal field references.  Also, various code is
simplified because only one kind of struct needs to be processed.

Additionally, I have sorted guc_parameters.dat alphabetically by name, 
and have added code to enforce the sort order going forward.  (Note: The 
order is actually checked after lower-casing, to handle the likes of 
"DateStyle".)

The order in these lists was previously pretty random and had grown
organically over time.  This made it unnecessarily cumbersome to
maintain these lists, as there was no clear guidelines about where to
put new entries.  Also, after the merger of the type-specific GUC
structs, the list still reflected the previous type-specific
super-order.

By enforcing alphabetical order, the place for new entries becomes 
clear, and often related entries will be listed close together.

The patches in the series are arranged so that they can be considered 
and applied incrementally.

On Fri, Oct 3, 2025 at 1:55 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> Additionally, I have sorted guc_parameters.dat alphabetically by name,
> and have added code to enforce the sort order going forward.  (Note: The

That makes sense by default. Another possibility would be to keep them
in the same order as postgresql.conf.sample. You mentioned earlier it
may be possible to generate the latter, although it's not yet clear if
that could be done easily. If not, then keeping them in sync would be
extra work.

--
John Naylor
Amazon Web Services

On 2025-Oct-13, John Naylor wrote:

> On Fri, Oct 3, 2025 at 1:55 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> > Additionally, I have sorted guc_parameters.dat alphabetically by name,
> > and have added code to enforce the sort order going forward.
> 
> That makes sense by default. Another possibility would be to keep them
> in the same order as postgresql.conf.sample. You mentioned earlier it
> may be possible to generate the latter, although it's not yet clear if
> that could be done easily. If not, then keeping them in sync would be
> extra work.

I agree that keeping the guc_parameters.dat file alphabetically sorted
by default would keep the maintenance cost lowest, because we won't have
to make any subjective decisions for new entries.  However, automatically
generating the .sample file sounds impractical, considering the
free-form comments that we currently have there.  For instance

#max_prepared_transactions = 0      # zero disables the feature
                    # (change requires restart)
# Caution: it is not advisable to set max_prepared_transactions nonzero unless
# you actively intend to use prepared transactions.

We'd have to store the comments together in the guc_parameters.dat table
so that the sample file knows where and how to put them, and it would
IMO look worse.  

Also, the order of entries in postgresql.conf.sample was at some point
carefully considered.  Messing that up (by making the entries match
alphabetical order, even within each section) would also be worse IMO.
So we'd have to add values for the sorting to know where in the file
each section goes, what its opening comment is, and where each entry
goes within each section.

I think instead of that mess, maybe we can simply keep the sample file
as-is, cross-check that a line for each non-hidden GUC variable exists,
and perhaps that the commented-out default value matches the data file.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".

On 13.10.25 13:39, Álvaro Herrera wrote:
> I agree that keeping the guc_parameters.dat file alphabetically sorted
> by default would keep the maintenance cost lowest, because we won't have
> to make any subjective decisions for new entries.  However, automatically
> generating the .sample file sounds impractical, considering the
> free-form comments that we currently have there.

Yes, I'm not sure if it's practical in the fullest extent.  But there 
were at various points discussions about alternative layouts and 
contents for postgresql.conf.sample, with a lot of opinions.  With this 
new framework, I think it might be interesting to experiment.  Which is 
why I mentioned it.

> I think instead of that mess, maybe we can simply keep the sample file
> as-is, cross-check that a line for each non-hidden GUC variable exists,
> and perhaps that the commented-out default value matches the data file.

Those would certainly be reasonable near-term steps.

On 09.10.25 09:15, Chao Li wrote:
> 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.

I have committed these first three.  Attached is the rest of the patch 
series rebased.

It looks like we have consensus in principle on the remaining changes, 
but I'll leave them out here a while longer in case there are any 
further thoughts.

Regarding ...

> 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);
> ```

I initially had it like that, but there are a couple of other places in 
the existing code that have a switch with only actual code for 
PGC_STRING, so I ended up following that pattern.

> @@ -261,15 +261,15 @@ static bool assignable_custom_variable_name(const char *name, bool skip_errors,
>                                              int elevel);
>  static void do_serialize(char **destptr, Size *maxbytes,
>                           const char *fmt,...) pg_attribute_printf(3, 4);
> -static bool call_bool_check_hook(const struct config_bool *conf, bool *newval,
> +static bool call_bool_check_hook(const struct config_generic *conf, bool *newval,
>                                   void **extra, GucSource source, int elevel);
> -static bool call_int_check_hook(const struct config_int *conf, int *newval,
> +static bool call_int_check_hook(const struct config_generic *conf, int *newval,
>                                  void **extra, GucSource source, int elevel);
> -static bool call_real_check_hook(const struct config_real *conf, double *newval,
> +static bool call_real_check_hook(const struct config_generic *conf, double *newval,
>                                   void **extra, GucSource source, int elevel);
> -static bool call_string_check_hook(const struct config_string *conf, char **newval,
> +static bool call_string_check_hook(const struct config_generic *conf, char **newval,
>                                     void **extra, GucSource source, int elevel);
> -static bool call_enum_check_hook(const struct config_enum *conf, int *newval,
> +static bool call_enum_check_hook(const struct config_generic *conf, int *newval,
>                                   void **extra, GucSource source, int elevel);

The new signatures for these function are less specific than before, 
making them a little worse IMO. Overall +1 on the patches, despite that 
little drawback.

- Heikki

On 24.10.25 14:21, Heikki Linnakangas wrote:
>> @@ -261,15 +261,15 @@ static bool 
>> assignable_custom_variable_name(const char *name, bool skip_errors,
>>                                              int elevel);
>>  static void do_serialize(char **destptr, Size *maxbytes,
>>                           const char *fmt,...) pg_attribute_printf(3, 4);
>> -static bool call_bool_check_hook(const struct config_bool *conf, bool 
>> *newval,
>> +static bool call_bool_check_hook(const struct config_generic *conf, 
>> bool *newval,
>>                                   void **extra, GucSource source, int 
>> elevel);
>> -static bool call_int_check_hook(const struct config_int *conf, int 
>> *newval,
>> +static bool call_int_check_hook(const struct config_generic *conf, 
>> int *newval,
>>                                  void **extra, GucSource source, int 
>> elevel);
>> -static bool call_real_check_hook(const struct config_real *conf, 
>> double *newval,
>> +static bool call_real_check_hook(const struct config_generic *conf, 
>> double *newval,
>>                                   void **extra, GucSource source, int 
>> elevel);
>> -static bool call_string_check_hook(const struct config_string *conf, 
>> char **newval,
>> +static bool call_string_check_hook(const struct config_generic *conf, 
>> char **newval,
>>                                     void **extra, GucSource source, 
>> int elevel);
>> -static bool call_enum_check_hook(const struct config_enum *conf, int 
>> *newval,
>> +static bool call_enum_check_hook(const struct config_generic *conf, 
>> int *newval,
>>                                   void **extra, GucSource source, int 
>> elevel);
> 
> The new signatures for these function are less specific than before, 
> making them a little worse IMO. Overall +1 on the patches, despite that 
> little drawback.

Thanks, pushed.

On 29.10.25 10:07, Peter Eisentraut wrote:
> On 24.10.25 14:21, Heikki Linnakangas wrote:
>>> @@ -261,15 +261,15 @@ static bool 
>>> assignable_custom_variable_name(const char *name, bool skip_errors,
>>>                                              int elevel);
>>>  static void do_serialize(char **destptr, Size *maxbytes,
>>>                           const char *fmt,...) pg_attribute_printf(3, 
>>> 4);
>>> -static bool call_bool_check_hook(const struct config_bool *conf, 
>>> bool *newval,
>>> +static bool call_bool_check_hook(const struct config_generic *conf, 
>>> bool *newval,
>>>                                   void **extra, GucSource source, int 
>>> elevel);
>>> -static bool call_int_check_hook(const struct config_int *conf, int 
>>> *newval,
>>> +static bool call_int_check_hook(const struct config_generic *conf, 
>>> int *newval,
>>>                                  void **extra, GucSource source, int 
>>> elevel);
>>> -static bool call_real_check_hook(const struct config_real *conf, 
>>> double *newval,
>>> +static bool call_real_check_hook(const struct config_generic *conf, 
>>> double *newval,
>>>                                   void **extra, GucSource source, int 
>>> elevel);
>>> -static bool call_string_check_hook(const struct config_string *conf, 
>>> char **newval,
>>> +static bool call_string_check_hook(const struct config_generic 
>>> *conf, char **newval,
>>>                                     void **extra, GucSource source, 
>>> int elevel);
>>> -static bool call_enum_check_hook(const struct config_enum *conf, int 
>>> *newval,
>>> +static bool call_enum_check_hook(const struct config_generic *conf, 
>>> int *newval,
>>>                                   void **extra, GucSource source, int 
>>> elevel);
>>
>> The new signatures for these function are less specific than before, 
>> making them a little worse IMO. Overall +1 on the patches, despite 
>> that little drawback.
> 
> Thanks, pushed.

The remaining patches to sort the list alphabetically have also been pushed.

On 03.11.25 12:16, Peter Eisentraut wrote:
> The remaining patches to sort the list alphabetically have also been 
> pushed.

Here are a few more small patches to fix related things I found 
afterwards or in passing.

The first one straightens out how the qsort comparison function for GUC 
records works, and also removes the implicit requirement that the name 
field is first in the struct.

The second one fixes up an NLS build rule for the recent changes.

The third fixes some translation markers.  This is an older problem.

On 2025-Nov-07, Peter Eisentraut wrote:

> @@ -3135,14 +3135,14 @@ parse_and_validate_value(const struct config_generic *record,
>                      char       *hintmsg;
>  
>                      hintmsg = config_enum_get_options(conf,
> -                                                      "Available values: ",
> -                                                      ".", ", ");
> +                                                      _("Available values: "),
> +                                                      _("."), _(", "));

Hmm, it seems unclear (from the message catalog perspective) what the
last two strings will be used for, so please add a /* translator */
comment to each.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La fuerza no está en los medios físicos
sino que reside en una voluntad indomable" (Gandhi)

Peter Eisentraut <peter@eisentraut.org> writes:
> Here are a few more small patches to fix related things I found
> afterwards or in passing.

Looks sane, except that the comparator could do with an extra "const"
if there's enough room on the line:

+    const struct config_generic *ca = *(const struct config_generic *const *) a;
+    const struct config_generic *cb = *(const struct config_generic *const *) b;

Not essential, but casting const away even transiently looks ugly.

            regards, tom lane

On 07.11.25 10:38, Álvaro Herrera wrote:
> On 2025-Nov-07, Peter Eisentraut wrote:
> 
>> @@ -3135,14 +3135,14 @@ parse_and_validate_value(const struct config_generic *record,
>>                       char       *hintmsg;
>>   
>>                       hintmsg = config_enum_get_options(conf,
>> -                                                      "Available values: ",
>> -                                                      ".", ", ");
>> +                                                      _("Available values: "),
>> +                                                      _("."), _(", "));
> 
> Hmm, it seems unclear (from the message catalog perspective) what the
> last two strings will be used for, so please add a /* translator */
> comment to each.

Actually, the ", " already exists in the message catalog, so adding a 
comment here might confuse the other site.

On 2025-Nov-07, Peter Eisentraut wrote:

> Actually, the ", " already exists in the message catalog, so adding a
> comment here might confuse the other site.

This one?
#: replication/logical/relation.c:245
#, c-format
msgid ", \"%s\""
msgstr ", »%s«"

It's different.  (And warrants a comment too, IMO)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)

On 07.11.25 16:14, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> Here are a few more small patches to fix related things I found
>> afterwards or in passing.
> 
> Looks sane, except that the comparator could do with an extra "const"
> if there's enough room on the line:
> 
> +    const struct config_generic *ca = *(const struct config_generic *const *) a;
> +    const struct config_generic *cb = *(const struct config_generic *const *) b;
> 
> Not essential, but casting const away even transiently looks ugly.

committed with that amendment

On 07.11.25 16:53, Álvaro Herrera wrote:
> On 2025-Nov-07, Peter Eisentraut wrote:
> 
>> Actually, the ", " already exists in the message catalog, so adding a
>> comment here might confuse the other site.
> 
> This one?
> #: replication/logical/relation.c:245
> #, c-format
> msgid ", \"%s\""
> msgstr ", »%s«"
> 
> It's different.  (And warrants a comment too, IMO)

Well, that's the one, but the code actually looks like this now:

     while ((i = bms_next_member(atts, i)) >= 0)
     {
         attcnt++;
         if (attcnt > 1)
             appendStringInfoString(&attsbuf, _(", "));

         appendStringInfo(&attsbuf, _("\"%s\""), remoterel->attnames[i]);
     }

The catalog entries you are showing appear to be from pre-PG18.

There are also similar instances in ExecBuildSlotValueDescription() that 
are not marked up for gettext but perhaps should be.  There might be 
others, and I would expect more like this to appear over time, as people 
like to add more detail like this to diagnostics messages.

Do you have a suggestion what kind of comment to attach there?

On 2025-Nov-12, Peter Eisentraut wrote:

> Well, that's the one, but the code actually looks like this now:
> 
>     while ((i = bms_next_member(atts, i)) >= 0)
>     {
>         attcnt++;
>         if (attcnt > 1)
>             appendStringInfoString(&attsbuf, _(", "));
> 
>         appendStringInfo(&attsbuf, _("\"%s\""), remoterel->attnames[i]);
>     }
> 
> The catalog entries you are showing appear to be from pre-PG18.

Ah, right.

> There are also similar instances in ExecBuildSlotValueDescription() that are
> not marked up for gettext but perhaps should be.  There might be others, and
> I would expect more like this to appear over time, as people like to add
> more detail like this to diagnostics messages.

Sure.

> Do you have a suggestion what kind of comment to attach there?

I would say "This is a separator in a list of entity names."  We don't
need to be specific as to what kind of entity it is, I think.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree.              (Don Knuth)

On 14.11.25 12:00, Álvaro Herrera wrote:
> On 2025-Nov-12, Peter Eisentraut wrote:
> 
>> Well, that's the one, but the code actually looks like this now:
>>
>>      while ((i = bms_next_member(atts, i)) >= 0)
>>      {
>>          attcnt++;
>>          if (attcnt > 1)
>>              appendStringInfoString(&attsbuf, _(", "));
>>
>>          appendStringInfo(&attsbuf, _("\"%s\""), remoterel->attnames[i]);
>>      }
>>
>> The catalog entries you are showing appear to be from pre-PG18.
> 
> Ah, right.
> 
>> There are also similar instances in ExecBuildSlotValueDescription() that are
>> not marked up for gettext but perhaps should be.  There might be others, and
>> I would expect more like this to appear over time, as people like to add
>> more detail like this to diagnostics messages.
> 
> Sure.
> 
>> Do you have a suggestion what kind of comment to attach there?
> 
> I would say "This is a separator in a list of entity names."  We don't
> need to be specific as to what kind of entity it is, I think.

Ok, committed that way.