Thread: assert pg_class.relnatts is consistent

assert pg_class.relnatts is consistent

From
Justin Pryzby
Date:
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 <pryzbyj@telsasoft.com>
> 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(&entry,
> -- 
> 2.7.4
> 

> From 4777be522a7aa8b8c77b13f765cbd02043438f2a Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <pryzbyj@telsasoft.com>
> 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<lengthof(checknatts); ++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
>

Attachment

Re: assert pg_class.relnatts is consistent

From
Amit Langote
Date:
On Thu, Feb 13, 2020 at 3:23 AM Justin Pryzby <pryzby@telsasoft.com> 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



Re: assert pg_class.relnatts is consistent

From
Michael Paquier
Date:
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

Attachment

Re: assert pg_class.relnatts is consistent

From
Justin Pryzby
Date:
On Thu, Feb 13, 2020 at 04:51:01PM +0900, Amit Langote wrote:
> On Thu, Feb 13, 2020 at 3:23 AM Justin Pryzby <pryzby@telsasoft.com> 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

From
Amit Langote
Date:
On Thu, Feb 13, 2020 at 4:51 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Feb 13, 2020 at 3:23 AM Justin Pryzby <pryzby@telsasoft.com> 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

From
Tom Lane
Date:
Amit Langote <amitlangote09@gmail.com> writes:
> On Thu, Feb 13, 2020 at 4:51 PM Amit Langote <amitlangote09@gmail.com> 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

From
Amit Langote
Date:
On Fri, Feb 14, 2020 at 1:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Thu, Feb 13, 2020 at 4:51 PM Amit Langote <amitlangote09@gmail.com> 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

From
Amit Langote
Date:
On Fri, Feb 14, 2020 at 2:58 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Feb 14, 2020 at 1:04 AM Tom Lane <tgl@sss.pgh.pa.us> 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

Attachment

Re: assert pg_class.relnatts is consistent

From
Michael Paquier
Date:
On Fri, Feb 14, 2020 at 06:00:05PM +0900, Amit Langote wrote:
> On Fri, Feb 14, 2020 at 2:58 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Feb 14, 2020 at 1:04 AM Tom Lane <tgl@sss.pgh.pa.us> 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

Attachment

Re: assert pg_class.relnatts is consistent

From
John Naylor
Date:
On Fri, Feb 14, 2020 at 5:00 PM Amit Langote <amitlangote09@gmail.com> 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 Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: assert pg_class.relnatts is consistent

From
John Naylor
Date:
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 Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: assert pg_class.relnatts is consistent

From
John Naylor
Date:
> pronangs, since we need more information than what's in each pg_class

Sigh, and of course I met pg_proc.pronargs.

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: assert pg_class.relnatts is consistent

From
Amit Langote
Date:
On Fri, Feb 14, 2020 at 6:47 PM Michael Paquier <michael@paquier.xyz> 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 <amitlangote09@gmail.com> wrote:
> > > On Fri, Feb 14, 2020 at 1:04 AM Tom Lane <tgl@sss.pgh.pa.us> 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

From
Amit Langote
Date:
Hi John,

On Fri, Feb 14, 2020 at 6:50 PM John Naylor <john.naylor@2ndquadrant.com> wrote:
> On Fri, Feb 14, 2020 at 5:00 PM Amit Langote <amitlangote09@gmail.com> 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

Attachment

Re: assert pg_class.relnatts is consistent

From
Alvaro Herrera
Date:
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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: assert pg_class.relnatts is consistent

From
Alvaro Herrera
Date:
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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: assert pg_class.relnatts is consistent

From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> 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 => 'pg_class' },

 ]
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index a12fc1f..78b33b2 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -24,6 +24,9 @@
 /* ----------------
  *        pg_class definition.  cpp turns this into
  *        typedef struct FormData_pg_class
+ *
+ * Note that the BKI_DEFAULT values below are only used for rows describing
+ * BKI_BOOTSTRAP catalogs, since only those rows appear in pg_class.dat.
  * ----------------
  */
 CATALOG(pg_class,1259,RelationRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83,RelationRelation_Rowtype_Id)
