Re: not null constraints, again

On Sat, Aug 31, 2024 at 11:59 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> Hello
>
> Here I present another attempt at making not-null constraints be
> catalogued. This is largely based on the code reverted at 9ce04b50e120,
> except that we now have a not-null constraint automatically created for
> every column of a primary key, and such constraint cannot be removed
> while the PK exists. Thanks to this, a lot of rather ugly code is gone,
> both in pg_dump and in backend -- in particular the handling of NO
> INHERIT, which was needed for pg_dump.
>
> Noteworthy psql difference: because there are now even more not-null
> constraints than before, the \d+ display would be far too noisy if we
> just let it grow. So here I've made it omit any constraints that
> underlie the primary key. This should be OK since you can't do much
> with those constraints while the PK is still there. If you drop the PK,
> the next \d+ will show those constraints.
>

hi.
my brief review.

create table t1(a int, b int, c int not null, primary key(a, b));
\d+ t1
ERROR: operator is not unique: smallint[] <@ smallint[]
LINE 8: coalesce(NOT ARRAY[at.attnum] <@ (SELECT conkey FROM pg_cata...
^
HINT: Could not choose a best candidate operator. You might need to
add explicit type casts.

the regression test still passed, i have no idea why.
anyway, the following changes make the above ERROR disappear.
also seems more lean.

printfPQExpBuffer(&buf,
/* FIXME the coalesce trick looks silly. What's a better way? */
"SELECT co.conname, at.attname, co.connoinherit, co.conislocal,\n"
"co.coninhcount <> 0\n"
"FROM pg_catalog.pg_constraint co JOIN\n"
"pg_catalog.pg_attribute at ON\n"
"(at.attrelid = co.conrelid AND at.attnum = co.conkey[1])\n"
"WHERE co.contype = 'n' AND\n"
"co.conrelid = '%s'::pg_catalog.regclass AND\n"
"coalesce(NOT ARRAY[at.attnum] <@ (SELECT conkey FROM
pg_catalog.pg_constraint\n"
" WHERE contype = 'p' AND conrelid = '%s'::regclass), true)\n"
"ORDER BY at.attnum",
oid,
oid);

change to

printfPQExpBuffer(&buf,
"SELECT co.conname, at.attname,
co.connoinherit, co.conislocal,\n"
"co.coninhcount <> 0\n"
"FROM pg_catalog.pg_constraint co JOIN\n"
"pg_catalog.pg_attribute at ON\n"
"(at.attrelid = co.conrelid AND
at.attnum = co.conkey[1])\n"
"WHERE co.contype = 'n' AND\n"
"co.conrelid = '%s'::pg_catalog.regclass AND\n"
"NOT EXISTS (SELECT 1 FROM
pg_catalog.pg_constraint co1 where co1.contype = 'p'\n"
"AND at.attnum = any(co1.conkey) AND
co1.conrelid = '%s'::pg_catalog.regclass)\n"
"ORDER BY at.attnum",
oid,
oid);

steal idea from https://stackoverflow.com/a/75614278/15603477
============
create type comptype as (r float8, i float8);
create domain dcomptype1 as comptype not null no inherit;
with cte as (
SELECT oid, conrelid::regclass, conname FROM pg_catalog.pg_constraint
where contypid in ('dcomptype1'::regtype))
select pg_get_constraintdef(oid) from cte;
current output is
NOT NULL

but it's not the same as
CREATE TABLE ATACC1 (a int, not null a no inherit);
with cte as ( SELECT oid, conrelid::regclass, conname FROM
pg_catalog.pg_constraint
where conrelid in ('ATACC1'::regclass))
select pg_get_constraintdef(oid) from cte;
NOT NULL a NO INHERIT

i don't really sure the meaning of "on inherit" in
"create domain dcomptype1 as comptype not null no inherit;"

====================
bold idea. print out the constraint name: violates not-null constraint \"%s\"
for the following code:
ereport(ERROR,
(errcode(ERRCODE_NOT_NULL_VIOLATION),
errmsg("null value in column \"%s\" of
relation \"%s\" violates not-null constraint",
NameStr(att->attname),
RelationGetRelationName(orig_rel)),
val_desc ? errdetail("Failing row contains
%s.", val_desc) : 0,
errtablecol(orig_rel, attrChk)));

====================
in extractNotNullColumn
we can Assert(colnum > 0);

create table t3(a int , b int , c int ,not null a, not null c, not
null b, not null tableoid);
this should not be allowed?

foreach(lc,
RelationGetNotNullConstraints(RelationGetRelid(relation), false))
{
AlterTableCmd *atsubcmd;

atsubcmd = makeNode(AlterTableCmd);
atsubcmd->subtype = AT_AddConstraint;
atsubcmd->def = (Node *) lfirst_node(Constraint, lc);
atsubcmds = lappend(atsubcmds, atsubcmd);
}
forgive me for being hypocritical.
I guess this is not a good coding pattern.
one reason would be: if you do:
=
list *a = RelationGetNotNullConstraints(RelationGetRelid(relation), false);
foreach(lc, a)
=
then you can call pprint(a).

+ /*
+ * If INCLUDING INDEXES is not given and a primary key exists, we need to
+ * add not-null constraints to the columns covered by the PK (except those
+ * that already have one.) This is required for backwards compatibility.
+ */
+ if ((table_like_clause->options & CREATE_TABLE_LIKE_INDEXES) == 0)
+ {
+ Bitmapset *pkcols;
+ int x = -1;
+ Bitmapset *donecols = NULL;
+ ListCell *lc;
+
+ /*
+ * Obtain a bitmapset of columns on which we'll add not-null
+ * constraints in expandTableLikeClause, so that we skip this for
+ * those.
+ */
+ foreach(lc, RelationGetNotNullConstraints(RelationGetRelid(relation), true))
+ {
+ CookedConstraint *cooked = (CookedConstraint *) lfirst(lc);
+
+ donecols = bms_add_member(donecols, cooked->attnum);
+ }
+
+ pkcols = RelationGetIndexAttrBitmap(relation,
+ INDEX_ATTR_BITMAP_PRIMARY_KEY);
+ while ((x = bms_next_member(pkcols, x)) >= 0)
+ {
+ Constraint *notnull;
+ AttrNumber attnum = x + FirstLowInvalidHeapAttributeNumber;
+ String *colname;
+ Form_pg_attribute attForm;
+
+ /* ignore if we already have one for this column */
+ if (bms_is_member(attnum, donecols))
+ continue;
+
+ attForm = TupleDescAttr(tupleDesc, attnum - 1);
+ colname = makeString(pstrdup(NameStr(attForm->attname)));
+ notnull = makeNotNullConstraint(colname);
+
+ cxt->nnconstraints = lappend(cxt->nnconstraints, notnull);
+ }
+ }
this part (" if (bms_is_member(attnum, donecols))" will always be true?
donecols is all not-null attnums, pkcols is pk not-null attnums.
so pkcols info will always be included in donecols.
i placed a "elog(INFO, "%s we are in", __func__);"
above
"attForm = TupleDescAttr(tupleDesc, attnum - 1);"
all regression tests still passed.