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@endpoint.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