Re: gen_guc_tables.pl: Validate required GUC fields before code generation

Chao Li <li(dot)evan(dot)chao(at)gmail(dot)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