> > ------------------------------------------------------------------------
> > i think your patch messed up with pg_constraint.conislocal.
> > for example:
> >
> > CREATE TABLE parted (id bigint default 1,id_abc bigint) PARTITION BY LIST
> > (id);
> > alter TABLE parted add CONSTRAINT dummy_constr not null id not valid;
> > CREATE TABLE parted_1 (id bigint default 1,id_abc bigint);
> > alter TABLE parted_1 add CONSTRAINT dummy_constr not null id;
> > ALTER TABLE parted ATTACH PARTITION parted_1 FOR VALUES IN ('1');
>
> It's still not clear to me what to do to fix this problem. But while
> trying to understand it, I had the chance to rework the pg_dump code
> somewhat, so here it is. Feel free to propose fixes on top of this.
we need special code for handing parent is invalid, child is valid
(table inheritance or partitioning).
To do that we need to check if these valid children have invalid
parent constraints or not.
Therefore another ExecuteSqlQuery query execution is needed.
it may cause performance problems for pg_dump, I guess.
There may be other simple ways to do it.
just sharing what i comp up with.
I also attached a sql test file. i tested all the check constraints,
not-null constraints
where parent is invalid while child is valid scarenio.
then i use
``
create table after_dump as
select conrelid::regclass::text, conname, convalidated, coninhcount,
conislocal, conparentid, contype
from pg_constraint
where conrelid::regclass::text = ANY('{...}';
``
to query before and after dumping whether pg_constraint information
remains as is or not.
The attached patch is based on v6-0001-NOT-NULL-NOT-VALID.patch.
It resolves the issue described in [1] and [2], (sql example demo)
where pg_dump restore failed to retain all pg_constraint properties in cases
where the child constraint was valid while the parent was invalid.
[1]
https://postgr.es/m/cacjufxecvsdwsc4j0wo2lf-+qoacsfx_scv-ngzqxwjzpf1...@mail.gmail.com
[2]
https://postgr.es/m/cacjufxgnxtj59wm_qqh_jnq2xc8hqnbjdhaixncs2vr3j_1...@mail.gmail.com
From 5d6da8791aa46d63e872c5a9f463b05bb2d0f9e8 Mon Sep 17 00:00:00 2001
From: jian he <jian.universal...@gmail.com>
Date: Wed, 2 Apr 2025 21:47:22 +0800
Subject: [PATCH v6 1/1] ensure pg_dump table constraint info remain the same
resolve case where parent constraint convalidated is false
and children constraint convalidated is true
all the dumped information is correct.
for example:
CREATE TABLE ttchk (a INTEGER);
ALTER TABLE ttchk ADD CONSTRAINT cc check (a is NOT NULL) NOT VALID;
CREATE TABLE ttchk_child(a INTEGER) INHERITS(ttchk);
we need dump as:
CREATE TABLE ttchk (a INTEGER);
CREATE TABLE public.ttchk_child (a integer, CONSTRAINT cc CHECK ((a IS NOT NULL))) INHERITS (public.ttchk);
ALTER TABLE public.ttchk ADD CONSTRAINT cc CHECK ((a IS NOT NULL)) NOT VALID;
---
src/backend/catalog/heap.c | 21 +++++++--
src/backend/catalog/pg_constraint.c | 9 ++++
src/bin/pg_dump/pg_dump.c | 66 ++++++++++++++++++++++++++-
src/bin/pg_dump/pg_dump.h | 1 +
src/test/regress/expected/inherit.out | 2 +-
5 files changed, 92 insertions(+), 7 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 734e34110fa..0bdcd59fa39 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2839,11 +2839,24 @@ MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr,
{
if (is_local)
con->conislocal = true;
- else if (pg_add_s16_overflow(con->coninhcount, 1,
+ else
+ {
+ if(pg_add_s16_overflow(con->coninhcount, 1,
&con->coninhcount))
- ereport(ERROR,
- errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
- errmsg("too many inheritance parents"));
+ ereport(ERROR,
+ errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("too many inheritance parents"));
+
+ /*
+ * If the children already has a valid constraint and we are
+ * creating an invalid one on it. The children's constraint
+ * will remain valid as is, but it can longer be marked as
+ * local.
+ */
+ if (!is_initially_valid && con->convalidated &&
+ is_enforced && con->conenforced)
+ con->conislocal = false;
+ }
}
if (is_no_inherit)
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index c7ccc5bef32..48ac5a22e66 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -775,6 +775,15 @@ AdjustNotNullInheritance(Relation rel, AttrNumber attnum,
ereport(ERROR,
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many inheritance parents"));
+
+ /*
+ * If the children already has a valid constraint and we are
+ * creating an invalid one on it. The children's constraint will
+ * remain valid as is, but it can longer be marked as local.
+ */
+ if (is_notvalid && conform->convalidated && conform->conenforced)
+ conform->conislocal = false;
+
changed = true;
}
else if (!conform->conislocal)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 96e45f888d4..282e774f3cb 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -401,6 +401,7 @@ static void setupDumpWorker(Archive *AH);
static TableInfo *getRootTableInfo(const TableInfo *tbinfo);
static bool forcePartitionRootLoad(const TableInfo *tbinfo);
static void read_dump_filters(const char *filename, DumpOptions *dopt);
+static bool constr_dump_force_not_seperate(Archive *fout, Oid conrelid, char conkind);
int
@@ -7950,6 +7951,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
constrinfo->conperiod = *(PQgetvalue(res, j, i_conperiod)) == 't';
constrinfo->conislocal = true;
constrinfo->separate = true;
+ constrinfo->force_notseperate = false;
indxinfo[j].indexconstraint = constrinfo->dobj.dumpId;
if (relstats != NULL)
@@ -8167,6 +8169,7 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
constrinfo[j].condeferred = false;
constrinfo[j].conislocal = true;
constrinfo[j].separate = true;
+ constrinfo[j].force_notseperate = false;
/*
* Restoring an FK that points to a partitioned table requires that
@@ -8306,6 +8309,7 @@ getDomainConstraints(Archive *fout, TypeInfo *tyinfo)
constrinfo[i].conislocal = true;
constrinfo[i].separate = !validated;
+ constrinfo[i].force_notseperate = false;
/*
* Make the domain depend on the constraint, ensuring it won't be
@@ -9559,6 +9563,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
int i_consrc;
int i_conislocal;
int i_convalidated;
+ int i_coninhcount;
pg_log_info("finding table check constraints");
@@ -9566,7 +9571,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
appendPQExpBuffer(q,
"SELECT c.tableoid, c.oid, conrelid, conname, "
"pg_catalog.pg_get_constraintdef(c.oid) AS consrc, "
- "conislocal, convalidated "
+ "conislocal, convalidated, coninhcount "
"FROM unnest('%s'::pg_catalog.oid[]) AS src(tbloid)\n"
"JOIN pg_catalog.pg_constraint c ON (src.tbloid = c.conrelid)\n"
"WHERE contype = 'c' "
@@ -9585,6 +9590,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
i_consrc = PQfnumber(res, "consrc");
i_conislocal = PQfnumber(res, "conislocal");
i_convalidated = PQfnumber(res, "convalidated");
+ i_coninhcount = PQfnumber(res, "coninhcount");
/* As above, this loop iterates once per table, not once per row */
curtblindx = -1;
@@ -9653,6 +9659,10 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
constrs[j].dobj.dump = tbinfo->dobj.dump;
+ constrs[j].force_notseperate = false;
+ if(validated && (atoi(PQgetvalue(res, j, i_coninhcount)) > 0))
+ constrs[j].force_notseperate = constr_dump_force_not_seperate(fout, conrelid, 'c');
+
/*
* Mark the constraint as needing to appear before the table
* --- this is so that any other dependencies of the
@@ -9681,6 +9691,54 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
destroyPQExpBuffer(checkoids);
}
+/*
+ * (XXX need refinement)
+ * comments for cases: parent constraint is not valid, child is valid:
+ * child's constraint conislocal is false, pg_dump won't dump it, later creating
+ * parent constraint will also create children's. pg_dump will dump invalidated
+ * constraint seperately.
+ * So: for cases: parent constraint is not valid, child is valid, pu_dump's
+ * output will make parent and childconstraint is not valid.
+ *
+ * To resolve this issue:
+ * The child's (conislocal is false) constraint should be dumped within CREATE
+ * TABLE statement. This ensures ALTER TABLE ADD CONSTRAINT on the parent
+ * cascades to the child, no issues occur.
+ *
+ * the caller should make sure 'conrelid' have inherited constraint!
+ *
+*/
+static bool
+constr_dump_force_not_seperate(Archive *fout, Oid conrelid, char conkind)
+{
+ int64 cnt = 0;
+ PQExpBuffer qs = createPQExpBuffer();
+ PGresult *res;
+
+ appendPQExpBuffer(qs,
+ "SELECT count(*) as cnt\n"
+ "FROM pg_constraint pc\n"
+ "JOIN pg_inherits pi ON pi.inhrelid = pc.conrelid\n"
+ "JOIN pg_constraint pc1 ON pc1.conrelid = pi.inhparent\n"
+ "WHERE pc.contype = '%c' "
+ "AND pc.convalidated AND NOT pc1.convalidated "
+ "AND pc.conrelid = %u ",
+ conkind, conrelid);
+
+ res = ExecuteSqlQuery(fout, qs->data, PGRES_TUPLES_OK);
+
+ Assert(PQntuples(res) == 1);
+
+ cnt = strtoi64(PQgetvalue(res, 0, PQfnumber(res, "cnt")), NULL, 10);
+
+ destroyPQExpBuffer(qs);
+
+ if (cnt > 0)
+ return true;
+ else
+ return false;
+}
+
/*
* Based on the getTableAttrs query's row corresponding to one column, set
* the name and flags to handle a not-null constraint for that column in
@@ -16843,6 +16901,10 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
dopt->binary_upgrade ||
tbinfo->ispartition));
+ if (!print_notnull && tbinfo->numParents > 0 &&
+ tbinfo->notnull_constrs[j] != NULL)
+ print_notnull = constr_dump_force_not_seperate(fout, tbinfo->dobj.catId.oid, 'n');
+
if (print_notnull)
{
if (tbinfo->notnull_constrs[j][0] == '\0')
@@ -16913,7 +16975,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
ConstraintInfo *constr = &(tbinfo->checkexprs[j]);
if (constr->separate ||
- (!constr->conislocal && !tbinfo->ispartition))
+ (!constr->conislocal && !tbinfo->ispartition && !constr->force_notseperate))
continue;
if (actual_atts == 0)
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 195b4495768..1720c621abe 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -518,6 +518,7 @@ typedef struct _constraintInfo
bool conperiod; /* true if the constraint is WITHOUT OVERLAPS */
bool conislocal; /* true if constraint has local definition */
bool separate; /* true if must dump as separate item */
+ bool force_notseperate; /* true if must not dump as separate item */
} ConstraintInfo;
typedef struct _procLangInfo
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index c01e9d5244f..2cb2c482ea5 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1558,7 +1558,7 @@ order by 1, 2;
relname | conname | convalidated | conislocal | coninhcount | connoinherit
-------------------------+----------------------+--------------+------------+-------------+--------------
invalid_check_con | inh_check_constraint | f | t | 0 | f
- invalid_check_con_child | inh_check_constraint | t | t | 1 | f
+ invalid_check_con_child | inh_check_constraint | t | f | 1 | f
(2 rows)
-- We don't drop the invalid_check_con* tables, to test dump/reload with
--
2.34.1
scratch147.sql
Description: application/sql
