public inbox for [email protected]
help / color / mirror / Atom feedFrom: Zsolt Parragi <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Subject: gen_guc_tables improvements
Date: Sun, 15 Mar 2026 12:18:53 +0000
Message-ID: <CAN4CZFP=3xUoXb9jpn5OWwicg+rbyrca8-tVmgJsQAa4+OExkw@mail.gmail.com> (raw)
Hello
While reviewing a patch, I noticed a typo in guc_params.dat. The code
compiled and seemingly worked, and I was very surprised that the
generator script didn't catch the mistake.
I looked into it, and I found several missing checks in
gen_guc_tables. I attached fixes for 4 that I think would definitely
improve the script (for now as separate patches, so it is easy to
select only some of them):
* 0001 fixes the issue that started this, it validates the allowed
field names, preventing typos in their names
* 0002 goes a step further and validates that fields specific to some
types can only appear for those types
* 0003 just improves the error reported by duplicate names, previously
this was confusing (it referred to incorrect ordering)
* 0004 adds basic checks about allowed characters in GUC names
I was also thinking about adding validations for the enum/define
values (config group, flags, guc context), but that requires a
somewhat fragile extraction code, and I decided to leave that out for
now.
What do you think about these changes?
Attachments:
[application/octet-stream] 0001-gen_guc_tables-reject-unrecognized-field-names-in-gu.patch (1.5K, 2-0001-gen_guc_tables-reject-unrecognized-field-names-in-gu.patch)
download | inline diff:
From 2dea67bdc43b93c8d85979cc4f4a0ed839dad6e5 Mon Sep 17 00:00:00 2001
From: Zsolt Parragi <[email protected]>
Date: Sun, 15 Mar 2026 11:21:33 +0000
Subject: [PATCH 1/4] gen_guc_tables: reject unrecognized field names in
guc_parameters.dat
Previously, a typo in a field name (e.g. "contex" instead of "context")
would be silently ignored, potentially leading to subtle bugs or missing
configuration. Add a whitelist of all recognized fields and report an
error if any entry contains an unknown key.
---
src/backend/utils/misc/gen_guc_tables.pl | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/src/backend/utils/misc/gen_guc_tables.pl b/src/backend/utils/misc/gen_guc_tables.pl
index a285c62f98d..ba2e6f001f1 100644
--- a/src/backend/utils/misc/gen_guc_tables.pl
+++ b/src/backend/utils/misc/gen_guc_tables.pl
@@ -53,6 +53,25 @@ sub validate_guc_entry
string => [], # no extra required fields
);
+ # All fields recognized by the generator. "line_number" is injected
+ # by Catalog::ParseData and is not a user-facing field.
+ my %valid_fields = map { $_ => 1 } (
+ @required_common,
+ qw(long_desc flags ifdef min max options
+ check_hook assign_hook show_hook
+ line_number));
+
+ for my $f (sort keys %$entry)
+ {
+ unless ($valid_fields{$f})
+ {
+ die sprintf(
+ qq{%s:%d: error: entry "%s" has unrecognized field "%s"\n},
+ $input_fname, $entry->{line_number},
+ $entry->{name} // '<unknown>', $f);
+ }
+ }
+
for my $f (@required_common)
{
unless (defined $entry->{$f})
--
2.43.0
[application/octet-stream] 0002-gen_guc_tables-reject-type-inappropriate-fields-in-g.patch (2.6K, 3-0002-gen_guc_tables-reject-type-inappropriate-fields-in-g.patch)
download | inline diff:
From 33634107d72866cb85544f274a217ad592f61fe0 Mon Sep 17 00:00:00 2001
From: Zsolt Parragi <[email protected]>
Date: Sun, 15 Mar 2026 11:22:00 +0000
Subject: [PATCH 2/4] gen_guc_tables: reject type-inappropriate fields in
guc_parameters.dat
Detect when a GUC entry has fields that don't apply to its type, such
as "min" or "max" on a bool or string, or "options" on an int. These
would be silently ignored, likely indicating a copy-paste mistake.
The existing required_by_type structure is replaced with a single
type_specific_fields hash that drives both the required-field and
inappropriate-field checks.
---
src/backend/utils/misc/gen_guc_tables.pl | 32 ++++++++++++++++++------
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/misc/gen_guc_tables.pl b/src/backend/utils/misc/gen_guc_tables.pl
index ba2e6f001f1..b1c1b7afc6c 100644
--- a/src/backend/utils/misc/gen_guc_tables.pl
+++ b/src/backend/utils/misc/gen_guc_tables.pl
@@ -45,12 +45,12 @@ sub validate_guc_entry
my @required_common =
qw(name type context group short_desc variable boot_val);
- 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
+ my %type_specific_fields = (
+ int => { map { $_ => 1 } qw(min max) },
+ real => { map { $_ => 1 } qw(min max) },
+ enum => { map { $_ => 1 } qw(options) },
+ bool => {},
+ string => {},
);
# All fields recognized by the generator. "line_number" is injected
@@ -83,7 +83,7 @@ sub validate_guc_entry
}
}
- unless (exists $required_by_type{ $entry->{type} })
+ unless (exists $type_specific_fields{ $entry->{type} })
{
die sprintf(
qq{%s:%d: error: entry "%s" has unrecognized GUC type "%s"\n},
@@ -91,7 +91,9 @@ sub validate_guc_entry
$entry->{name}, $entry->{type} // '<unknown>');
}
- for my $f (@{ $required_by_type{ $entry->{type} } })
+ my $fields_for_type = $type_specific_fields{ $entry->{type} };
+
+ for my $f (sort keys %$fields_for_type)
{
unless (defined $entry->{$f})
{
@@ -101,6 +103,20 @@ sub validate_guc_entry
$entry->{type}, $f);
}
}
+
+ my %all_type_specific;
+ $all_type_specific{$_} = 1
+ for map { keys %$_ } values %type_specific_fields;
+ for my $f (sort keys %$entry)
+ {
+ if ($all_type_specific{$f} && !$fields_for_type->{$f})
+ {
+ die sprintf(
+ qq{%s:%d: error: entry "%s" of type "%s" must not have field "%s"\n},
+ $input_fname, $entry->{line_number}, $entry->{name},
+ $entry->{type}, $f);
+ }
+ }
}
# Print GUC table.
--
2.43.0
[application/octet-stream] 0003-gen_guc_tables-report-duplicate-entry-names-distinct.patch (1.5K, 4-0003-gen_guc_tables-report-duplicate-entry-names-distinct.patch)
download | inline diff:
From 163c196f4043b97a27ff79b7d77cc9954f35a63c Mon Sep 17 00:00:00 2001
From: Zsolt Parragi <[email protected]>
Date: Sun, 15 Mar 2026 11:22:15 +0000
Subject: [PATCH 3/4] gen_guc_tables: report duplicate entry names distinctly
The existing alphabetical-order check catches duplicates, but reports
them as "not in alphabetical order" which is confusing. Split the
check to give a clear "duplicate entry" message, and also include the
file name and line number in the ordering error.
---
src/backend/utils/misc/gen_guc_tables.pl | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/misc/gen_guc_tables.pl b/src/backend/utils/misc/gen_guc_tables.pl
index b1c1b7afc6c..9b6e3ed6622 100644
--- a/src/backend/utils/misc/gen_guc_tables.pl
+++ b/src/backend/utils/misc/gen_guc_tables.pl
@@ -133,10 +133,17 @@ sub print_table
{
validate_guc_entry($entry);
- if (defined($prev_name) && lc($prev_name) ge lc($entry->{name}))
+ if (defined($prev_name) && lc($prev_name) eq lc($entry->{name}))
{
die sprintf(
- "entries are not in alphabetical order: \"%s\", \"%s\"\n",
+ qq{%s:%d: error: duplicate entry "%s"\n},
+ $input_fname, $entry->{line_number}, $entry->{name});
+ }
+ if (defined($prev_name) && lc($prev_name) gt lc($entry->{name}))
+ {
+ die sprintf(
+ qq{%s:%d: error: entries are not in alphabetical order: "%s", "%s"\n},
+ $input_fname, $entry->{line_number},
$prev_name, $entry->{name});
}
--
2.43.0
[application/octet-stream] 0004-gen_guc_tables-validate-GUC-name-format.patch (1.1K, 5-0004-gen_guc_tables-validate-GUC-name-format.patch)
download | inline diff:
From 9f92321ce09309de1b25b04567195045235d5134 Mon Sep 17 00:00:00 2001
From: Zsolt Parragi <[email protected]>
Date: Sun, 15 Mar 2026 11:22:34 +0000
Subject: [PATCH 4/4] gen_guc_tables: validate GUC name format
Verify that each entry's name consists of letters, digits, and
underscores, starting with a letter. This catches accidental spaces
or special characters that would produce invalid C code.
---
src/backend/utils/misc/gen_guc_tables.pl | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/backend/utils/misc/gen_guc_tables.pl b/src/backend/utils/misc/gen_guc_tables.pl
index 9b6e3ed6622..2e8a3391046 100644
--- a/src/backend/utils/misc/gen_guc_tables.pl
+++ b/src/backend/utils/misc/gen_guc_tables.pl
@@ -83,6 +83,13 @@ sub validate_guc_entry
}
}
+ unless ($entry->{name} =~ /^[a-zA-Z][a-zA-Z0-9_]*$/)
+ {
+ die sprintf(
+ qq{%s:%d: error: entry name "%s" is not a valid GUC name (must start with a letter, contain only letters, digits, and underscores)\n},
+ $input_fname, $entry->{line_number}, $entry->{name});
+ }
+
unless (exists $type_specific_fields{ $entry->{type} })
{
die sprintf(
--
2.43.0
view thread (4+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: gen_guc_tables improvements
In-Reply-To: <CAN4CZFP=3xUoXb9jpn5OWwicg+rbyrca8-tVmgJsQAa4+OExkw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox