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.
/messages/by-id/20200207143935.GP403@telsasoft.com
Is there a better place to implement assertion from 0002 ?
Show quoted text
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
Attachments:
v1-0002-Give-developer-a-helpful-kick-in-the-pants-if-the.patchtext/x-diff; charset=us-asciiDownload
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
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.
/messages/by-id/20200207143935.GP403@telsasoft.comIs 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
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
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.
/messages/by-id/20200207143935.GP403@telsasoft.comIs 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
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.
/messages/by-id/20200207143935.GP403@telsasoft.comIs 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
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
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
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
Attachments:
0001-Don-t-require-relnatts-to-be-specified-in-pg_class.d.patchtext/plain; charset=US-ASCII; name=0001-Don-t-require-relnatts-to-be-specified-in-pg_class.d.patchDownload
From ec690b6e78176354ae033073cda1b0770e956a72 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlangote09@gmail.com>
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', relispartition => 'f', relfrozenxid => '3',
@@ -54,7 +57,7 @@
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',
+ relpersistence => 'p', relkind => 'r', relchecks => '0',
relhasrules => 'f', relhastriggers => 'f', relhassubclass => 'f',
relrowsecurity => 'f', relforcerowsecurity => 'f', relispopulated => 't',
relreplident => 'n', relispartition => 'f', relfrozenxid => '3',
--
2.16.5
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
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
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
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
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
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
Attachments:
v2-0001-Don-t-require-relnatts-to-be-specified-in-pg_clas.patchapplication/octet-stream; name=v2-0001-Don-t-require-relnatts-to-be-specified-in-pg_clas.patchDownload
From 1522a894a9e9b6a50514f04f218ff85d812d9aff Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlangote09@gmail.com>
Date: Fri, 14 Feb 2020 16:04:48 +0900
Subject: [PATCH v2] Don't require relnatts to be specified in pg_class.dat
---
src/backend/catalog/Catalog.pm | 4 ++++
src/backend/catalog/genbki.pl | 5 +++++
src/include/catalog/pg_class.dat | 14 ++++++++------
src/include/catalog/reformat_dat_file.pl | 13 ++++++++++---
4 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 368b1dea3e..fdeb0c1855 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 6eff045bd9..8ef10de2db 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -57,6 +57,7 @@ my %catalog_data;
my @toast_decls;
my @index_decls;
my %oidcounts;
+my %catalog_ncols;
foreach my $header (@ARGV)
{
@@ -72,6 +73,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
@@ -179,6 +181,9 @@ my %classoids;
foreach my $row (@{ $catalog_data{pg_class} })
{
$classoids{ $row->{relname} } = $row->{oid};
+
+ # Also fill in relnatts while at it.
+ $row->{relnatts} = $catalog_ncols{$row->{relname}};
}
# collation OID lookup
diff --git a/src/include/catalog/pg_class.dat b/src/include/catalog/pg_class.dat
index 9bcf28676d..befa345a8c 100644
--- a/src/include/catalog/pg_class.dat
+++ b/src/include/catalog/pg_class.dat
@@ -14,17 +14,19 @@
# 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.
+# their CATALOG and BKI_ROWTYPE_OID macros.
# 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 +36,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 +46,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', relispartition => 'f', relfrozenxid => '3',
@@ -54,7 +56,7 @@
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',
+ relpersistence => 'p', relkind => 'r', relchecks => '0',
relhasrules => 'f', relhastriggers => 'f', relhassubclass => 'f',
relrowsecurity => 'f', relforcerowsecurity => 'f', relispopulated => 't',
relreplident => 'n', relispartition => 'f', relfrozenxid => '3',
diff --git a/src/include/catalog/reformat_dat_file.pl b/src/include/catalog/reformat_dat_file.pl
index fd4dbad67e..19d9c3ceae 100755
--- a/src/include/catalog/reformat_dat_file.pl
+++ b/src/include/catalog/reformat_dat_file.pl
@@ -184,10 +184,12 @@ sub strip_default_values
{
my $attname = $column->{name};
- # It's okay if we have no oid value, since it will be assigned
- # automatically before bootstrap.
+ # It's okay if we have no oid and relnatts value, since it will be
+ # assigned automatically before bootstrap.
die "strip_default_values: $catname.$attname undefined\n"
- if !defined $row->{$attname} and $attname ne 'oid';
+ if !defined $row->{$attname} and
+ $attname ne 'oid' and
+ $attname ne 'relnatts';
if (defined $column->{default}
and ($row->{$attname} eq $column->{default}))
@@ -203,6 +205,11 @@ sub strip_default_values
delete $row->{pronargs} if defined $row->{proargtypes};
}
+ if ($catname eq 'pg_class')
+ {
+ delete $row->{relnatts} if defined $row->{relnatts};
+ }
+
# If a pg_type entry has an auto-generated array type, then its
# typarray field is a computed value too (see GenerateArrayTypes).
if ($catname eq 'pg_type')
--
2.20.1 (Apple Git-117)
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
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
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
Attachments:
v3-0001-no-manual-relnatts-assignment.patchtext/x-diff; charset=us-ascii; name=v3-0001-no-manual-relnatts-assignment.patchDownload
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;
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
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
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