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&amp;cid=4647152)

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.dat guc_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.dat guc_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.dat guc_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/

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

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?

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.

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'
```

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

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.

On Wed, Nov 19, 2025 at 7:56 PM Dagfinn Ilmari Mannsåker <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'
```

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.