Обсуждение: gen_guc_tables.pl: Validate required GUC fields before code generation

Chao Li <li.evan.chao@gmail.com> writes:

> Hi Hacker,
>
> While working on the other patch and editing guc_parameters.dat, I
> mistakenly deleted a “max” value line, then I got the following error
> during build:
>
> ```
> '/opt/homebrew/bin/perl' ../../../src/backend/utils/misc/gen_guc_tables.pl
> ../../../src/backend/utils/misc/guc_parameters.dat guc_tables.inc.c
> Use of uninitialized value in printf at ../../../src/backend/utils/misc/
> gen_guc_tables.pl line 78.
> make[2]: *** [guc_tables.inc.c] Error 25
> ```

Yeah, that's not a very helpful error.

> The error message is unclear and is not helpful to identify the real issue.
>
> I am not an expert of perl, but I tried to make an enhancement that will
> print an error message like:

The patch looks good overall, but it could be made a bit more concise
(without sacrificing readability, IMO):

> +sub validate_guc_entry
> +{
> +    my ($entry) = @_;
> +
> +    my @required_common = qw(name type context group short_desc variable boot_val);
> +
> +    # Type-specific required fields
> +    my %required_by_type = (
> +        int  => [qw(min max)],
> +        real => [qw(min max)],
> +        enum => [qw(options)],
> +        bool => [],   # no extra required fields
> +        string => [], # no extra required fields
> +    );

Since we're checking if the key exists, there's no point in having these
empty arrays for types that don't have any extra fields.

> +    # Check common required fields
> +    for my $f (@required_common) {
> +        unless (defined $entry->{$f}) {
> +            die sprintf("error: guc_parameters.data: '%s' is missing required field '%s'\n",
> +                        $entry->{name} // '<unknown>', $f);
> +        }
> +    }
> +
> +    # Check type-specific fields
> +    if (exists $required_by_type{$entry->{type}}) {
> +        for my $f (@{ $required_by_type{$entry->{type}} }) {
> +            unless (defined $entry->{$f}) {
> +                die sprintf("error: guc_parameters.data: '%s' of type '%s' is missing required field '%s'\n",
> +                            $entry->{name}, $entry->{type}, $f);
> +            }
> +        }
> +    }
> +}

These two loops could be combined, with something like:

    for my $f (@required_common, @{ $required_by_type{$entry->{type}} // [] }) {

The `// []` makes it default to an empty array for types that don't have
any extra fields, so we don't need to list them in the %required_by_type
hash.  OTOH, we could instead throw an error if there's no entry, to
catch typos in the type field here, instead of only when we get around
to compiling the generated file. But that should IMO be done after the
checking for required fields, so we want the fallback here even if we
have the empty arrays.

There's no need for a `// ''` on the `->{type}` lookup, since it
would have died while checking the common fields if that were missing.

- ilmari
-- resident perl nitpicker

On 2025-Nov-10, Chao Li wrote:

> While working on the other patch and editing guc_parameters.dat, I
> mistakenly deleted a “max” value line, then I got the following error
> during build:

> The error message is unclear and is not helpful to identify the real issue.

Funny -- I made a very similar mistake recently and was also going to
complain about the resulting error message, so +1 for getting it
improved along the lines you proposed, plus Ilmari's suggestions.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Those who use electric razors are infidels destined to burn in hell while
we drink from rivers of beer, download free vids and mingle with naked
well shaved babes." (http://slashdot.org/comments.pl?sid=44793&cid=4647152)

Added to CF: https://commitfest.postgresql.org/patch/6226/

> On Nov 12, 2025, at 09:10, Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> On Mon, Nov 10, 2025 at 8:02 PM Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote:
> Chao Li <li.evan.chao@gmail.com> writes:
>
> > Hi Hacker,
> >
> > While working on the other patch and editing guc_parameters.dat, I
> > mistakenly deleted a “max” value line, then I got the following error
> > during build:
> >
> > ```
> > '/opt/homebrew/bin/perl' ../../../src/backend/utils/misc/gen_guc_tables.pl
> > ../../../src/backend/utils/misc/guc_parameters.dat guc_tables.inc.c
> > Use of uninitialized value in printf at ../../../src/backend/utils/misc/
> > gen_guc_tables.pl line 78.
> > make[2]: *** [guc_tables.inc.c] Error 25
> > ```
>
> Yeah, that's not a very helpful error.
>
> > The error message is unclear and is not helpful to identify the real issue.
> >
> > I am not an expert of perl, but I tried to make an enhancement that will
> > print an error message like:
>
> The patch looks good overall, but it could be made a bit more concise
> (without sacrificing readability, IMO):
>
> > +sub validate_guc_entry
> > +{
> > +    my ($entry) = @_;
> > +
> > +    my @required_common = qw(name type context group short_desc variable boot_val);
> > +
> > +    # Type-specific required fields
> > +    my %required_by_type = (
> > +        int  => [qw(min max)],
> > +        real => [qw(min max)],
> > +        enum => [qw(options)],
> > +        bool => [],   # no extra required fields
> > +        string => [], # no extra required fields
> > +    );
>
> Since we're checking if the key exists, there's no point in having these
> empty arrays for types that don't have any extra fields.
>
> Deleted bool and string.
>
> > +    # Check common required fields
> > +    for my $f (@required_common) {
> > +        unless (defined $entry->{$f}) {
> > +            die sprintf("error: guc_parameters.data: '%s' is missing required field '%s'\n",
> > +                        $entry->{name} // '<unknown>', $f);
> > +        }
> > +    }
> > +
> > +    # Check type-specific fields
> > +    if (exists $required_by_type{$entry->{type}}) {
> > +        for my $f (@{ $required_by_type{$entry->{type}} }) {
> > +            unless (defined $entry->{$f}) {
> > +                die sprintf("error: guc_parameters.data: '%s' of type '%s' is missing required field '%s'\n",
> > +                            $entry->{name}, $entry->{type}, $f);
> > +            }
> > +        }
> > +    }
> > +}
>
> These two loops could be combined, with something like:
>
>     for my $f (@required_common, @{ $required_by_type{$entry->{type}} // [] }) {
>
> The `// []` makes it default to an empty array for types that don't have
> any extra fields, so we don't need to list them in the %required_by_type
> hash.  OTOH, we could instead throw an error if there's no entry, to
> catch typos in the type field here, instead of only when we get around
> to compiling the generated file. But that should IMO be done after the
> checking for required fields, so we want the fallback here even if we
> have the empty arrays.
>
> Combined the two loops.
>
> There's no need for a `// ''` on the `->{type}` lookup, since it
> would have died while checking the common fields if that were missing.
>
>
> Based on my test, a `// ''` on the `->{type}` lookup is still needed, otherwise if I remove a "type" field, then I
got:
>
> ```
> '/opt/homebrew/bin/perl' ../../../src/backend/utils/misc/gen_guc_tables.pl
../../../src/backend/utils/misc/guc_parameters.datguc_tables.inc.c 
> Use of uninitialized value in hash element at ../../../src/backend/utils/misc/gen_guc_tables.pl line 55.
> ```
>
> In v2, I also added line number to the error message, because I found that if "name" field is missing or typo on
"name",then the error message without a line number doesn't help either, like: 
> ```
> '/opt/homebrew/bin/perl' ../../../src/backend/utils/misc/gen_guc_tables.pl
../../../src/backend/utils/misc/guc_parameters.datguc_tables.inc.c 
> error: guc_parameters.data: '<unknown>' of type 'int' is missing required field 'name'
> ```
>  With a line number, we can easily identify the error place:
> ```
> '/opt/homebrew/bin/perl' ../../../src/backend/utils/misc/gen_guc_tables.pl
../../../src/backend/utils/misc/guc_parameters.datguc_tables.inc.c 
> error: guc_parameters.data line 1882: '<unknown>' of type 'int' is missing required field 'name'
> ```
>
> Best regards,
> Chao Li (Evan)
> ---------------------
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>  <v2-0001-gen_guc_tables.pl-Validate-required-GUC-fields-be.patch>

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

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

I tested patch v2 on top of current master.

- The patch applies cleanly.
- Full build and regression tests pass (“All 231 tests passed.”).
- I tested the validation by removing the `max` field from an int GUC
  (archive_timeout). The script now reports a clear error pointing to the
  exact line and missing field, instead of Perl “uninitialized value” warnings.
- Verified that the generated guc_tables.c output is unchanged.
- The added validation code is small and straightforward, and integrates
  cleanly into the existing script.

Everything works as described.  
I think this patch is ready for committer.

Reviewed-by: Mahmoud Ayman

The new status of this patch is: Ready for Committer

I find the data structures that you have constructed here barely 
understandable:

     my %required_by_type = (
         int  => [qw(min max)],
         real => [qw(min max)],
         enum => [qw(options)],
     );

     for my $f (@required_common, @{ $required_by_type{$entry->{type} // 
''} // [] }) {

[qw(min max)] is an array inside an array reference?  I think?  Do we 
need two levels of nesting?

I think this // notation is unnecessarily confusing, and why do we need 
two of them.  I thought your first patch

+        bool => [],   # no extra required fields
+        string => [], # no extra required fields

was clearer.  And that way, we also check that the field type is one of 
the ones we support.

Chao Li <li.evan.chao@gmail.com> writes:

> On Wed, Nov 19, 2025 at 4:33 PM Peter Eisentraut <peter@eisentraut.org>
> wrote:
>
>> I find the data structures that you have constructed here barely
>> understandable:
>>
>>      my %required_by_type = (
>>          int  => [qw(min max)],
>>          real => [qw(min max)],
>>          enum => [qw(options)],
>>      );
>>
>>      for my $f (@required_common, @{ $required_by_type{$entry->{type} //
>> ''} // [] }) {
>>
>> [qw(min max)] is an array inside an array reference?  I think?  Do we
>> need two levels of nesting?

No there is no nesting. qw() is a quote operator ("quote words") that
returns a list of the words inside, and [] creates an array of the
elements of the list and returns a reference to it, So [qw(min max)] is
equivalent to ['min', 'max'].

>> I think this // notation is unnecessarily confusing, and why do we need
>> two of them.  I thought your first patch
>>
>> +        bool => [],   # no extra required fields
>> +        string => [], # no extra required fields
>>
>> was clearer.  And that way, we also check that the field type is one of
>> the ones we support.
>
> Yeah, the two levels of nesting is not necessary. It was to address the
> review comments of v1, I removed bool and string from required_by_type and
> combined the two loops into one. Because bool and string don't exist in
> required_by_type, so that $required_by_type{$entry->{type} needs a
> fallback, why's why // was added.

The fallback was only necessary if we wanted to combine the two
originally-identical loops to check all the required fields in one go,
before checking that specfied type is known.

But now that we're checking the type between the check for the common
and typ-specific fields that's no longer relevant.

Personally, as a long-time Perl programmer I prefer the conciseness of
the combined loop, but I guess in a predominantly C codebase it makes
sense to keep things simpler and more explicitly spelled out.

> v3 goes back to v1 plus I add a check for unknown field type per your
> suggestion, for example:
> ```
> error: guc_parameters.data line 2271: unknown GUC type 'intt'
> ```

We've already validated that the name field exists, so this error
message should include that, like the other ones do.

Otherwise, LGTM.

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

On 20.11.25 06:51, Chao Li wrote:
> 
> On Wed, Nov 19, 2025 at 7:56 PM Dagfinn Ilmari Mannsåker 
> <ilmari@ilmari.org <mailto:ilmari@ilmari.org>> wrote:
> 
> 
>      > v3 goes back to v1 plus I add a check for unknown field type per your
>      > suggestion, for example:
>      > ```
>      > error: guc_parameters.data line 2271: unknown GUC type 'intt'
>      > ```
> 
>     We've already validated that the name field exists, so this error
>     message should include that, like the other ones do.
> 
> 
> Make sense. I added field name to this error message in v4, now it looks 
> like:
> ```
> error: guc_parameters.dat line 2278: 'primary_conninfo' has unknown GUC 
> type 'string1'
> ```

Thanks, I committed this.  I made some changes to the error message 
format so that it matches the format recognized by editors, and to use 
PostgreSQL quoting style (double quotes).  I also removed some code 
comments that just restated what was already obvious from the code.