Thread: [HACKERS] [PATCH] Teach Catalog.pm how many attributes there should be perDATA() line
[HACKERS] [PATCH] Teach Catalog.pm how many attributes there should be perDATA() line
From
David Christensen
Date:
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 | 26 +++++++++++++++++++++++++-src/backend/utils/Gen_fmgrtab.pl | 18 ++++++++++++++++--2files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm index e1f3c3a..86f5b59 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) or + die sprintf "Wrong number of Natts in DATA() line %s:%d\n", $input_file, INPUT_FILE->input_line_number; + push @{ $catalog{data} }, { oid => $2, bki_values => $3 }; } elsif (/^DESCR\(\"(.*)\"\)$/) @@ -216,4 +226,18 @@ 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) = @_; + unless ($natts) + { + die "Could not find definition for Natts_${catname} before start of DATA()\n"; + } + + # we're working with a copy and need to count the fields only, so collapse + $bki_val =~ s/"[^"]*?"/xxx/g; + + return (split /\s+/ => $bki_val) == $natts; +}1; diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl index cdd603a..49a5d80 100644 --- a/src/backend/utils/Gen_fmgrtab.pl +++ b/src/backend/utils/Gen_fmgrtab.pl @@ -56,9 +56,11 @@ foreach my $column (@{ $catalogs->{pg_proc}->{columns} })}my $data = $catalogs->{pg_proc}->{data}; +my $natts = $catalogs->{pg_proc}->{natts}; +my $elem = 0; +foreach my $row (@$data){ - # To construct fmgroids.h and fmgrtab.c, we need to inspect some # of the individual data fields. Just splittingon whitespace # won't work, because some quoted fields might contain internal @@ -67,7 +69,19 @@ foreach my $row (@$data) # fields that might need quoting, so this simple hack is # sufficient. $row->{bki_values} =~ s/"[^"]*"/"xxx"/g; - @{$row}{@attnames} = split /\s+/, $row->{bki_values}; + my @bki_values = split /\s+/, $row->{bki_values}; + + # verify we've got the expected number of data fields + if (@bki_values != $natts) + { + die sprintf <<EOF, $elem, $natts, scalar @bki_values; +Wrong number of attributes in pg_proc DATA() entry %d (expected %d but got %d) +EOF + } + $elem++; + + @{$row}{@attnames} = @bki_values; + # Select out just the rows for internal-language procedures. # Note assumption here that INTERNALlanguageId is 12. -- 2.8.4 (Apple Git-73)
Re: [HACKERS] [PATCH] Teach Catalog.pm how many attributes thereshould be per DATA() line
From
Peter Eisentraut
Date:
On 2/15/17 10:40, David Christensen wrote: > 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. I think this would be useful. I haven't reviewed the code, but the general idea looks reasonable. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
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
[HACKERS] Re: [PATCH] Teach Catalog.pm how many attributes there should be perDATA() line
From
David Christensen
Date:
> Hi David, > > Here's a review of your patch. Hi Ilmari, thanks for your time and review. I’m fine with the revised version. Best, David -- David Christensen End Point Corporation david@endpoint.com 785-727-1171
Re: [HACKERS] [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
David Christensen <david@endpoint.com> writes: >> Hi David, >> >> Here's a review of your patch. > > Hi Ilmari, thanks for your time and review. I’m fine with the revised version. Okay, I've marked the patch as Ready For Committer. Thanks, Ilmari -- "The surreality of the universe tends towards a maximum" -- Skud's Law "Never formulate a law or axiom that you're not prepared to live withthe consequences of." --Skud's Meta-Law
Re: [HACKERS] [PATCH] Teach Catalog.pm how many attributes thereshould be per DATA() line
From
Robert Haas
Date:
On Mon, Mar 6, 2017 at 11:37 AM, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote: > David Christensen <david@endpoint.com> writes: >>> Hi David, >>> >>> Here's a review of your patch. >> >> Hi Ilmari, thanks for your time and review. I’m fine with the revised version. > > Okay, I've marked the patch as Ready For Committer. Committed. Hopefully this doesn't contain any Perl bits that are sufficiently new as to cause problems for our older BF members ... I guess we'll see. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH] Teach Catalog.pm how many attributes thereshould be per DATA() line
From
Jeff Janes
Date:
On Thu, Mar 9, 2017 at 3:20 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Mar 6, 2017 at 11:37 AM, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> David Christensen <david@endpoint.com> writes:
>>> Hi David,
>>>
>>> Here's a review of your patch.
>>
>> Hi Ilmari, thanks for your time and review. I’m fine with the revised version.
>
> Okay, I've marked the patch as Ready For Committer.
Committed. Hopefully this doesn't contain any Perl bits that are
sufficiently new as to cause problems for our older BF members ... I
guess we'll see.
Bad luck there. I'm getting this error on CentOS6.8, perl v5.10.1
Can't locate object method "input_line_number" via package "IO::Handle" at ../../../src/backend/catalog/Catalog.pm line 82, <INPUT_FILE> line 148.
make[3]: *** [fmgrtab.c] Error 25
make[2]: *** [utils/fmgroids.h] Error 2
make[2]: *** Waiting for unfinished jobs....
Can't locate object method "input_line_number" via package "IO::Handle" at ../../../src/backend/catalog/Catalog.pm line 82, <INPUT_FILE> line 148.
make[3]: *** [postgres.bki] Error 25
make[2]: *** [submake-schemapg] Error 2
make[1]: *** [all-backend-recurse] Error 2
make: *** [all-src-recurse] Error 2
I think we can just save $. and use that, as in the attached.
I as sabotaged a random line in src/include/catalog/pg_amop.h and it seems to report the error correctly.
Cheers,
Jeff
Attachment
Re: [HACKERS] [PATCH] Teach Catalog.pm how many attributes thereshould be per DATA() line
From
Amit Langote
Date:
On 2017/03/10 9:14, Jeff Janes wrote: > On Thu, Mar 9, 2017 at 3:20 PM, Robert Haas <robertmhaas@gmail.com > <mailto:robertmhaas@gmail.com>> wrote: > > On Mon, Mar 6, 2017 at 11:37 AM, Dagfinn Ilmari Mannsåker > <ilmari@ilmari.org <mailto:ilmari@ilmari.org>> wrote: > > David Christensen <david@endpoint.com <mailto:david@endpoint.com>> writes: > >>> Hi David, > >>> > >>> Here's a review of your patch. > >> > >> Hi Ilmari, thanks for your time and review. I’m fine with the revised version. > > > > Okay, I've marked the patch as Ready For Committer. > > Committed. Hopefully this doesn't contain any Perl bits that are > sufficiently new as to cause problems for our older BF members ... I > guess we'll see. > > > > Bad luck there. I'm getting this error on CentOS6.8, perl v5.10.1 > > Can't locate object method "input_line_number" via package "IO::Handle" at > ../../../src/backend/catalog/Catalog.pm line 82, <INPUT_FILE> line 148. > make[3]: *** [fmgrtab.c] Error 25 > make[2]: *** [utils/fmgroids.h] Error 2 > make[2]: *** Waiting for unfinished jobs.... > Can't locate object method "input_line_number" via package "IO::Handle" at > ../../../src/backend/catalog/Catalog.pm line 82, <INPUT_FILE> line 148. > make[3]: *** [postgres.bki] Error 25 > make[2]: *** [submake-schemapg] Error 2 > make[1]: *** [all-backend-recurse] Error 2 > make: *** [all-src-recurse] Error 2 Same here. Some buildfarm animals failed too. https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&dt=2017-03-10%2000%3A55%3A42 > I think we can just save $. and use that, as in the attached. The patch works for me. Thanks, Amit
Re: [HACKERS] [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line
From
Tom Lane
Date:
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes: > On 2017/03/10 9:14, Jeff Janes wrote: >> I think we can just save $. and use that, as in the attached. > The patch works for me. Me too. Pushed; we'll soon see if that makes the oldest buildfarm critters happy. regards, tom lane
Re: [HACKERS] [PATCH] Teach Catalog.pm how many attributes thereshould be per DATA() line
From
Robert Haas
Date:
On Thu, Mar 9, 2017 at 8:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes: >> On 2017/03/10 9:14, Jeff Janes wrote: >>> I think we can just save $. and use that, as in the attached. > >> The patch works for me. > > Me too. Pushed; we'll soon see if that makes the oldest buildfarm > critters happy. $. has been around since the stone age (a.k.a. when I was a teenager) so we should be fine there. I hope. That whole input_line_number thing was making me a bit nervous, but I couldn't find any reference to when that had been added in a quick Google search, which gave me hope that it was old enough that we'd be OK. Sorry for the hassle. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH] Teach Catalog.pm how many attributes there should be per DATA() line
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
Jeff Janes <jeff.janes@gmail.com> writes: > On Thu, Mar 9, 2017 at 3:20 PM, Robert Haas <robertmhaas@gmail.com> wrote: > >> Committed. Hopefully this doesn't contain any Perl bits that are >> sufficiently new as to cause problems for our older BF members ... I >> guess we'll see. > > Bad luck there. I'm getting this error on CentOS6.8, perl v5.10.1 > > Can't locate object method "input_line_number" via package "IO::Handle" at [...] > I think we can just save $. and use that, as in the attached. Another option would have been to add an explicit 'use IO::Handle;', which makes the OO interface available on filehandles on all versions back to 5.8 (5.14 and newer do this automatically). - ilmari -- "I use RMS as a guide in the same way that a boat captain would usea lighthouse. It's good to know where it is, but yougenerallydon't want to find yourself in the same spot." - Tollef Fog Heen