From: | ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) |
---|---|
To: | David Christensen <david(at)endpoint(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line |
Date: | 2017-03-06 15:14:28 |
Message-ID: | d8jbmte4euj.fsf@dalvik.ping.uio.no |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
0001-Teach-Catalog.pm-how-many-attributes-there-should-be.patch | text/x-diff | 2.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Aleksander Alekseev | 2017-03-06 15:22:17 | Re: Declarative partitioning optimization for large amount of partitions |
Previous Message | Erik Rijkers | 2017-03-06 15:10:19 | Re: Logical replication existing data copy |