BKI_SCHEMA_MACRO
@@ -47,41 +50,41 @@ CATALOG(pg_class,1259,RelationRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83,Relat
     Oid            relowner BKI_DEFAULT(PGUID);

     /* access method; 0 if not a table / index */
-    Oid            relam BKI_LOOKUP(pg_am);
+    Oid            relam BKI_DEFAULT(heap) BKI_LOOKUP(pg_am);

     /* identifier of physical storage file */
     /* relfilenode == 0 means it is a "mapped" relation, see relmapper.c */
-    Oid            relfilenode;
+    Oid            relfilenode BKI_DEFAULT(0);

     /* identifier of table space for relation (0 means default for database) */
     Oid            reltablespace BKI_DEFAULT(0) BKI_LOOKUP(pg_tablespace);

     /* # of blocks (not always up-to-date) */
-    int32        relpages;
+    int32        relpages BKI_DEFAULT(0);

     /* # of tuples (not always up-to-date) */
-    float4        reltuples;
+    float4        reltuples BKI_DEFAULT(0);

     /* # of all-visible blocks (not always up-to-date) */
-    int32        relallvisible;
+    int32        relallvisible BKI_DEFAULT(0);

     /* OID of toast table; 0 if none */
-    Oid            reltoastrelid;
+    Oid            reltoastrelid BKI_DEFAULT(0);

     /* T if has (or has had) any indexes */
-    bool        relhasindex;
+    bool        relhasindex BKI_DEFAULT(f);

     /* T if shared across databases */
-    bool        relisshared;
+    bool        relisshared BKI_DEFAULT(f);

     /* see RELPERSISTENCE_xxx constants below */
-    char        relpersistence;
+    char        relpersistence BKI_DEFAULT(p);

     /* see RELKIND_xxx constants below */
-    char        relkind;
+    char        relkind BKI_DEFAULT(r);

     /* number of user attributes */
-    int16        relnatts;
+    int16        relnatts BKI_DEFAULT(0);    /* genbki.pl will fill this in */

     /*
      * Class pg_attribute must contain exactly "relnatts" user attributes
@@ -90,51 +93,51 @@ CATALOG(pg_class,1259,RelationRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83,Relat
      */

     /* # of CHECK constraints for class */
-    int16        relchecks;
+    int16        relchecks BKI_DEFAULT(0);

     /* has (or has had) any rules */
-    bool        relhasrules;
+    bool        relhasrules BKI_DEFAULT(f);

     /* has (or has had) any TRIGGERs */
-    bool        relhastriggers;
+    bool        relhastriggers BKI_DEFAULT(f);

     /* has (or has had) child tables or indexes */
-    bool        relhassubclass;
+    bool        relhassubclass BKI_DEFAULT(f);

     /* row security is enabled or not */
-    bool        relrowsecurity;
+    bool        relrowsecurity BKI_DEFAULT(f);

     /* row security forced for owners or not */
-    bool        relforcerowsecurity;
+    bool        relforcerowsecurity BKI_DEFAULT(f);

     /* matview currently holds query results */
-    bool        relispopulated;
+    bool        relispopulated BKI_DEFAULT(t);

     /* see REPLICA_IDENTITY_xxx constants */
-    char        relreplident;
+    char        relreplident BKI_DEFAULT(n);

     /* is relation a partition? */
-    bool        relispartition;
+    bool        relispartition BKI_DEFAULT(f);

     /* heap for rewrite during DDL, link to original rel */
     Oid            relrewrite BKI_DEFAULT(0);

     /* all Xids < this are frozen in this rel */
-    TransactionId relfrozenxid;
+    TransactionId relfrozenxid BKI_DEFAULT(3);    /* FirstNormalTransactionId */

     /* all multixacts in this rel are >= this; it is really a MultiXactId */
-    TransactionId relminmxid;
+    TransactionId relminmxid BKI_DEFAULT(1);    /* FirstMultiXactId */

 #ifdef CATALOG_VARLEN            /* variable-length fields start here */
     /* NOTE: These fields are not present in a relcache entry's rd_rel field. */
     /* access permissions */
-    aclitem        relacl[1];
+    aclitem        relacl[1] BKI_DEFAULT(_null_);

     /* access-method-specific options */
-    text        reloptions[1];
+    text        reloptions[1] BKI_DEFAULT(_null_);

     /* partition bound node tree */
-    pg_node_tree relpartbound;
+    pg_node_tree relpartbound BKI_DEFAULT(_null_);
 #endif
 } FormData_pg_class;


Re: assert pg_class.relnatts is consistent

From
Tom Lane
Date:
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

From
Amit Langote
Date:
On Sun, Feb 16, 2020 at 5:25 AM Tom Lane <tgl@sss.pgh.pa.us> 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

From
Justin Pryzby
Date:
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