Re: [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line

Hi David,

Here's a review of your patch.

David Christensen <david(at)endpoint(dot)com> writes:

> Throws a build error if we encounter a different number of fields in a
> DATA() line than we expect for the catalog in question.

The patch is a good idea, and as-is implements the suggested feature.
Tested by removing an attribute from a line in catalog/pg_proc.h and
observing the expected failure. With the attribute in place, it builds and
passes make check-world.

Detailed code review:

[…]
> diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
[…]
> + check_natts($filename, $catalog{natts},$3) or
> + die sprintf "Wrong number of Natts in DATA() line %s:%d\n", $input_file,INPUT_FILE->input_line_number;

Including the expected/actual number of attributes in the error message
would be nice. In fact, the 'die' could be moved into check_natts(),
where the actual number is already available, and would clutter the main
loop less.

> + unless ($natts)
> + {
> + die "Could not find definition for Natts_${catname} before start of DATA()\n";
> + }

More idiomatically written as:

die ....
unless $natts;

> diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl

The changes to this file are redundant, since it calls Catalog::Catalogs(),
which already checks that the number of attributes matches.

Attached is a suggested revised patch.

- ilmari

--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl