On Mon, Nov 10, 2025 at 8:02 PM Dagfinn Ilmari Mannsåker <[email protected]> wrote:
> Chao Li <[email protected]> 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
Description: Binary data
