Re: [HACKERS] [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line - Mailing list pgsql-hackers

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

From b78b281983e7a0406c15461340391c21d6cddef9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 6 Mar 2017 14:51:50 +0000
Subject: [PATCH] Teach Catalog.pm how many attributes there should be per
 DATA() line

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

Previously, it was possible to silently ignore any mismatches at build
time which could result in symbol undefined errors at link time.  Now
we stop and identify the infringing line as soon as we encounter it,
which greatly speeds up the debugging process.
---
 src/backend/catalog/Catalog.pm | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index e1f3c3a5ee..85a537ad95 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -46,6 +46,9 @@ sub Catalogs
 
         open(INPUT_FILE, '<', $input_file) || die "$input_file: $!";
 
+        my ($filename) = ($input_file =~ m/(\w+)\.h$/);
+        my $natts_pat = "Natts_$filename";
+
         # Scan the input file.
         while (<INPUT_FILE>)
         {
@@ -70,8 +73,15 @@ sub Catalogs
             s/\s+/ /g;
 
             # Push the data into the appropriate data structure.
-            if (/^DATA\(insert(\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
+            if (/$natts_pat\s+(\d+)/)
+            {
+                $catalog{natts} = $1;
+            }
+            elsif (/^DATA\(insert(\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
             {
+                check_natts($filename, $catalog{natts}, $3,
+                            $input_file, INPUT_FILE->input_line_number);
+
                 push @{ $catalog{data} }, { oid => $2, bki_values => $3 };
             }
             elsif (/^DESCR\(\"(.*)\"\)$/)
@@ -216,4 +226,20 @@ sub RenameTempFile
     rename($temp_name, $final_name) || die "rename: $temp_name: $!";
 }
 
+# verify the number of fields in the passed-in bki structure
+sub check_natts
+{
+    my ($catname, $natts, $bki_val, $file, $line) = @_;
+    die "Could not find definition for Natts_${catname} before start of DATA() in $file\n"
+        unless defined $natts;
+
+    # we're working with a copy and need to count the fields only, so collapse
+    $bki_val =~ s/"[^"]*?"/xxx/g;
+    my @atts = split /\s+/ => $bki_val;
+
+    die sprintf
+        "Wrong number of attributes in DATA() entry at %s:%d (expected %d but got %d)\n",
+        $file, $line, $natts, scalar @atts
+      unless $natts == @atts;
+}
 1;
-- 
2.11.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers