On 2025-Apr-09, jian he wrote:
> hi.
>
> attached patch is for address pg_dump inconsistency
> when parent is "not null not valid" while child is "not null".
Here's my take on this patch. We don't really need the
notnull_parent_invalid flag; in flagInhAttrs we can just set "islocal"
to convince getTableAttrs to print the constraint. This also fixes the
bug that in getTableAttrs() you handled the case where
shouldPrintColumn() is true and not the other one.
I also added test cases to pg_dump/t/002_pg_dump.pl to verify that the
output was correct in those cases. In constraints.sql I added a couple
of tables to ensure that the pg_upgrade handling (the pg_dump
--binary-upgrade mode) is tested as well.
Looking at the surrounding code in flagInhAttrs I noticed that we're
mishandling this case:
create table parent1 (a int);
create table parent2 (a int);
create table child () inherits (parent1, parent2);
alter table parent1 add not null a;
alter table parent2 add not null a not valid;
We print the constraint for table child for no apparent reason.
Patch 0002 is a part of your proposed patch that I don't think we need,
but I'm open to hearing arguments about why we do, preferrably with some
test cases.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Hay que recordar que la existencia en el cosmos, y particularmente la
elaboración de civilizaciones dentro de él no son, por desgracia,
nada idílicas" (Ijon Tichy)
>From 00c46ba90a24a99857cdc849943cf3fd7b96bd8e Mon Sep 17 00:00:00 2001
From: jian he <jian.universal...@gmail.com>
Date: Wed, 9 Apr 2025 13:07:58 +0800
Subject: [PATCH 1/2] Fix pg_dump for inherited validated not-null constraints
When a child constraint is validated and its parent constraint isn't,
pg_dump requires special handling.
---
src/bin/pg_dump/common.c | 12 +++++++
src/bin/pg_dump/pg_dump.c | 12 ++++++-
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/pg_dump/t/002_pg_dump.pl | 41 +++++++++++++++++++++--
src/test/regress/expected/constraints.out | 24 +++++++++++++
src/test/regress/sql/constraints.sql | 14 ++++++++
6 files changed, 101 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 56b6c368acf..b0973d44f32 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -546,6 +546,18 @@ flagInhAttrs(Archive *fout, DumpOptions *dopt, TableInfo *tblinfo, int numTables
parent->notnull_constrs[inhAttrInd] != NULL)
foundNotNull = true;
+ /*
+ * For validated not-null constraints in child tables which
+ * derive from a parent constraint marked NOT VALID, we
+ * artificially mark the child constraint as local so that
+ * it is printed independently. Failing to do this would
+ * result in the child constraint being restored as NOT
+ * VALID.
+ */
+ if (fout->remoteVersion >= 180000 &&
+ parent->notnull_invalid[inhAttrInd])
+ tbinfo->notnull_islocal[j] = true;
+
foundDefault |= (parentDef != NULL &&
strcmp(parentDef->adef_expr, "NULL") != 0 &&
!parent->attgenerated[inhAttrInd]);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 105e917aa7b..0e2485479f8 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -9255,6 +9255,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
tbinfo->attfdwoptions = (char **) pg_malloc(numatts * sizeof(char *));
tbinfo->attmissingval = (char **) pg_malloc(numatts * sizeof(char *));
tbinfo->notnull_constrs = (char **) pg_malloc(numatts * sizeof(char *));
+ tbinfo->notnull_invalid = (bool *) pg_malloc(numatts * sizeof(bool));
tbinfo->notnull_noinh = (bool *) pg_malloc(numatts * sizeof(bool));
tbinfo->notnull_islocal = (bool *) pg_malloc(numatts * sizeof(bool));
tbinfo->attrdefs = (AttrDefInfo **) pg_malloc(numatts * sizeof(AttrDefInfo *));
@@ -9280,6 +9281,8 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
tbinfo->attlen[j] = atoi(PQgetvalue(res, r, i_attlen));
tbinfo->attalign[j] = *(PQgetvalue(res, r, i_attalign));
tbinfo->attislocal[j] = (PQgetvalue(res, r, i_attislocal)[0] == 't');
+ tbinfo->notnull_invalid[j] = false; /* it only change in
+ * determineNotNullFlags */
/* Handle not-null constraint name and flags */
determineNotNullFlags(fout, res, r,
@@ -9756,6 +9759,12 @@ determineNotNullFlags(Archive *fout, PGresult *res, int r,
else
appendPQExpBuffer(*invalidnotnulloids, ",%s", constroid);
+ /*
+ * Track when a parent constraint is invalid for the cases where a
+ * child constraint has been validated independenly.
+ */
+ tbinfo->notnull_invalid[j] = true;
+
/* nothing else to do */
tbinfo->notnull_constrs[j] = NULL;
return;
@@ -9763,10 +9772,11 @@ determineNotNullFlags(Archive *fout, PGresult *res, int r,
/*
* notnull_noinh is straight from the query result. notnull_islocal also,
- * though flagInhAttrs may change that one later in versions < 18.
+ * though flagInhAttrs may change that one later.
*/
tbinfo->notnull_noinh[j] = PQgetvalue(res, r, i_notnull_noinherit)[0] == 't';
tbinfo->notnull_islocal[j] = PQgetvalue(res, r, i_notnull_islocal)[0] == 't';
+ tbinfo->notnull_invalid[j] = false;
/*
* Determine a constraint name to use. If the column is not marked not-
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index b426b5e4736..7417eab6aef 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -365,6 +365,7 @@ typedef struct _tableInfo
* there isn't one on this column. If
* empty string, unnamed constraint
* (pre-v17) */
+ bool *notnull_invalid; /* true for NOT NULL NOT VALID */
bool *notnull_noinh; /* NOT NULL is NO INHERIT */
bool *notnull_islocal; /* true if NOT NULL has local definition */
struct _attrDefInfo **attrdefs; /* DEFAULT expressions */
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 6c03eca8e50..4494acaed8d 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -1118,10 +1118,17 @@ my %tests = (
},
},
- 'CONSTRAINT NOT NULL / INVALID' => {
+ 'CONSTRAINT NOT NULL / NOT VALID' => {
create_sql => 'CREATE TABLE dump_test.test_table_nn (
col1 int);
- ALTER TABLE dump_test.test_table_nn ADD CONSTRAINT nn NOT NULL col1 NOT VALID;',
+ CREATE TABLE dump_test.test_table_nn_chld1 (
+ ) INHERITS (dump_test.test_table_nn);
+ CREATE TABLE dump_test.test_table_nn_chld2 (
+ col1 int
+ ) INHERITS (dump_test.test_table_nn);
+ ALTER TABLE dump_test.test_table_nn ADD CONSTRAINT nn NOT NULL col1 NOT VALID;
+ ALTER TABLE dump_test.test_table_nn_chld1 VALIDATE CONSTRAINT nn;
+ ALTER TABLE dump_test.test_table_nn_chld2 VALIDATE CONSTRAINT nn;',
regexp => qr/^
\QALTER TABLE dump_test.test_table_nn\E \n^\s+
\QADD CONSTRAINT nn NOT NULL col1 NOT VALID;\E
@@ -1135,6 +1142,36 @@ my %tests = (
},
},
+ 'CONSTRAINT NOT NULL / NOT VALID (child1)' => {
+ regexp => qr/^
+ \QCREATE TABLE dump_test.test_table_nn_chld1 (\E\n
+ ^\s+\QCONSTRAINT nn NOT NULL col1\E$
+ /xm,
+ like => {
+ %full_runs, %dump_test_schema_runs, section_pre_data => 1,
+ },
+ unlike => {
+ exclude_dump_test_schema => 1,
+ only_dump_measurement => 1,
+ binary_upgrade => 1,
+ },
+ },
+
+ 'CONSTRAINT NOT NULL / NOT VALID (child2)' => {
+ regexp => qr/^
+ \QCREATE TABLE dump_test.test_table_nn_chld2 (\E\n
+ ^\s+\Qcol1 integer CONSTRAINT nn NOT NULL\E$
+ /xm,
+ like => {
+ %full_runs, %dump_test_schema_runs, section_pre_data => 1,
+ },
+ unlike => {
+ exclude_dump_test_schema => 1,
+ only_dump_measurement => 1,
+ },
+ },
+
+
'CONSTRAINT PRIMARY KEY / WITHOUT OVERLAPS' => {
create_sql => 'CREATE TABLE dump_test.test_table_tpk (
col1 int4range,
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index 92e441a16cd..89f88cf9eb2 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -1625,6 +1625,30 @@ EXECUTE get_nnconstraint_info('{notnull_part1_upg, notnull_part1_1_upg, notnull_
notnull_part1_upg | notnull_con | f | t | 0
(4 rows)
+-- Inheritance test tables for pg_upgrade
+create table constr_parent (a int);
+create table constr_child (a int) inherits (constr_parent);
+NOTICE: merging column "a" with inherited definition
+alter table constr_parent add not null a not valid;
+alter table constr_child validate constraint constr_parent_a_not_null;
+EXECUTE get_nnconstraint_info('{constr_parent, constr_child}');
+ tabname | conname | convalidated | conislocal | coninhcount
+---------------+--------------------------+--------------+------------+-------------
+ constr_child | constr_parent_a_not_null | t | f | 1
+ constr_parent | constr_parent_a_not_null | f | t | 0
+(2 rows)
+
+create table constr_parent2 (a int);
+create table constr_child2 () inherits (constr_parent2);
+alter table constr_parent2 add not null a not valid;
+alter table constr_child2 validate constraint constr_parent2_a_not_null;
+EXECUTE get_nnconstraint_info('{constr_parent2, constr_child2}');
+ tabname | conname | convalidated | conislocal | coninhcount
+----------------+---------------------------+--------------+------------+-------------
+ constr_child2 | constr_parent2_a_not_null | t | f | 1
+ constr_parent2 | constr_parent2_a_not_null | f | t | 0
+(2 rows)
+
DEALLOCATE get_nnconstraint_info;
-- end NOT NULL NOT VALID
-- Comments
diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql
index 5d6d749c150..f93b4095044 100644
--- a/src/test/regress/sql/constraints.sql
+++ b/src/test/regress/sql/constraints.sql
@@ -979,6 +979,20 @@ INSERT INTO notnull_part1_3_upg values(NULL,1);
ALTER TABLE notnull_part1_3_upg add CONSTRAINT nn3 NOT NULL a NOT VALID;
ALTER TABLE notnull_part1_upg ATTACH PARTITION notnull_part1_3_upg FOR VALUES IN (NULL,5);
EXECUTE get_nnconstraint_info('{notnull_part1_upg, notnull_part1_1_upg, notnull_part1_2_upg, notnull_part1_3_upg}');
+
+-- Inheritance test tables for pg_upgrade
+create table constr_parent (a int);
+create table constr_child (a int) inherits (constr_parent);
+alter table constr_parent add not null a not valid;
+alter table constr_child validate constraint constr_parent_a_not_null;
+EXECUTE get_nnconstraint_info('{constr_parent, constr_child}');
+
+create table constr_parent2 (a int);
+create table constr_child2 () inherits (constr_parent2);
+alter table constr_parent2 add not null a not valid;
+alter table constr_child2 validate constraint constr_parent2_a_not_null;
+EXECUTE get_nnconstraint_info('{constr_parent2, constr_child2}');
+
DEALLOCATE get_nnconstraint_info;
-- end NOT NULL NOT VALID
--
2.39.5
>From 91efc55ab422fcdff83fdc4fb9bd28b17c74c980 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvhe...@alvh.no-ip.org>
Date: Thu, 24 Apr 2025 21:09:22 +0200
Subject: [PATCH 2/2] Unnecessary change
I think we don't have a good reason to do this.
---
src/backend/catalog/pg_constraint.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 70528679e57..6d5f83f9329 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -779,6 +779,17 @@ AdjustNotNullInheritance(Oid relid, AttrNumber attnum,
ereport(ERROR,
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many inheritance parents"));
+
+
+ /*
+ * If the child already has a valid constraint and we are creating
+ * an invalid one with same definition on it. The child's
+ * constraint will remain valid, but can no longer be marked as
+ * local.
+ */
+ if (is_notvalid && conform->convalidated && conform->conenforced)
+ conform->conislocal = false;
+
changed = true;
}
else if (!conform->conislocal)
--
2.39.5
