At Mon, 28 Aug 2023 13:36:00 +0200, Alvaro Herrera <[email protected]>
wrote in
> On 2023-Aug-28, Kyotaro Horiguchi wrote:
>
> > But with these tables:
> >
> > create table p (a int, b int not null default 0);
> > create table c1 (a int, b int not null NO INHERIT default 1) inherits (p);
> >
> > I get:
> >
> > > Not-null constraints:
> > > "c1_b_not_null" NOT NULL "b" *NO INHERIT*
> >
> > Here, "NO INHERIT" is mapped from connoinherit, and conislocal and
> > "coninhcount <> 0" align with "local" and "inherited". For a clearer
> > picuture, those values for c1 are as follows.
>
> Hmm, I think the bug here is that we let you create a constraint in c1
> that is NOINHERIT. If the parent already has one INHERIT constraint
> in that column, then the child must have that one also; it's not
> possible to have both a constraint that inherits and one that doesn't.
Yeah, I had the same question about the coexisting of the two.
> I understand that there are only three possibilities for a NOT NULL
> constraint in a column:
>
> - There's a NO INHERIT constraint. A NO INHERIT constraint is always
> defined locally in that table. In this case, if there is a parent
> relation, then it must either not have a NOT NULL constraint in that
> column, or it may also have a NO INHERIT one. Therefore, it's
> correct to print NO INHERIT and nothing else. We could also print
> "(local)" but I see no point in doing that.
>
> - A constraint comes inherited from one or more parent tables and has no
> local definition. In this case, the constraint always inherits
> (otherwise, the parent wouldn't have given it to this table). So
> printing "(inherited)" and nothing else is correct.
>
> - A constraint can have a local definition and also be inherited. In
> this case, printing "(local, inherited)" is correct.
>
> Have I missed other cases?
Seems correct. I don't see another case given that NO INHERIT is
inhibited when a table has an inherited constraint.
> The NO INHERIT bit is part of the syntax, which is why I put it in
> uppercase and not marked it for translation. The other two are
> informational, so they are translatable.
Given the conditions above, I agree with you.
Attached is the initial version of the patch. It prevents "CREATE
TABLE" from executing if there is an inconsisntent not-null
constraint. Also I noticed that "ALTER TABLE t ADD NOT NULL c NO
INHERIT" silently ignores the "NO INHERIT" part and fixed it.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index b534da7d80..39fbb0f151 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2533,7 +2533,7 @@ AddRelationNewConstraints(Relation rel,
* update its catalog status and we're done.
*/
if (AdjustNotNullInheritance1(RelationGetRelid(rel), colnum,
- cdef->inhcount))
+ cdef->inhcount, cdef->is_no_inherit))
continue;
/*
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 2a725f6280..bd589c0e7c 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -669,7 +669,8 @@ extractNotNullColumn(HeapTuple constrTup)
* If no not-null constraint is found for the column, return false.
*/
bool
-AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count)
+AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
+ bool is_no_inherit)
{
HeapTuple tup;
@@ -681,6 +682,14 @@ AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count)
pg_constraint = table_open(ConstraintRelationId, RowExclusiveLock);
conform = (Form_pg_constraint) GETSTRUCT(tup);
+
+ if (is_no_inherit)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot change NO INHERIT status of inherited NOT NULL constraint \"%s\" on relation \"%s\"",
+ NameStr(conform->conname), get_rel_name(relid)));
+
+
if (count > 0)
conform->coninhcount += count;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 47c900445c..ae5ef6e115 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -350,8 +350,8 @@ static void truncate_check_perms(Oid relid, Form_pg_class reltuple);
static void truncate_check_activity(Relation rel);
static void RangeVarCallbackForTruncate(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
-static List *MergeAttributes(List *schema, List *supers, char relpersistence,
- bool is_partition, List **supconstr,
+static List *MergeAttributes(List *schema, List *supers, List *nnconstr,
+ char relpersistence, bool is_partition, List **supconstr,
List **supnotnulls);
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel);
@@ -873,7 +873,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
* modified by MergeAttributes.)
*/
stmt->tableElts =
- MergeAttributes(stmt->tableElts, inheritOids,
+ MergeAttributes(stmt->tableElts, inheritOids, stmt->nnconstraints,
stmt->relation->relpersistence,
stmt->partbound != NULL,
&old_constraints, &old_notnulls);
@@ -2318,6 +2318,7 @@ storage_name(char c)
* 'schema' is the column/attribute definition for the table. (It's a list
* of ColumnDef's.) It is destructively changed.
* 'supers' is a list of OIDs of parent relations, already locked by caller.
+ * 'child_nnconstr' is a list of not-null constraints on the child relation.
* 'relpersistence' is the persistence type of the table.
* 'is_partition' tells if the table is a partition.
*
@@ -2377,12 +2378,13 @@ storage_name(char c)
*----------
*/
static List *
-MergeAttributes(List *schema, List *supers, char relpersistence,
- bool is_partition, List **supconstr, List **supnotnulls)
+MergeAttributes(List *schema, List *supers, List *child_nnconstr,
+ char relpersistence, bool is_partition, List **supconstr,
+ List **supnotnulls)
{
List *inhSchema = NIL;
List *constraints = NIL;
- List *nnconstraints = NIL;
+ List *super_nnconstr = NIL;
bool have_bogus_defaults = false;
int child_attno;
static Node bogus_marker = {0}; /* marks conflicting defaults */
@@ -2718,7 +2720,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
nn->inhcount = 1;
nn->is_no_inherit = false;
- nnconstraints = lappend(nnconstraints, nn);
+ super_nnconstr = lappend(super_nnconstr, nn);
}
/*
@@ -2807,7 +2809,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
nn->inhcount = 1;
nn->is_no_inherit = false;
- nnconstraints = lappend(nnconstraints, nn);
+ super_nnconstr = lappend(super_nnconstr, nn);
}
}
@@ -2967,7 +2969,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
nn->is_local = false;
nn->inhcount = 1;
- nnconstraints = lappend(nnconstraints, nn);
+ super_nnconstr = lappend(super_nnconstr, nn);
}
free_attrmap(newattmap);
@@ -3096,6 +3098,30 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
errdetail("%s versus %s", def->compression, newdef->compression)));
}
+ /*
+ * Don't allow NO INHERIT when a not-null constraint is
+ * inherited
+ */
+ if (def->is_not_null)
+ {
+ ListCell *lcnnconstr;
+
+ foreach(lcnnconstr, child_nnconstr)
+ {
+ Constraint *con = (Constraint *) lfirst(lcnnconstr);
+ char *child_colname = strVal(linitial(con->keys));
+
+ if (con->is_no_inherit &&
+ strncmp(child_colname, attributeName,
+ NAMEDATALEN) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("cannot define not-null constraint on column \"%s\" with NO INHERIT",
+ attributeName),
+ errdetail("The cloumn has an inherited not-null constraint.")));
+ }
+ }
+
/*
* Merge of not-null constraints = OR 'em together
*/
@@ -3281,7 +3307,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
}
*supconstr = constraints;
- *supnotnulls = nnconstraints;
+ *supnotnulls = super_nnconstr;
return schema;
}
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index f6f5796fe0..f366f8cd14 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -248,7 +248,8 @@ extern char *ChooseConstraintName(const char *name1, const char *name2,
extern HeapTuple findNotNullConstraintAttnum(Oid relid, AttrNumber attnum);
extern HeapTuple findNotNullConstraint(Oid relid, const char *colname);
extern AttrNumber extractNotNullColumn(HeapTuple constrTup);
-extern bool AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count);
+extern bool AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
+ bool is_no_inherit);
extern void AdjustNotNullInheritance(Oid relid, Bitmapset *columns, int count);
extern List *RelationGetNotNullConstraints(Oid relid, bool cooked);
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 6daca12340..7ec00aaebc 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2051,6 +2051,15 @@ Not-null constraints:
Inherits: pp1,
cc1
+-- cannot create table with inconsistent NO INHERIT contraint: fails
+create table cc3 (a2 int not null no inherit) inherits (cc1);
+NOTICE: moving and merging column "a2" with inherited definition
+DETAIL: User-specified column moved to the position of the inherited column.
+ERROR: cannot define not-null constraint on column "a2" with NO INHERIT
+DETAIL: The cloumn has an inherited not-null constraint.
+-- change NO INHERIT status of inherited constraint: no dice, it's inherited
+alter table cc2 add not null a2 no inherit;
+ERROR: cannot change NO INHERIT status of inherited NOT NULL constraint "nn" on relation "cc2"
-- remove constraint from cc2: no dice, it's inherited
alter table cc2 alter column a2 drop not null;
ERROR: cannot drop inherited constraint "nn" of relation "cc2"
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index d8fae92a53..b58a9286f3 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -736,6 +736,12 @@ alter table pp1 alter column f1 set not null;
\d+ cc1
\d+ cc2
+-- cannot create table with inconsistent NO INHERIT contraint: fails
+create table cc3 (a2 int not null no inherit) inherits (cc1);
+
+-- change NO INHERIT status of inherited constraint: no dice, it's inherited
+alter table cc2 add not null a2 no inherit;
+
-- remove constraint from cc2: no dice, it's inherited
alter table cc2 alter column a2 drop not null;
