Re: assert pg_class.relnatts is consistent
On Mon, Feb 17, 2020 at 01:25:05PM +0900, Amit Langote wrote: > > Pushed both of those. > > Thank you. > > It's amazing to see how simple bootstrapping has now become thanks to > the work you guys have done recently. On Fri, Feb 14, 2020 at 06:00:05PM +0900, Amit Langote wrote: > > I can't write Perl myself (maybe Justin), but +1 to this idea. > > I tried and think it works but not sure if that's good Perl > programming. See the attached. And thanks for picking up perl so I didn't have to remember what I ever knew. -- Justin
Re: assert pg_class.relnatts is consistent
On Sun, Feb 16, 2020 at 5:25 AM Tom Lane wrote: > I wrote: > > So that leads me to the attached. > > ... > > (I agree with Alvaro's thought of shortening AddDefaultValues, > > but didn't do that here.) > > Pushed both of those. Thank you. It's amazing to see how simple bootstrapping has now become thanks to the work you guys have done recently. Thanks, Amit
Re: assert pg_class.relnatts is consistent
I wrote: > So that leads me to the attached. > ... > (I agree with Alvaro's thought of shortening AddDefaultValues, > but didn't do that here.) Pushed both of those. I also did something with the stale comment that Justin referred to in the initial message (it wasn't really good practice to try to deal with both things in one thread). I think we're done here, though maybe the difficulty of finding a clean way to get genbki.pl to do this suggests that AddDefaultValues needs to be redesigned. Not sure what that'd look like. regards, tom lane
Re: assert pg_class.relnatts is consistent
Alvaro Herrera writes:
> On 2020-Feb-14, John Naylor wrote:
>> One possible objection to what I wrote above is that it adds a
>> different kind of special case, but in a sneaky way. Perhaps it would
>> be more principled to treat it the same as oid after all. If we do
>> that, it would help to add a comment that we can't treat relnatts like
>> pronangs, since we need more information than what's in each pg_class
>> row.
> How about something like this? (untested)
I think John's idea of setting a dummy BKI_DEFAULT value is better,
as that means the only code that has to worry about this directly
is the code that's actually filling in relnatts. As far as said
code goes, we don't need an additional global variable when we can
just look in the $catalogs data structure; and I'm not a fan of
cramming this into the OID-assignment logic just to save a loop.
So that leads me to the attached.
(I agree with Alvaro's thought of shortening AddDefaultValues,
but didn't do that here.)
regards, tom lane
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 8032512..ad24f4d 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -184,6 +184,15 @@ my $PG_CATALOG_NAMESPACE =
'PG_CATALOG_NAMESPACE');
+# Fill in pg_class.relnatts by looking at the referenced catalog's schema.
+# This is ugly but there's no better place; Catalog::AddDefaultValues
+# can't do it, for lack of easy access to the other catalog.
+foreach my $row (@{ $catalog_data{pg_class} })
+{
+ $row->{relnatts} = scalar(@{ $catalogs{ $row->{relname} }->{columns} });
+}
+
+
# Build lookup tables.
# access method OID lookup
diff --git a/src/include/catalog/pg_class.dat b/src/include/catalog/pg_class.dat
index f70d5ba..b28bba7 100644
--- a/src/include/catalog/pg_class.dat
+++ b/src/include/catalog/pg_class.dat
@@ -14,51 +14,15 @@
# Note: only bootstrap catalogs, ie those marked BKI_BOOTSTRAP, need to
# have entries here. Be sure that the OIDs listed here match those given in
-# their CATALOG and BKI_ROWTYPE_OID macros, and that the relnatts values are
-# correct.
-
-# Note: "3" in the relfrozenxid column stands for FirstNormalTransactionId;
-# similarly, "1" in relminmxid stands for FirstMultiXactId
+# their CATALOG and BKI_ROWTYPE_OID macros.
{ oid => '1247',
- relname => 'pg_type', reltype => 'pg_type', relam => 'heap',
- relfilenode => '0', relpages => '0', reltuples => '0', relallvisible => '0',
- reltoastrelid => '0', relhasindex => 'f', relisshared => 'f',
- relpersistence => 'p', relkind => 'r', relnatts => '31', relchecks => '0',
- relhasrules => 'f', relhastriggers => 'f', relhassubclass => 'f',
- relrowsecurity => 'f', relforcerowsecurity => 'f', relispopulated => 't',
- relreplident => 'n', relispartition => 'f', relfrozenxid => '3',
- relminmxid => '1', relacl => '_null_', reloptions => '_null_',
- relpartbound => '_null_' },
+ relname => 'pg_type', reltype => 'pg_type' },
{ oid => '1249',
- relname => 'pg_attribute', reltype => 'pg_attribute', relam => 'heap',
- relfilenode => '0', relpages => '0', reltuples => '0', relallvisible => '0',
- reltoastrelid => '0', relhasindex => 'f', relisshared => 'f',
- relpersistence => 'p', relkind => 'r', relnatts => '25', relchecks => '0',
- relhasrules => 'f', relhastriggers => 'f', relhassubclass => 'f',
- relrowsecurity => 'f', relforcerowsecurity => 'f', relispopulated => 't',
- relreplident => 'n', relispartition => 'f', relfrozenxid => '3',
- relminmxid => '1', relacl => '_null_', reloptions => '_null_',
- relpartbound => '_null_' },
+ relname => 'pg_attribute', reltype => 'pg_attribute' },
{ oid => '1255',
- relname => 'pg_proc', reltype => 'pg_proc', relam => 'heap',
- relfilenode => '0', relpages => '0', reltuples => '0', relallvisible => '0',
- reltoastrelid => '0', relhasindex => 'f', relisshared => 'f',
- relpersistence => 'p', relkind => 'r', relnatts => '29', relchecks => '0',
- relhasrules => 'f', relhastriggers => 'f', relhassubclass => 'f',
- relrowsecurity => 'f', relforcerowsecurity => 'f', relispopulated => 't',
- relreplident => 'n', relispartition => 'f', relfrozenxid => '3',
- relminmxid => '1', relacl => '_null_', reloptions => '_null_',
- relpartbound => '_null_' },
+ relname => 'pg_proc', reltype => 'pg_proc' },
{ oid => '1259',
- relname => 'pg_class', reltype => 'pg_class', relam => 'heap',
- relfilenode => '0', relpages => '0', reltuples => '0', relallvisible => '0',
- reltoastrelid => '0', relhasindex => 'f', relisshared => 'f',
- relpersistence => 'p', relkind => 'r', relnatts => '33', relchecks => '0',
- relhasrules => 'f', relhastriggers => 'f', relhassubclass => 'f',
- relrowsecurity => 'f', relforcerowsecurity => 'f', relispopulated => 't',
- relreplident => 'n', relispartition => 'f', relfrozenxid => '3',
- relminmxid => '1', relacl => '_null_', reloptions => '_null_',
- relpartbound => '_null_' },
+ relname => 'pg_class', reltype =>
Re: assert pg_class.relnatts is consistent
On 2020-Feb-14, John Naylor wrote:
> One possible objection to what I wrote above is that it adds a
> different kind of special case, but in a sneaky way. Perhaps it would
> be more principled to treat it the same as oid after all. If we do
> that, it would help to add a comment that we can't treat relnatts like
> pronangs, since we need more information than what's in each pg_class
> row.
How about something like this? (untested)
# oids are a special case; ignore
next if $attname eq 'oid';
# pg_class.relnatts is computed from pg_attribute rows; ignore
next if $catname eq 'pg_class' and $attname eq 'relnatts';
# Raise error unless a value exists.
die "strip_default_values: $catname.$attname undefined\n"
if !defined $row->{$attname};
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: assert pg_class.relnatts is consistent
I propose this more concise coding for AddDefaultValues,
# Now fill in defaults, and note any columns that remain undefined.
foreach my $column (@$schema)
{
my $attname = $column->{name};
my $atttype = $column->{type};
# Skip if a value already exists
next if defined $row->{$attname};
# 'oid' and 'relnatts' are special cases. Ignore.
next if $attname eq 'oid';
next if $attname eq 'relnatts';
# This column has a default value. Fill it in.
if (defined $column->{default})
{
$row->{$attname} = $column->{default};
next;
}
# Failed to find a value.
push @missing_fields, $attname;
}
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: assert pg_class.relnatts is consistent
Hi John,
On Fri, Feb 14, 2020 at 6:50 PM John Naylor wrote:
> On Fri, Feb 14, 2020 at 5:00 PM Amit Langote wrote:
> > I tried and think it works but not sure if that's good Perl
> > programming. See the attached.
>
> Hi Amit,
> I took this for a spin -- I just have a couple comments.
Thanks for chiming in.
> + elsif ($attname eq 'relnatts')
> + {
> + ;
> + }
>
> With your patch, I get this when running
> src/include/catalog/reformat_dat_file.pl:
>
> strip_default_values: pg_class.relnatts undefined
I think I have fixed this in the attached.
> + if ($catname eq "pg_class" && $attname eq "relnatts")
> + {
> + $bki_values{$attname} = $catalog_ncols{$bki_values{relname}};
> + }
> +
>
> You could avoid the name/attr checks if you do it while building the
> pg_class lookup table, like this:
>
> foreach my $row (@{ $catalog_data{pg_class} })
> {
> $classoids{ $row->{relname} } = $row->{oid};
> +
> + # Also fill in correct value for relnatts.
> + $row->{relnatts} = $catalog_ncols{ $row->{relname} };
> }
Did this too. Attached updated patch, which also addresses Michael's comment.
I'm still trying to understand your comment about using placeholder
BKI_DEFAULT...
Thanks,
Amit
v2-0001-Don-t-require-relnatts-to-be-specified-in-pg_clas.patch
Description: Binary data
Re: assert pg_class.relnatts is consistent
On Fri, Feb 14, 2020 at 6:47 PM Michael Paquier wrote: > On Fri, Feb 14, 2020 at 06:00:05PM +0900, Amit Langote wrote: > > On Fri, Feb 14, 2020 at 2:58 PM Amit Langote > > wrote: > > > On Fri, Feb 14, 2020 at 1:04 AM Tom Lane wrote: > > > > I've been burnt by this too :-(. However, I think this patch is > > > > completely the wrong way to go about improving this. What we should > > > > be doing, now that we have all that perl code generating postgres.bki, > > > > is eliminating the problem at the source. That is, drop the hand-coded > > > > relnatts values from pg_class.dat altogether, and let the perl code fill > > > > it in --- compare the handling of pg_proc.pronargs for instance. > > > > > > I can't write Perl myself (maybe Justin), but +1 to this idea. > > > > I tried and think it works but not sure if that's good Perl > > programming. See the attached. > > I quite like what you have here. Please note that this comment in > genbki.pl is incorrect regarding relnatts (the last part could just be > deleted): > # Note: only bootstrap catalogs, ie those marked BKI_BOOTSTRAP, need to > # have entries here. Be sure that the OIDs listed here match those given in > # their CATALOG and BKI_ROWTYPE_OID macros, and that the relnatts values are > # correct. You're right, although this comment is in pg_class.dat. Thanks, Amit
Re: assert pg_class.relnatts is consistent
> pronangs, since we need more information than what's in each pg_class Sigh, and of course I met pg_proc.pronargs. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: assert pg_class.relnatts is consistent
I wrote:
> + elsif ($attname eq 'relnatts')
> + {
> + ;
> + }
>
> With your patch, I get this when running
> src/include/catalog/reformat_dat_file.pl:
>
> strip_default_values: pg_class.relnatts undefined
>
> Rather than adding this one-off case to AddDefaultValues and then
> another special case to strip_default_values, maybe it would be better
> to just add a placeholder BKI_DEFAULT(0) to pg_class.h, with a comment
> that it's just a placeholder.
One possible objection to what I wrote above is that it adds a
different kind of special case, but in a sneaky way. Perhaps it would
be more principled to treat it the same as oid after all. If we do
that, it would help to add a comment that we can't treat relnatts like
pronangs, since we need more information than what's in each pg_class
row.
--
John Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: assert pg_class.relnatts is consistent
On Fri, Feb 14, 2020 at 5:00 PM Amit Langote wrote:
> I tried and think it works but not sure if that's good Perl
> programming. See the attached.
Hi Amit,
I took this for a spin -- I just have a couple comments.
+ elsif ($attname eq 'relnatts')
+ {
+ ;
+ }
With your patch, I get this when running
src/include/catalog/reformat_dat_file.pl:
strip_default_values: pg_class.relnatts undefined
Rather than adding this one-off case to AddDefaultValues and then
another special case to strip_default_values, maybe it would be better
to just add a placeholder BKI_DEFAULT(0) to pg_class.h, with a comment
that it's just a placeholder.
+ if ($catname eq "pg_class" && $attname eq "relnatts")
+ {
+ $bki_values{$attname} = $catalog_ncols{$bki_values{relname}};
+ }
+
You could avoid the name/attr checks if you do it while building the
pg_class lookup table, like this:
foreach my $row (@{ $catalog_data{pg_class} })
{
$classoids{ $row->{relname} } = $row->{oid};
+
+ # Also fill in correct value for relnatts.
+ $row->{relnatts} = $catalog_ncols{ $row->{relname} };
}
--
John Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: assert pg_class.relnatts is consistent
On Fri, Feb 14, 2020 at 06:00:05PM +0900, Amit Langote wrote: > On Fri, Feb 14, 2020 at 2:58 PM Amit Langote wrote: > > On Fri, Feb 14, 2020 at 1:04 AM Tom Lane wrote: > > > I've been burnt by this too :-(. However, I think this patch is > > > completely the wrong way to go about improving this. What we should > > > be doing, now that we have all that perl code generating postgres.bki, > > > is eliminating the problem at the source. That is, drop the hand-coded > > > relnatts values from pg_class.dat altogether, and let the perl code fill > > > it in --- compare the handling of pg_proc.pronargs for instance. > > > > I can't write Perl myself (maybe Justin), but +1 to this idea. > > I tried and think it works but not sure if that's good Perl > programming. See the attached. I quite like what you have here. Please note that this comment in genbki.pl is incorrect regarding relnatts (the last part could just be deleted): # Note: only bootstrap catalogs, ie those marked BKI_BOOTSTRAP, need to # have entries here. Be sure that the OIDs listed here match those given in # their CATALOG and BKI_ROWTYPE_OID macros, and that the relnatts values are # correct. -- Michael signature.asc Description: PGP signature
Re: assert pg_class.relnatts is consistent
On Fri, Feb 14, 2020 at 2:58 PM Amit Langote wrote:
> On Fri, Feb 14, 2020 at 1:04 AM Tom Lane wrote:
> > I've been burnt by this too :-(. However, I think this patch is
> > completely the wrong way to go about improving this. What we should
> > be doing, now that we have all that perl code generating postgres.bki,
> > is eliminating the problem at the source. That is, drop the hand-coded
> > relnatts values from pg_class.dat altogether, and let the perl code fill
> > it in --- compare the handling of pg_proc.pronargs for instance.
>
> I can't write Perl myself (maybe Justin), but +1 to this idea.
I tried and think it works but not sure if that's good Perl
programming. See the attached.
Thanks,
Amit
From ec690b6e78176354ae033073cda1b0770e956a72 Mon Sep 17 00:00:00 2001
From: Amit Langote
Date: Fri, 14 Feb 2020 16:04:48 +0900
Subject: [PATCH] Don't require relnatts to be specified in pg_class.dat
---
src/backend/catalog/Catalog.pm | 4
src/backend/catalog/genbki.pl| 7 +++
src/include/catalog/pg_class.dat | 11 +++
3 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index c089b1d71d..8e84bf9d2d 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -341,6 +341,10 @@ sub AddDefaultValues
{
;
}
+ elsif ($attname eq 'relnatts')
+ {
+ ;
+ }
elsif (defined $column->{default})
{
$row->{$attname} = $column->{default};
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 803251207b..eac4542ade 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -56,6 +56,7 @@ my %catalog_data;
my @toast_decls;
my @index_decls;
my %oidcounts;
+my %catalog_ncols;
foreach my $header (@ARGV)
{
@@ -71,6 +72,7 @@ foreach my $header (@ARGV)
{
push @catnames, $catname;
$catalogs{$catname} = $catalog;
+ $catalog_ncols{$catname} = scalar(@$schema);
}
# While checking for duplicated OIDs, we ignore the pg_class OID and
@@ -524,6 +526,11 @@ EOM
my $attname = $column->{name};
my $atttype = $column->{type};
+ if ($catname eq "pg_class" && $attname eq "relnatts")
+ {
+ $bki_values{$attname} =
$catalog_ncols{$bki_values{relname}};
+ }
+
# Assign oid if oid column exists and no explicit
assignment in row
if ($attname eq "oid" and not defined
$bki_values{$attname})
{
diff --git a/src/include/catalog/pg_class.dat b/src/include/catalog/pg_class.dat
index f70d5bacb9..d901988858 100644
--- a/src/include/catalog/pg_class.dat
+++ b/src/include/catalog/pg_class.dat
@@ -20,11 +20,14 @@
# Note: "3" in the relfrozenxid column stands for FirstNormalTransactionId;
# similarly, "1" in relminmxid stands for FirstMultiXactId
+# Note: relnatts is computed automatically by genbki.pl, so need not be
+# specified here.
+
{ oid => '1247',
relname => 'pg_type', reltype => 'pg_type', relam => 'heap',
relfilenode => '0', relpages => '0', reltuples => '0', relallvisible => '0',
reltoastrelid => '0', relhasindex => 'f', relisshared => 'f',
- relpersistence => 'p', relkind => 'r', relnatts => '31', relchecks => '0',
+ relpersistence => 'p', relkind => 'r', relchecks => '0',
relhasrules => 'f', relhastriggers => 'f', relhassubclass => 'f',
relrowsecurity => 'f', relforcerowsecurity => 'f', relispopulated => 't',
relreplident => 'n', relispartition => 'f', relfrozenxid => '3',
@@ -34,7 +37,7 @@
relname => 'pg_attribute', reltype => 'pg_attribute', relam => 'heap',
relfilenode => '0', relpages => '0', reltuples => '0', relallvisible => '0',
reltoastrelid => '0', relhasindex => 'f', relisshared => 'f',
- relpersistence => 'p', relkind => 'r', relnatts => '25', relchecks => '0',
+ relpersistence => 'p', relkind => 'r', relchecks => '0',
relhasrules => 'f', relhastriggers => 'f', relhassubclass => 'f',
relrowsecurity => 'f', relforcerowsecurity => 'f', relispopulated => 't',
relreplident => 'n', relispartition => 'f', relfrozenxid => '3',
@@ -44,7 +47,7 @@
relname => 'pg_proc', reltype => 'pg_proc', relam => 'heap',
relfilenode => '0', relpages => '0', reltuples => '0', relallvisible => '0',
reltoastrelid => '0', relhasindex => 'f', relisshared => 'f',
- relpersistence => 'p', relkind => 'r', relnatts => '29', relchecks => '0',
+ relpersistence => 'p', relkind => 'r', relchecks => '0',
relhasrules => 'f', relhastriggers => 'f', relhassubclass => 'f',
relrowsecurity => 'f', relforcerowsecurity => 'f', relispopulated => 't',
relreplident => 'n',
Re: assert pg_class.relnatts is consistent
On Fri, Feb 14, 2020 at 1:04 AM Tom Lane wrote: > > Amit Langote writes: > > On Thu, Feb 13, 2020 at 4:51 PM Amit Langote > > wrote: > >> I know I have made the mistake of not updating relnatts when I added > >> relispartition, etc. to pg_class, only to be bitten by it in the form > >> of seemingly random errors/crashes. Is that why? > > > Sorry for not having read the patch properly. > >> + /* Check that pg_class data is consistent now, rather than failing > >> obscurely later */ > > That seems to be it. > > I've been burnt by this too :-(. However, I think this patch is > completely the wrong way to go about improving this. What we should > be doing, now that we have all that perl code generating postgres.bki, > is eliminating the problem at the source. That is, drop the hand-coded > relnatts values from pg_class.dat altogether, and let the perl code fill > it in --- compare the handling of pg_proc.pronargs for instance. I can't write Perl myself (maybe Justin), but +1 to this idea. Thanks, Amit
Re: assert pg_class.relnatts is consistent
Amit Langote writes: > On Thu, Feb 13, 2020 at 4:51 PM Amit Langote wrote: >> I know I have made the mistake of not updating relnatts when I added >> relispartition, etc. to pg_class, only to be bitten by it in the form >> of seemingly random errors/crashes. Is that why? > Sorry for not having read the patch properly. >> + /* Check that pg_class data is consistent now, rather than failing >> obscurely later */ > That seems to be it. I've been burnt by this too :-(. However, I think this patch is completely the wrong way to go about improving this. What we should be doing, now that we have all that perl code generating postgres.bki, is eliminating the problem at the source. That is, drop the hand-coded relnatts values from pg_class.dat altogether, and let the perl code fill it in --- compare the handling of pg_proc.pronargs for instance. (While we're at it, an awful lot of the bulk of pg_class.dat could be replaced by BKI_DEFAULT() entries in pg_class.h, though I'm less sure that that's much of an improvement. I think we intentionally didn't bother when we put in the BKI_DEFAULT support, reasoning that there were too few pg_class.dat entries to bother.) regards, tom lane
Re: assert pg_class.relnatts is consistent
On Thu, Feb 13, 2020 at 4:51 PM Amit Langote wrote: > On Thu, Feb 13, 2020 at 3:23 AM Justin Pryzby wrote: > > Forking this thread for two tangential patches which I think are more > > worthwhile than the original topic's patch. > > https://www.postgresql.org/message-id/20200207143935.GP403%40telsasoft.com > > > > Is there a better place to implement assertion from 0002 ? > > I would think the answer to that would be related to the answer of why > you think we need this assert in the first place? > > I know I have made the mistake of not updating relnatts when I added > relispartition, etc. to pg_class, only to be bitten by it in the form > of seemingly random errors/crashes. Is that why? Sorry for not having read the patch properly. > + /* Check that pg_class data is consistent now, rather than failing > obscurely later */ That seems to be it. Thanks, Amit
Re: assert pg_class.relnatts is consistent
On Thu, Feb 13, 2020 at 04:51:01PM +0900, Amit Langote wrote: > On Thu, Feb 13, 2020 at 3:23 AM Justin Pryzby wrote: > > Forking this thread for two tangential patches which I think are more > > worthwhile than the original topic's patch. > > https://www.postgresql.org/message-id/20200207143935.GP403%40telsasoft.com > > > > Is there a better place to implement assertion from 0002 ? > > I would think the answer to that would be related to the answer of why > you think we need this assert in the first place? > > I know I have made the mistake of not updating relnatts when I added > relispartition, etc. to pg_class, only to be bitten by it in the form > of seemingly random errors/crashes. Is that why? Right. If adding or removing a column from pg_class (or others) it's necessary not only to add the column in the .h file, and update references like Anum_*, but also to update that catalog's own pg_class.relnatts in pg_class.dat. On the other thead, Alvaro agreed it might be worth experimenting with moving "indisclustered" from boolean in pg_index to an Oid in pg_class. There's not many references to it, so I was able to make most of the necessary changes within an hour .. but spent some multiple of that tracing the crash in initdb, which I would prefer to have failed less obscurely. -- Justin
Re: assert pg_class.relnatts is consistent
On Thu, Feb 13, 2020 at 04:51:01PM +0900, Amit Langote wrote: > I would think the answer to that would be related to the answer of why > you think we need this assert in the first place? Taking this thread independently, and even after reading the thread mentioned upthread, I still don't quite understand why this change could be a good thing and in which cases it actually helps. The code includes no comments and the commit log says nothing either, so it is hard to follow what you are thinking here even if you are splitting the effort across multiple thread. Please note that the style of the code is not project-like, so you should try to indent it. And why does it matter to check this portion of the catalogs? Also, such checks are not really needed in non-assert builds, if actually needed. -- Michael signature.asc Description: PGP signature
Re: assert pg_class.relnatts is consistent
On Thu, Feb 13, 2020 at 3:23 AM Justin Pryzby wrote: > Forking this thread for two tangential patches which I think are more > worthwhile than the original topic's patch. > https://www.postgresql.org/message-id/20200207143935.GP403%40telsasoft.com > > Is there a better place to implement assertion from 0002 ? I would think the answer to that would be related to the answer of why you think we need this assert in the first place? I know I have made the mistake of not updating relnatts when I added relispartition, etc. to pg_class, only to be bitten by it in the form of seemingly random errors/crashes. Is that why? Thanks, Amit
assert pg_class.relnatts is consistent
Forking this thread for two tangential patches which I think are more
worthwhile than the original topic's patch.
https://www.postgresql.org/message-id/20200207143935.GP403%40telsasoft.com
Is there a better place to implement assertion from 0002 ?
On Fri, Feb 07, 2020 at 08:39:35AM -0600, Justin Pryzby wrote:
> From 7eea0a17e495fe13379ffd589b551f2f145f5672 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby
> Date: Thu, 6 Feb 2020 21:48:13 -0600
> Subject: [PATCH v1 1/3] Update comment obsolete since b9b8831a
>
> ---
> src/backend/commands/cluster.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
> index e9d7a7f..3adcbeb 100644
> --- a/src/backend/commands/cluster.c
> +++ b/src/backend/commands/cluster.c
> @@ -1539,9 +1539,9 @@ get_tables_to_cluster(MemoryContext cluster_context)
>
> /*
>* Get all indexes that have indisclustered set and are owned by
> - * appropriate user. System relations or nailed-in relations cannot ever
> - * have indisclustered set, because CLUSTER will refuse to set it when
> - * called with one of them as argument.
> + * appropriate user. Shared relations cannot ever have indisclustered
> + * set, because CLUSTER will refuse to set it when called with one as
> + * an argument.
>*/
> indRelation = table_open(IndexRelationId, AccessShareLock);
> ScanKeyInit(,
> --
> 2.7.4
>
> From 4777be522a7aa8b8c77b13f765cbd02043438f2a Mon Sep 17 00:00:00 2001
> From: Justin Pryzby
> Date: Fri, 7 Feb 2020 08:12:50 -0600
> Subject: [PATCH v1 2/3] Give developer a helpful kick in the pants if they
> change natts in one place but not another
>
> ---
> src/backend/bootstrap/bootstrap.c | 23 +++
> 1 file changed, 23 insertions(+)
>
> diff --git a/src/backend/bootstrap/bootstrap.c
> b/src/backend/bootstrap/bootstrap.c
> index bfc629c..d5e1888 100644
> --- a/src/backend/bootstrap/bootstrap.c
> +++ b/src/backend/bootstrap/bootstrap.c
> @@ -25,7 +25,9 @@
> #include "access/xlog_internal.h"
> #include "bootstrap/bootstrap.h"
> #include "catalog/index.h"
> +#include "catalog/pg_class.h"
> #include "catalog/pg_collation.h"
> +#include "catalog/pg_proc.h"
> #include "catalog/pg_type.h"
> #include "common/link-canary.h"
> #include "libpq/pqsignal.h"
> @@ -49,6 +51,7 @@
> #include "utils/ps_status.h"
> #include "utils/rel.h"
> #include "utils/relmapper.h"
> +#include "utils/syscache.h"
>
> uint32 bootstrap_data_checksum_version = 0;/* No checksum
> */
>
> @@ -602,6 +605,26 @@ boot_openrel(char *relname)
> TableScanDesc scan;
> HeapTuple tup;
>
> + /* Check that pg_class data is consistent now, rather than failing
> obscurely later */
> + struct { Oid oid; int natts; }
> + checknatts[] = {
> + {RelationRelationId, Natts_pg_class,},
> + {TypeRelationId, Natts_pg_type,},
> + {AttributeRelationId, Natts_pg_attribute,},
> + {ProcedureRelationId, Natts_pg_proc,},
> + };
> +
> + for (int i=0; i + Form_pg_class classForm;
> + HeapTuple tuple;
> + tuple = SearchSysCache1(RELOID,
> ObjectIdGetDatum(checknatts[i].oid));
> + if (!HeapTupleIsValid(tuple))
> + elog(ERROR, "cache lookup failed for relation %u",
> checknatts[i].oid);
> + classForm = (Form_pg_class) GETSTRUCT(tuple);
> + Assert(checknatts[i].natts == classForm->relnatts);
> + ReleaseSysCache(tuple);
> + }
> +
> if (strlen(relname) >= NAMEDATALEN)
> relname[NAMEDATALEN - 1] = '\0';
>
> --
> 2.7.4
>
>From 4777be522a7aa8b8c77b13f765cbd02043438f2a Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 7 Feb 2020 08:12:50 -0600
Subject: [PATCH v1 2/3] Give developer a helpful kick in the pants if they
change natts in one place but not another
---
src/backend/bootstrap/bootstrap.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index bfc629c..d5e1888 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -25,7 +25,9 @@
#include "access/xlog_internal.h"
#include "bootstrap/bootstrap.h"
#include "catalog/index.h"
+#include "catalog/pg_class.h"
#include "catalog/pg_collation.h"
+#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
#include "common/link-canary.h"
#include "libpq/pqsignal.h"
@@ -49,6 +51,7 @@
#include "utils/ps_status.h"
#include "utils/rel.h"
#include "utils/relmapper.h"
+#include "utils/syscache.h"
uint32 bootstrap_data_checksum_version = 0; /* No checksum */
@@ -602,6 +605,26 @@ boot_openrel(char *relname)
TableScanDesc scan;
HeapTuple tup;
+ /* Check that pg_class data is consistent now, rather than failing obscurely later */
+
