On 2024-May-07, Kyotaro Horiguchi wrote:
> Hello,
>
> At Wed, 1 May 2024 19:49:35 +0200, Alvaro Herrera <alvhe...@alvh.no-ip.org>
> wrote in
> > Here are two patches that I intend to push soon (hopefully tomorrow).
>
> This commit added and edited two error messages, resulting in using
> slightly different wordings "in" and "on" for relation constraints.
>
> + errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on
> relation \"%s\"",
> ===
> + errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in
> relation \"%s\"",
Thank you, I hadn't noticed the inconsistency -- I fix this in the
attached series.
While trying to convince myself that I could mark the remaining open
item for this work closed, I discovered that pg_dump fails to produce
working output for some combinations. Notably, if I create Andrew
Bille's example in 16:
create table test_0 (id serial primary key);
create table test_1 (id integer primary key) inherits (test_0);
then current master's pg_dump produces output that the current server
fails to restore, failing the PK creation in test_0:
ALTER TABLE ONLY public.test_0
ADD CONSTRAINT test_0_pkey PRIMARY KEY (id);
ERROR: cannot change NO INHERIT status of NOT NULL constraint
"pgdump_throwaway_notnull_0" in relation "test_1"
because we have already created the NOT NULL NO INHERIT constraint in
test_1 when we created it, and because of d45597f72fe5, we refuse to
change it into a regular inheritable constraint, which the PK in its
parent table needs.
I spent a long time trying to think how to fix this, and I had despaired
wanting to write that I would need to revert the whole NOT NULL business
for pg17 -- but that was until I realized that we don't actually need
this NOT NULL NO INHERIT business except during pg_upgrade, and that
simplifies things enough to give me confidence that the whole feature
can be kept.
Because, remember: the idea of those NO INHERIT "throwaway" constraints
is that we can skip reading the data when we create the PRIMARY KEY
during binary upgrade. We don't actually need the NO INHERIT
constraints for anything during regular pg_dump. So what we can do, is
restrict the usage of NOT NULL NO INHERIT so that they occur only during
pg_upgrade. I think this will make Justin P. happier, because we no
longer have these unsightly NOT NULL NO INHERIT nonstandard syntax in
dumps.
The attached patch series does that. Actually, it does a little more,
but it's not really much:
0001: fix the typos pointed out by Kyotaro.
0002: A mechanical code movement that takes some ugly ballast out of
getTableAttrs into its own routine. I realized that this new code was
far too ugly and messy to be in the middle of filling the tbinfo struct
of attributes. If you use "git show --color-moved
--color-moved-ws=ignore-all-space" with this commit you can see that
nothing happens apart from the code move.
0003: pgindent, fixes the comments just moved to account for different
indentation depth.
0004: moves again the moved PQfnumber() calls back to getTableAttrs(),
for efficiency (we don't want to search the result for those resnums for
every single attribute of all tables being dumped).
0005: This is the actual code change I describe above. We restrict
use_throwaway_nulls so that it's only set during binary upgrade mode.
This changes pg_dump output; in the normal case, we no longer have NOT
NULL NO INHERIT. I added one test stanza to verify that pg_upgrade
retains these clauses, where they are critical.
0006: Tighten up what d45597f72fe5 did, in that outside of binary
upgrade mode, we no longer accept changes to NOT NULL NO INHERIT
constraints so that they become INHERIT. Previously we accepted that
during recursion, but this isn't really very principled. (I had
accepted this because pg_dump required it for some other cases). This
changes some test output, and I also simplify some test cases that were
testing stuff that's no longer interesting.
(To push, I'll squash 0002+0003+0004 as a single one, and perhaps 0005
with them; I produced them like this only to make them easy to see
what's changing.)
I also have a pending patch for 16 that adds tables like the problematic
ones so that they remain for future pg_upgrade testing. With the
changes in this series, the whole thing finally works AFAICT.
I did notice one more small bit of weirdness, which is that at the end
of the process you may end up with constraints that retain the throwaway
name. This doesn't seem at all critical, considering that you can't
drop them anyway and such names do not survive a further dump (because
they are marked as inherited constraint without a "local" definition, so
they're not dumped separately). I would still like to fix it, but it
seems to require unduly contortions so I may end up not doing anything
about it.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
>From 5cdec1b6ce61f75d886109d7daafd57b8064a4a6 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 8 May 2024 11:15:53 +0200
Subject: [PATCH 1/6] Fix typos in error messages
Reported by Kyotaro Horiguchi
---
src/backend/catalog/pg_constraint.c | 2 +-
src/backend/commands/tablecmds.c | 3 +--
src/test/regress/expected/inherit.out | 4 ++--
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 6b8496e085..12a73d5a30 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -758,7 +758,7 @@ AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
if (!allow_noinherit_change)
ereport(ERROR,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in relation \"%s\"",
+ errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"",
NameStr(conform->conname), get_rel_name(relid)));
conform->connoinherit = false;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5bf5e69c5b..925978c35b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7905,11 +7905,10 @@ ATExecSetNotNull(List **wqueue, Relation rel, char *conName, char *colName,
if (conForm->connoinherit && recurse)
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in relation \"%s\"",
+ errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"",
NameStr(conForm->conname),
RelationGetRelationName(rel)));
-
/*
* If we find an appropriate constraint, we're almost done, but just
* need to change some properties on it: if we're recursing, increment
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index eaa65049c7..a621db0aa3 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2280,9 +2280,9 @@ drop table inh_nn_parent, inh_nn_child, inh_nn_child2;
CREATE TABLE inh_nn_parent (a int, NOT NULL a NO INHERIT);
CREATE TABLE inh_nn_child() INHERITS (inh_nn_parent);
ALTER TABLE inh_nn_parent ADD CONSTRAINT nna NOT NULL a;
-ERROR: cannot change NO INHERIT status of NOT NULL constraint "inh_nn_parent_a_not_null" in relation "inh_nn_parent"
+ERROR: cannot change NO INHERIT status of NOT NULL constraint "inh_nn_parent_a_not_null" on relation "inh_nn_parent"
ALTER TABLE inh_nn_parent ALTER a SET NOT NULL;
-ERROR: cannot change NO INHERIT status of NOT NULL constraint "inh_nn_parent_a_not_null" in relation "inh_nn_parent"
+ERROR: cannot change NO INHERIT status of NOT NULL constraint "inh_nn_parent_a_not_null" on relation "inh_nn_parent"
DROP TABLE inh_nn_parent cascade;
NOTICE: drop cascades to table inh_nn_child
-- Adding a PK at the top level of a hierarchy should cause all descendants
--
2.39.2
>From e6b5187a616bf3a37df7fd39cb71c710950bb826 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 8 May 2024 13:27:27 +0200
Subject: [PATCH 2/6] Mechanical move of not-null code out of getTableAttrs
---
src/bin/pg_dump/pg_dump.c | 314 ++++++++++++++++++++------------------
1 file changed, 166 insertions(+), 148 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 379debac24..1d8b137814 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -292,6 +292,8 @@ static void getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, c
static void makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo);
static void buildMatViewRefreshDependencies(Archive *fout);
static void getTableDataFKConstraints(void);
+static void determineNotNullFlags(Archive *fout, PGresult *res, int r,
+ TableInfo *tbinfo, int j, int *notnullcount);
static char *format_function_arguments(const FuncInfo *finfo, const char *funcargs,
bool is_agg);
static char *format_function_signature(Archive *fout,
@@ -8702,10 +8704,6 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
int i_attlen;
int i_attalign;
int i_attislocal;
- int i_notnull_name;
- int i_notnull_noinherit;
- int i_notnull_is_pk;
- int i_notnull_inh;
int i_attoptions;
int i_attcollation;
int i_attcompression;
@@ -8900,10 +8898,6 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
i_attlen = PQfnumber(res, "attlen");
i_attalign = PQfnumber(res, "attalign");
i_attislocal = PQfnumber(res, "attislocal");
- i_notnull_name = PQfnumber(res, "notnull_name");
- i_notnull_noinherit = PQfnumber(res, "notnull_noinherit");
- i_notnull_is_pk = PQfnumber(res, "notnull_is_pk");
- i_notnull_inh = PQfnumber(res, "notnull_inh");
i_attoptions = PQfnumber(res, "attoptions");
i_attcollation = PQfnumber(res, "attcollation");
i_attcompression = PQfnumber(res, "attcompression");
@@ -8980,10 +8974,6 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
for (int j = 0; j < numatts; j++, r++)
{
- bool use_named_notnull = false;
- bool use_unnamed_notnull = false;
- bool use_throwaway_notnull = false;
-
if (j + 1 != atoi(PQgetvalue(res, r, i_attnum)))
pg_fatal("invalid column numbering in table \"%s\"",
tbinfo->dobj.name);
@@ -9003,142 +8993,9 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
tbinfo->attalign[j] = *(PQgetvalue(res, r, i_attalign));
tbinfo->attislocal[j] = (PQgetvalue(res, r, i_attislocal)[0] == 't');
- /*
- * Not-null constraints require a jumping through a few hoops.
- * First, if the user has specified a constraint name that's not
- * the system-assigned default name, then we need to preserve
- * that. But if they haven't, then we don't want to use the
- * verbose syntax in the dump output. (Also, in versions prior to
- * 17, there was no constraint name at all.)
- *
- * (XXX Comparing the name this way to a supposed default name is
- * a bit of a hack, but it beats having to store a boolean flag in
- * pg_constraint just for this, or having to compute the knowledge
- * at pg_dump time from the server.)
- *
- * We also need to know if a column is part of the primary key. In
- * that case, we want to mark the column as not-null at table
- * creation time, so that the table doesn't have to be scanned to
- * check for nulls when the PK is created afterwards; this is
- * especially critical during pg_upgrade (where the data would not
- * be scanned at all otherwise.) If the column is part of the PK
- * and does not have any other not-null constraint, then we
- * fabricate a throwaway constraint name that we later use to
- * remove the constraint after the PK has been created.
- *
- * For inheritance child tables, we don't want to print not-null
- * when the constraint was defined at the parent level instead of
- * locally.
- */
-
- /*
- * We use notnull_inh to suppress unwanted not-null constraints in
- * inheritance children, when said constraints come from the
- * parent(s).
- */
- tbinfo->notnull_inh[j] = PQgetvalue(res, r, i_notnull_inh)[0] == 't';
-
- if (fout->remoteVersion < 170000)
- {
- if (!PQgetisnull(res, r, i_notnull_name) &&
- dopt->binary_upgrade &&
- !tbinfo->ispartition &&
- tbinfo->notnull_inh[j])
- {
- use_named_notnull = true;
- /* XXX should match ChooseConstraintName better */
- tbinfo->notnull_constrs[j] =
- psprintf("%s_%s_not_null", tbinfo->dobj.name,
- tbinfo->attnames[j]);
- }
- else if (PQgetvalue(res, r, i_notnull_is_pk)[0] == 't')
- {
- /*
- * We want this flag to be set for columns of a primary
- * key in which data is going to be loaded by the dump we
- * produce; thus a partitioned table doesn't need it.
- */
- if (tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
- use_throwaway_notnull = true;
- }
- else if (!PQgetisnull(res, r, i_notnull_name))
- use_unnamed_notnull = true;
- }
- else
- {
- if (!PQgetisnull(res, r, i_notnull_name))
- {
- /*
- * In binary upgrade of inheritance child tables, must
- * have a constraint name that we can UPDATE later.
- */
- if (dopt->binary_upgrade &&
- !tbinfo->ispartition &&
- tbinfo->notnull_inh[j])
- {
- use_named_notnull = true;
- tbinfo->notnull_constrs[j] =
- pstrdup(PQgetvalue(res, r, i_notnull_name));
-
- }
- else
- {
- char *default_name;
-
- /* XXX should match ChooseConstraintName better */
- default_name = psprintf("%s_%s_not_null", tbinfo->dobj.name,
- tbinfo->attnames[j]);
- if (strcmp(default_name,
- PQgetvalue(res, r, i_notnull_name)) == 0)
- use_unnamed_notnull = true;
- else
- {
- use_named_notnull = true;
- tbinfo->notnull_constrs[j] =
- pstrdup(PQgetvalue(res, r, i_notnull_name));
- }
- }
- }
- else if (PQgetvalue(res, r, i_notnull_is_pk)[0] == 't')
- {
- /* see above */
- if (tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
- use_throwaway_notnull = true;
- }
- }
-
- if (use_unnamed_notnull)
- {
- tbinfo->notnull_constrs[j] = "";
- tbinfo->notnull_throwaway[j] = false;
- }
- else if (use_named_notnull)
- {
- /* The name itself has already been determined */
- tbinfo->notnull_throwaway[j] = false;
- }
- else if (use_throwaway_notnull)
- {
- /*
- * Give this constraint a throwaway name.
- */
- tbinfo->notnull_constrs[j] =
- psprintf("pgdump_throwaway_notnull_%d", notnullcount++);
- tbinfo->notnull_throwaway[j] = true;
- tbinfo->notnull_inh[j] = false;
- }
- else
- {
- tbinfo->notnull_constrs[j] = NULL;
- tbinfo->notnull_throwaway[j] = false;
- }
-
- /*
- * Throwaway constraints must always be NO INHERIT; otherwise do
- * what the catalog says.
- */
- tbinfo->notnull_noinh[j] = use_throwaway_notnull ||
- PQgetvalue(res, r, i_notnull_noinherit)[0] == 't';
+ /* Handle not-null constraint name flags separately */
+ determineNotNullFlags(fout, res, r,
+ tbinfo, j, ¬nullcount);
tbinfo->attoptions[j] = pg_strdup(PQgetvalue(res, r, i_attoptions));
tbinfo->attcollation[j] = atooid(PQgetvalue(res, r, i_attcollation));
@@ -9428,6 +9285,167 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
destroyPQExpBuffer(checkoids);
}
+/*
+ * Based on the getTableAttrs query for one row, set the name and flags to
+ * handle its not-null constraint.
+ *
+ * Result row 'r' is for tbinfo's attribute 'j'.
+ */
+static void
+determineNotNullFlags(Archive *fout, PGresult *res, int r,
+ TableInfo *tbinfo, int j, int *notnullcount)
+{
+ DumpOptions *dopt = fout->dopt;
+ int i_notnull_name;
+ int i_notnull_noinherit;
+ int i_notnull_is_pk;
+ int i_notnull_inh;
+ bool use_named_notnull = false;
+ bool use_unnamed_notnull = false;
+ bool use_throwaway_notnull = false;
+
+ i_notnull_name = PQfnumber(res, "notnull_name");
+ i_notnull_noinherit = PQfnumber(res, "notnull_noinherit");
+ i_notnull_is_pk = PQfnumber(res, "notnull_is_pk");
+ i_notnull_inh = PQfnumber(res, "notnull_inh");
+
+ /*
+ * Not-null constraints require a jumping through a few hoops.
+ * First, if the user has specified a constraint name that's not
+ * the system-assigned default name, then we need to preserve
+ * that. But if they haven't, then we don't want to use the
+ * verbose syntax in the dump output. (Also, in versions prior to
+ * 17, there was no constraint name at all.)
+ *
+ * (XXX Comparing the name this way to a supposed default name is
+ * a bit of a hack, but it beats having to store a boolean flag in
+ * pg_constraint just for this, or having to compute the knowledge
+ * at pg_dump time from the server.)
+ *
+ * We also need to know if a column is part of the primary key. In
+ * that case, we want to mark the column as not-null at table
+ * creation time, so that the table doesn't have to be scanned to
+ * check for nulls when the PK is created afterwards; this is
+ * especially critical during pg_upgrade (where the data would not
+ * be scanned at all otherwise.) If the column is part of the PK
+ * and does not have any other not-null constraint, then we
+ * fabricate a throwaway constraint name that we later use to
+ * remove the constraint after the PK has been created.
+ *
+ * For inheritance child tables, we don't want to print not-null
+ * when the constraint was defined at the parent level instead of
+ * locally.
+ */
+
+ /*
+ * We use notnull_inh to suppress unwanted not-null constraints in
+ * inheritance children, when said constraints come from the
+ * parent(s).
+ */
+ tbinfo->notnull_inh[j] = PQgetvalue(res, r, i_notnull_inh)[0] == 't';
+
+ if (fout->remoteVersion < 170000)
+ {
+ if (!PQgetisnull(res, r, i_notnull_name) &&
+ dopt->binary_upgrade &&
+ !tbinfo->ispartition &&
+ tbinfo->notnull_inh[j])
+ {
+ use_named_notnull = true;
+ /* XXX should match ChooseConstraintName better */
+ tbinfo->notnull_constrs[j] =
+ psprintf("%s_%s_not_null", tbinfo->dobj.name,
+ tbinfo->attnames[j]);
+ }
+ else if (PQgetvalue(res, r, i_notnull_is_pk)[0] == 't')
+ {
+ /*
+ * We want this flag to be set for columns of a primary
+ * key in which data is going to be loaded by the dump we
+ * produce; thus a partitioned table doesn't need it.
+ */
+ if (tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
+ use_throwaway_notnull = true;
+ }
+ else if (!PQgetisnull(res, r, i_notnull_name))
+ use_unnamed_notnull = true;
+ }
+ else
+ {
+ if (!PQgetisnull(res, r, i_notnull_name))
+ {
+ /*
+ * In binary upgrade of inheritance child tables, must
+ * have a constraint name that we can UPDATE later.
+ */
+ if (dopt->binary_upgrade &&
+ !tbinfo->ispartition &&
+ tbinfo->notnull_inh[j])
+ {
+ use_named_notnull = true;
+ tbinfo->notnull_constrs[j] =
+ pstrdup(PQgetvalue(res, r, i_notnull_name));
+ }
+ else
+ {
+ char *default_name;
+
+ /* XXX should match ChooseConstraintName better */
+ default_name = psprintf("%s_%s_not_null", tbinfo->dobj.name,
+ tbinfo->attnames[j]);
+ if (strcmp(default_name,
+ PQgetvalue(res, r, i_notnull_name)) == 0)
+ use_unnamed_notnull = true;
+ else
+ {
+ use_named_notnull = true;
+ tbinfo->notnull_constrs[j] =
+ pstrdup(PQgetvalue(res, r, i_notnull_name));
+ }
+ }
+ }
+ else if (PQgetvalue(res, r, i_notnull_is_pk)[0] == 't')
+ {
+ /* see above */
+ if (tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
+ use_throwaway_notnull = true;
+ }
+ }
+
+ if (use_unnamed_notnull)
+ {
+ tbinfo->notnull_constrs[j] = "";
+ tbinfo->notnull_throwaway[j] = false;
+ }
+ else if (use_named_notnull)
+ {
+ /* The name itself has already been determined */
+ tbinfo->notnull_throwaway[j] = false;
+ }
+ else if (use_throwaway_notnull)
+ {
+ /*
+ * Give this constraint a throwaway name.
+ */
+ tbinfo->notnull_constrs[j] =
+ psprintf("pgdump_throwaway_notnull_%d", (*notnullcount)++);
+ tbinfo->notnull_throwaway[j] = true;
+ tbinfo->notnull_inh[j] = false;
+ }
+ else
+ {
+ tbinfo->notnull_constrs[j] = NULL;
+ tbinfo->notnull_throwaway[j] = false;
+ }
+
+ /*
+ * Throwaway constraints must always be NO INHERIT; otherwise do
+ * what the catalog says.
+ */
+ tbinfo->notnull_noinh[j] = use_throwaway_notnull ||
+ PQgetvalue(res, r, i_notnull_noinherit)[0] == 't';
+}
+
/*
* Test whether a column should be printed as part of table's CREATE TABLE.
* Column number is zero-based.
--
2.39.2
>From 00f2d7f61e5ec50f33366fe63015b2edfbf71f8a Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 8 May 2024 13:28:01 +0200
Subject: [PATCH 3/6] pgindent run
---
src/bin/pg_dump/pg_dump.c | 58 ++++++++++++++++++---------------------
1 file changed, 27 insertions(+), 31 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 1d8b137814..739b16516f 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -9310,37 +9310,33 @@ determineNotNullFlags(Archive *fout, PGresult *res, int r,
i_notnull_inh = PQfnumber(res, "notnull_inh");
/*
- * Not-null constraints require a jumping through a few hoops.
- * First, if the user has specified a constraint name that's not
- * the system-assigned default name, then we need to preserve
- * that. But if they haven't, then we don't want to use the
- * verbose syntax in the dump output. (Also, in versions prior to
- * 17, there was no constraint name at all.)
+ * Not-null constraints require a jumping through a few hoops. First, if
+ * the user has specified a constraint name that's not the system-assigned
+ * default name, then we need to preserve that. But if they haven't, then
+ * we don't want to use the verbose syntax in the dump output. (Also, in
+ * versions prior to 17, there was no constraint name at all.)
*
- * (XXX Comparing the name this way to a supposed default name is
- * a bit of a hack, but it beats having to store a boolean flag in
- * pg_constraint just for this, or having to compute the knowledge
- * at pg_dump time from the server.)
+ * (XXX Comparing the name this way to a supposed default name is a bit of
+ * a hack, but it beats having to store a boolean flag in pg_constraint
+ * just for this, or having to compute the knowledge at pg_dump time from
+ * the server.)
*
- * We also need to know if a column is part of the primary key. In
- * that case, we want to mark the column as not-null at table
- * creation time, so that the table doesn't have to be scanned to
- * check for nulls when the PK is created afterwards; this is
- * especially critical during pg_upgrade (where the data would not
- * be scanned at all otherwise.) If the column is part of the PK
- * and does not have any other not-null constraint, then we
- * fabricate a throwaway constraint name that we later use to
- * remove the constraint after the PK has been created.
+ * We also need to know if a column is part of the primary key. In that
+ * case, we want to mark the column as not-null at table creation time, so
+ * that the table doesn't have to be scanned to check for nulls when the
+ * PK is created afterwards; this is especially critical during pg_upgrade
+ * (where the data would not be scanned at all otherwise.) If the column
+ * is part of the PK and does not have any other not-null constraint, then
+ * we fabricate a throwaway constraint name that we later use to remove
+ * the constraint after the PK has been created.
*
- * For inheritance child tables, we don't want to print not-null
- * when the constraint was defined at the parent level instead of
- * locally.
+ * For inheritance child tables, we don't want to print not-null when the
+ * constraint was defined at the parent level instead of locally.
*/
/*
* We use notnull_inh to suppress unwanted not-null constraints in
- * inheritance children, when said constraints come from the
- * parent(s).
+ * inheritance children, when said constraints come from the parent(s).
*/
tbinfo->notnull_inh[j] = PQgetvalue(res, r, i_notnull_inh)[0] == 't';
@@ -9360,9 +9356,9 @@ determineNotNullFlags(Archive *fout, PGresult *res, int r,
else if (PQgetvalue(res, r, i_notnull_is_pk)[0] == 't')
{
/*
- * We want this flag to be set for columns of a primary
- * key in which data is going to be loaded by the dump we
- * produce; thus a partitioned table doesn't need it.
+ * We want this flag to be set for columns of a primary key in
+ * which data is going to be loaded by the dump we produce; thus a
+ * partitioned table doesn't need it.
*/
if (tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
use_throwaway_notnull = true;
@@ -9375,8 +9371,8 @@ determineNotNullFlags(Archive *fout, PGresult *res, int r,
if (!PQgetisnull(res, r, i_notnull_name))
{
/*
- * In binary upgrade of inheritance child tables, must
- * have a constraint name that we can UPDATE later.
+ * In binary upgrade of inheritance child tables, must have a
+ * constraint name that we can UPDATE later.
*/
if (dopt->binary_upgrade &&
!tbinfo->ispartition &&
@@ -9439,8 +9435,8 @@ determineNotNullFlags(Archive *fout, PGresult *res, int r,
}
/*
- * Throwaway constraints must always be NO INHERIT; otherwise do
- * what the catalog says.
+ * Throwaway constraints must always be NO INHERIT; otherwise do what the
+ * catalog says.
*/
tbinfo->notnull_noinh[j] = use_throwaway_notnull ||
PQgetvalue(res, r, i_notnull_noinherit)[0] == 't';
--
2.39.2
>From 1511877eb2b0eb3cc70cbe616f3652583daff8f9 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 8 May 2024 19:41:08 +0200
Subject: [PATCH 4/6] Take PQfnumber() calls out of the routine
---
src/bin/pg_dump/pg_dump.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 739b16516f..8af9127ba2 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -293,7 +293,9 @@ static void makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo);
static void buildMatViewRefreshDependencies(Archive *fout);
static void getTableDataFKConstraints(void);
static void determineNotNullFlags(Archive *fout, PGresult *res, int r,
- TableInfo *tbinfo, int j, int *notnullcount);
+ TableInfo *tbinfo, int j, int *notnullcount,
+ int i_notnull_name, int i_notnull_noinherit,
+ int i_notnull_is_pk, int i_notnull_inh);
static char *format_function_arguments(const FuncInfo *finfo, const char *funcargs,
bool is_agg);
static char *format_function_signature(Archive *fout,
@@ -8704,6 +8706,10 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
int i_attlen;
int i_attalign;
int i_attislocal;
+ int i_notnull_name;
+ int i_notnull_noinherit;
+ int i_notnull_is_pk;
+ int i_notnull_inh;
int i_attoptions;
int i_attcollation;
int i_attcompression;
@@ -8898,6 +8904,10 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
i_attlen = PQfnumber(res, "attlen");
i_attalign = PQfnumber(res, "attalign");
i_attislocal = PQfnumber(res, "attislocal");
+ i_notnull_name = PQfnumber(res, "notnull_name");
+ i_notnull_noinherit = PQfnumber(res, "notnull_noinherit");
+ i_notnull_is_pk = PQfnumber(res, "notnull_is_pk");
+ i_notnull_inh = PQfnumber(res, "notnull_inh");
i_attoptions = PQfnumber(res, "attoptions");
i_attcollation = PQfnumber(res, "attcollation");
i_attcompression = PQfnumber(res, "attcompression");
@@ -8995,7 +9005,9 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
/* Handle not-null constraint name flags separately */
determineNotNullFlags(fout, res, r,
- tbinfo, j, ¬nullcount);
+ tbinfo, j, ¬nullcount,
+ i_notnull_name, i_notnull_noinherit,
+ i_notnull_is_pk, i_notnull_inh);
tbinfo->attoptions[j] = pg_strdup(PQgetvalue(res, r, i_attoptions));
tbinfo->attcollation[j] = atooid(PQgetvalue(res, r, i_attcollation));
@@ -9293,22 +9305,15 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
*/
static void
determineNotNullFlags(Archive *fout, PGresult *res, int r,
- TableInfo *tbinfo, int j, int *notnullcount)
+ TableInfo *tbinfo, int j, int *notnullcount,
+ int i_notnull_name, int i_notnull_noinherit,
+ int i_notnull_is_pk, int i_notnull_inh)
{
DumpOptions *dopt = fout->dopt;
- int i_notnull_name;
- int i_notnull_noinherit;
- int i_notnull_is_pk;
- int i_notnull_inh;
bool use_named_notnull = false;
bool use_unnamed_notnull = false;
bool use_throwaway_notnull = false;
- i_notnull_name = PQfnumber(res, "notnull_name");
- i_notnull_noinherit = PQfnumber(res, "notnull_noinherit");
- i_notnull_is_pk = PQfnumber(res, "notnull_is_pk");
- i_notnull_inh = PQfnumber(res, "notnull_inh");
-
/*
* Not-null constraints require a jumping through a few hoops. First, if
* the user has specified a constraint name that's not the system-assigned
--
2.39.2
>From e7b208fbccf74a9ea098b94392edf642712a9c44 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 8 May 2024 17:42:19 +0200
Subject: [PATCH 5/6] Use NOT NULL NO INHERIT only in pg_upgrade mode
---
src/bin/pg_dump/pg_dump.c | 6 ++++--
src/bin/pg_dump/t/002_pg_dump.pl | 17 +++++++++++++++--
2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 8af9127ba2..3b587499e1 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -9365,7 +9365,8 @@ determineNotNullFlags(Archive *fout, PGresult *res, int r,
* which data is going to be loaded by the dump we produce; thus a
* partitioned table doesn't need it.
*/
- if (tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
+ if (dopt->binary_upgrade &&
+ tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
use_throwaway_notnull = true;
}
else if (!PQgetisnull(res, r, i_notnull_name))
@@ -9408,7 +9409,8 @@ determineNotNullFlags(Archive *fout, PGresult *res, int r,
else if (PQgetvalue(res, r, i_notnull_is_pk)[0] == 't')
{
/* see above */
- if (tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
+ if (dopt->binary_upgrade &&
+ tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
use_throwaway_notnull = true;
}
}
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 7085053a2d..52eeb95bb6 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -3242,7 +3242,7 @@ my %tests = (
);',
regexp => qr/^
\QCREATE TABLE dump_test.fk_reference_test_table (\E
- \n\s+\Qcol1 integer CONSTRAINT \E[a-z0-9_]*\Q NOT NULL NO INHERIT\E
+ \n\s+\Qcol1 integer\E
\n\);
/xm,
like =>
@@ -3250,9 +3250,21 @@ my %tests = (
unlike => {
exclude_dump_test_schema => 1,
only_dump_measurement => 1,
+ binary_upgrade => 1,
},
},
+ # The same table as above when dumped in binary-upgrade mode
+ 'binary-upgrade dump for fk_reference_test_table' => {
+ regexp => qr/^
+ \QCREATE TABLE dump_test.fk_reference_test_table (\E
+ \n\s+\Qcol1 integer CONSTRAINT \E[a-z0-9_]*\Q NOT NULL NO INHERIT\E
+ \n\);
+ /xm,
+ like =>
+ { binary_upgrade => 1 }
+ },
+
'CREATE TABLE test_second_table' => {
create_order => 6,
create_sql => 'CREATE TABLE dump_test.test_second_table (
@@ -3635,7 +3647,7 @@ my %tests = (
);',
regexp => qr/^
\QCREATE TABLE dump_test.test_table_generated (\E\n
- \s+\Qcol1 integer CONSTRAINT \E[a-z0-9_]*\Q NOT NULL NO INHERIT,\E\n
+ \s+\Qcol1 integer,\E\n
\s+\Qcol2 integer GENERATED ALWAYS AS ((col1 * 2)) STORED\E\n
\);
/xms,
@@ -3644,6 +3656,7 @@ my %tests = (
unlike => {
exclude_dump_test_schema => 1,
only_dump_measurement => 1,
+ binary_upgrade => 1,
},
},
--
2.39.2
>From 92363783fa8bdd23e4a34d147a173354d7025e67 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 8 May 2024 18:38:20 +0200
Subject: [PATCH 6/6] Don't accept NO INHERIT changes to INHERIT in normal mode
---
src/backend/catalog/heap.c | 20 +++-------
src/test/regress/expected/constraints.out | 48 ++++-------------------
src/test/regress/expected/inherit.out | 13 ++----
src/test/regress/sql/constraints.sql | 22 ++---------
src/test/regress/sql/inherit.sql | 6 +--
5 files changed, 21 insertions(+), 88 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 922ba79ac2..38df163edb 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2549,23 +2549,15 @@ AddRelationNewConstraints(Relation rel,
/*
* If the column already has an inheritable not-null constraint,
- * we need only adjust its coninhcount and we're done. In certain
- * cases (see below), if the constraint is there but marked NO
- * INHERIT, then we mark it as no longer such and coninhcount
- * updated, plus we must also recurse to the children (if any) to
- * add the constraint there.
- *
- * We only allow the inheritability status to change during binary
- * upgrade (where it's used to add the not-null constraints for
- * children of tables with primary keys), or when we're recursing
- * processing a table down an inheritance hierarchy; directly
- * allowing a constraint to change from NO INHERIT to INHERIT
- * during ALTER TABLE ADD CONSTRAINT would be far too surprising
- * behavior.
+ * we need only adjust its coninhcount and we're done. In binary
+ * upgrade, if the constraint is there but marked NO INHERIT, then
+ * we mark it as no longer such and coninhcount updated, plus we
+ * must also recurse to the children (if any) to add the
+ * constraint there.
*/
existing = AdjustNotNullInheritance1(RelationGetRelid(rel), colnum,
cdef->inhcount, cdef->is_no_inherit,
- IsBinaryUpgrade || allow_merge);
+ IsBinaryUpgrade);
if (existing == 1)
continue; /* all done! */
else if (existing == -1)
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index ec7c9e53d0..0ffe4fd3f7 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -321,29 +321,22 @@ ALTER TABLE ATACC1 ADD NOT NULL a NO INHERIT;
Inherits: atacc1
DROP TABLE ATACC1, ATACC2;
--- no can do
+-- partitioned tables don't accept NO INHERIT constraints
CREATE TABLE ATACC1 (a int NOT NULL NO INHERIT) PARTITION BY LIST (a);
ERROR: not-null constraints on partitioned tables cannot be NO INHERIT
CREATE TABLE ATACC1 (a int, NOT NULL a NO INHERIT) PARTITION BY LIST (a);
ERROR: not-null constraints on partitioned tables cannot be NO INHERIT
--- overridding a no-inherit constraint with an inheritable one
+-- not possible to override a no-inherit constraint with an inheritable one
CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT);
CREATE TABLE ATACC1 (a int);
-CREATE TABLE ATACC3 (a int) INHERITS (ATACC2);
-NOTICE: merging column "a" with inherited definition
-INSERT INTO ATACC3 VALUES (null); -- make sure we scan atacc3
ALTER TABLE ATACC2 INHERIT ATACC1;
ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a;
-ERROR: column "a" of relation "atacc3" contains null values
-DELETE FROM ATACC3;
-ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a;
-\d+ ATACC[123]
+ERROR: cannot change NO INHERIT status of NOT NULL constraint "a_is_not_null" on relation "atacc2"
+\d+ ATACC[12]
Table "public.atacc1"
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
- a | integer | | not null | | plain | |
-Not-null constraints:
- "ditto" NOT NULL "a"
+ a | integer | | | | plain | |
Child tables: atacc2
Table "public.atacc2"
@@ -351,37 +344,10 @@ Child tables: atacc2
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | not null | | plain | |
Not-null constraints:
- "a_is_not_null" NOT NULL "a" (local, inherited)
+ "a_is_not_null" NOT NULL "a" NO INHERIT
Inherits: atacc1
-Child tables: atacc3
- Table "public.atacc3"
- Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
---------+---------+-----------+----------+---------+---------+--------------+-------------
- a | integer | | not null | | plain | |
-Not-null constraints:
- "ditto" NOT NULL "a" (inherited)
-Inherits: atacc2
-
-ALTER TABLE ATACC2 DROP CONSTRAINT a_is_not_null;
-ALTER TABLE ATACC1 DROP CONSTRAINT ditto;
-\d+ ATACC3
- Table "public.atacc3"
- Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
---------+---------+-----------+----------+---------+---------+--------------+-------------
- a | integer | | | | plain | |
-Inherits: atacc2
-
-DROP TABLE ATACC1, ATACC2, ATACC3;
--- The same cannot be achieved this way
-CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT);
-CREATE TABLE ATACC1 (a int, CONSTRAINT ditto NOT NULL a);
-CREATE TABLE ATACC3 (a int) INHERITS (ATACC2);
-NOTICE: merging column "a" with inherited definition
-ALTER TABLE ATACC2 INHERIT ATACC1;
-ERROR: cannot add NOT NULL constraint to column "a" of relation "atacc2" with inheritance children
-DETAIL: Existing constraint "a_is_not_null" is marked NO INHERIT.
-DROP TABLE ATACC1, ATACC2, ATACC3;
+DROP TABLE ATACC1, ATACC2;
--
-- Check constraints on INSERT INTO
--
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index a621db0aa3..1d40468015 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2290,21 +2290,14 @@ NOTICE: drop cascades to table inh_nn_child
CREATE TABLE inh_nn_lvl1 (a int);
CREATE TABLE inh_nn_lvl2 () INHERITS (inh_nn_lvl1);
CREATE TABLE inh_nn_lvl3 (CONSTRAINT foo NOT NULL a NO INHERIT) INHERITS (inh_nn_lvl2);
-CREATE TABLE inh_nn_lvl4 () INHERITS (inh_nn_lvl3);
-CREATE TABLE inh_nn_lvl5 () INHERITS (inh_nn_lvl4);
-INSERT INTO inh_nn_lvl2 VALUES (NULL);
ALTER TABLE inh_nn_lvl1 ADD PRIMARY KEY (a);
-ERROR: column "a" of relation "inh_nn_lvl2" contains null values
-DELETE FROM inh_nn_lvl2;
-INSERT INTO inh_nn_lvl5 VALUES (NULL);
+ERROR: cannot change NO INHERIT status of NOT NULL constraint "foo" on relation "inh_nn_lvl3"
+ALTER TABLE inh_nn_lvl3 DROP CONSTRAINT foo;
ALTER TABLE inh_nn_lvl1 ADD PRIMARY KEY (a);
-ERROR: column "a" of relation "inh_nn_lvl5" contains null values
DROP TABLE inh_nn_lvl1 CASCADE;
-NOTICE: drop cascades to 4 other objects
+NOTICE: drop cascades to 2 other objects
DETAIL: drop cascades to table inh_nn_lvl2
drop cascades to table inh_nn_lvl3
-drop cascades to table inh_nn_lvl4
-drop cascades to table inh_nn_lvl5
--
-- test inherit/deinherit
--
diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql
index e753b8c345..f8ca855e8b 100644
--- a/src/test/regress/sql/constraints.sql
+++ b/src/test/regress/sql/constraints.sql
@@ -212,31 +212,17 @@ ALTER TABLE ATACC1 ADD NOT NULL a NO INHERIT;
\d+ ATACC2
DROP TABLE ATACC1, ATACC2;
--- no can do
+-- partitioned tables don't accept NO INHERIT constraints
CREATE TABLE ATACC1 (a int NOT NULL NO INHERIT) PARTITION BY LIST (a);
CREATE TABLE ATACC1 (a int, NOT NULL a NO INHERIT) PARTITION BY LIST (a);
--- overridding a no-inherit constraint with an inheritable one
+-- not possible to override a no-inherit constraint with an inheritable one
CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT);
CREATE TABLE ATACC1 (a int);
-CREATE TABLE ATACC3 (a int) INHERITS (ATACC2);
-INSERT INTO ATACC3 VALUES (null); -- make sure we scan atacc3
ALTER TABLE ATACC2 INHERIT ATACC1;
ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a;
-DELETE FROM ATACC3;
-ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a;
-\d+ ATACC[123]
-ALTER TABLE ATACC2 DROP CONSTRAINT a_is_not_null;
-ALTER TABLE ATACC1 DROP CONSTRAINT ditto;
-\d+ ATACC3
-DROP TABLE ATACC1, ATACC2, ATACC3;
-
--- The same cannot be achieved this way
-CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT);
-CREATE TABLE ATACC1 (a int, CONSTRAINT ditto NOT NULL a);
-CREATE TABLE ATACC3 (a int) INHERITS (ATACC2);
-ALTER TABLE ATACC2 INHERIT ATACC1;
-DROP TABLE ATACC1, ATACC2, ATACC3;
+\d+ ATACC[12]
+DROP TABLE ATACC1, ATACC2;
--
-- Check constraints on INSERT INTO
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 2205e59aff..d37d2d3ce8 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -855,12 +855,8 @@ DROP TABLE inh_nn_parent cascade;
CREATE TABLE inh_nn_lvl1 (a int);
CREATE TABLE inh_nn_lvl2 () INHERITS (inh_nn_lvl1);
CREATE TABLE inh_nn_lvl3 (CONSTRAINT foo NOT NULL a NO INHERIT) INHERITS (inh_nn_lvl2);
-CREATE TABLE inh_nn_lvl4 () INHERITS (inh_nn_lvl3);
-CREATE TABLE inh_nn_lvl5 () INHERITS (inh_nn_lvl4);
-INSERT INTO inh_nn_lvl2 VALUES (NULL);
ALTER TABLE inh_nn_lvl1 ADD PRIMARY KEY (a);
-DELETE FROM inh_nn_lvl2;
-INSERT INTO inh_nn_lvl5 VALUES (NULL);
+ALTER TABLE inh_nn_lvl3 DROP CONSTRAINT foo;
ALTER TABLE inh_nn_lvl1 ADD PRIMARY KEY (a);
DROP TABLE inh_nn_lvl1 CASCADE;
--
2.39.2
