From 0e13b13d26e870cb18fe6ecf9f8697ddfcf2c740 Mon Sep 17 00:00:00 2001 From: =?utf8?q?=C3=81lvaro=20Herrera?= Date: Mon, 28 Apr 2025 16:25:06 +0200 Subject: [PATCH] Fix pg_dump for inherited validated not-null constraints MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit When a child constraint is validated and the parent constraint it derives from isn't, pg_dump must be coerced into printing the child constraint; failing to do would result in a dump that restores the constraint as not valid, which would be incorrect. Co-authored-by: jian he Co-authored-by: Álvaro Herrera Reported-by: jian he Message-id: https://postgr.es/m/CACJufxGHNNMc0E2JphUqJMzD3=bwRSuAEVBF5ekgkG8uY0Q3hg@mail.gmail.com --- src/bin/pg_dump/common.c | 26 +++++++++- src/bin/pg_dump/pg_dump.c | 20 ++++++-- src/bin/pg_dump/pg_dump.h | 1 + src/bin/pg_dump/t/002_pg_dump.pl | 59 ++++++++++++++++++++++- src/test/regress/expected/constraints.out | 34 +++++++++++++ src/test/regress/sql/constraints.sql | 18 +++++++ 6 files changed, 149 insertions(+), 9 deletions(-) diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c index 56b6c368acf..aa1589e3331 100644 --- a/src/bin/pg_dump/common.c +++ b/src/bin/pg_dump/common.c @@ -453,8 +453,8 @@ flagInhIndexes(Archive *fout, TableInfo tblinfo[], int numTables) * What we need to do here is: * * - Detect child columns that inherit NOT NULL bits from their parents, so - * that we needn't specify that again for the child. (Versions >= 18 no - * longer need this.) + * that we needn't specify that again for the child. For versions 18 and + * up, this is needed when the parent is NOT VALID and the child isn't. * * - Detect child columns that have DEFAULT NULL when their parents had some * non-null default. In this case, we make up a dummy AttrDefInfo object so @@ -516,6 +516,8 @@ flagInhAttrs(Archive *fout, DumpOptions *dopt, TableInfo *tblinfo, int numTables bool foundDefault; /* Found a default in a parent */ bool foundSameGenerated; /* Found matching GENERATED */ bool foundDiffGenerated; /* Found non-matching GENERATED */ + bool allNotNullsInvalid = true; /* is NOT NULL NOT VALID + * on all parents? */ /* no point in examining dropped columns */ if (tbinfo->attisdropped[j]) @@ -546,6 +548,18 @@ flagInhAttrs(Archive *fout, DumpOptions *dopt, TableInfo *tblinfo, int numTables parent->notnull_constrs[inhAttrInd] != NULL) foundNotNull = true; + /* + * Keep track of whether all the parents that have a + * not-null constraint on this column have it as NOT + * VALID; if they all are, arrange to have it printed for + * this column. If at least one parent has it as valid, + * there's no need. + */ + if (fout->remoteVersion >= 180000 && + parent->notnull_constrs[inhAttrInd] && + !parent->notnull_invalid[inhAttrInd]) + allNotNullsInvalid = false; + foundDefault |= (parentDef != NULL && strcmp(parentDef->adef_expr, "NULL") != 0 && !parent->attgenerated[inhAttrInd]); @@ -571,6 +585,14 @@ flagInhAttrs(Archive *fout, DumpOptions *dopt, TableInfo *tblinfo, int numTables if (fout->remoteVersion < 180000 && foundNotNull) tbinfo->notnull_islocal[j] = false; + /* + * For versions >18, we must print the not-null constraint locally + * for this table even if it isn't really locally defined, but is + * valid for the child and no parent has it as valid. + */ + if (fout->remoteVersion >= 180000 && allNotNullsInvalid) + tbinfo->notnull_islocal[j] = true; + /* * Manufacture a DEFAULT NULL clause if necessary. This breaks * the advice given above to avoid changing state that might get diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 105e917aa7b..e2e7975b34e 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -9099,8 +9099,8 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) * * We track in notnull_islocal whether the constraint was defined directly * in this table or via an ancestor, for binary upgrade. flagInhAttrs - * might modify this later for servers older than 18; it's also in charge - * of determining the correct inhcount. + * might modify this later; that routine is also in charge of determining + * the correct inhcount. */ if (fout->remoteVersion >= 180000) appendPQExpBufferStr(q, @@ -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 *)); @@ -9708,8 +9709,10 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) * constraints and the columns in the child don't have their own NOT NULL * declarations, we suppress printing constraints in the child: the * constraints are acquired at the point where the child is attached to the - * parent. This is tracked in ->notnull_islocal (which is set in flagInhAttrs - * for servers pre-18). + * parent. This is tracked in ->notnull_islocal; for servers pre-18 this is + * set not here but in flagInhAttrs. That flag is also used when the + * constraint was validated in a child but all its parent have it as NOT + * VALID. * * Any of these constraints might have the NO INHERIT bit. If so we set * ->notnull_noinh and NO INHERIT will be printed by dumpTableSchema. @@ -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..55d892d9c16 100644 --- a/src/bin/pg_dump/t/002_pg_dump.pl +++ b/src/bin/pg_dump/t/002_pg_dump.pl @@ -1118,10 +1118,21 @@ 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_2 ( + col1 int NOT NULL); + 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); + CREATE TABLE dump_test.test_table_nn_chld3 ( + ) INHERITS (dump_test.test_table_nn, dump_test.test_table_nn_2); + 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 +1146,50 @@ 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 NOT NULL / NOT VALID (child3)' => { + regexp => qr/^ + \QCREATE TABLE dump_test.test_table_nn_chld3 (\E\n + ^\Q)\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 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 eba4c0be0d8..ad6aaab7385 100644 --- a/src/test/regress/expected/constraints.out +++ b/src/test/regress/expected/constraints.out @@ -1625,6 +1625,40 @@ 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) + +create table constr_parent3 (a int not null); +create table constr_child3 () inherits (constr_parent2, constr_parent3); +NOTICE: merging multiple inherited definitions of column "a" +EXECUTE get_nnconstraint_info('{constr_parent3, constr_child3}'); + tabname | conname | convalidated | conislocal | coninhcount +----------------+---------------------------+--------------+------------+------------- + constr_child3 | constr_parent2_a_not_null | t | f | 2 + constr_parent3 | constr_parent3_a_not_null | t | 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..337baab7ced 100644 --- a/src/test/regress/sql/constraints.sql +++ b/src/test/regress/sql/constraints.sql @@ -979,6 +979,24 @@ 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}'); + +create table constr_parent3 (a int not null); +create table constr_child3 () inherits (constr_parent2, constr_parent3); +EXECUTE get_nnconstraint_info('{constr_parent3, constr_child3}'); + DEALLOCATE get_nnconstraint_info; -- end NOT NULL NOT VALID -- 2.30.2