Re: not null constraints, again - Mailing list pgsql-hackers

On Sat, Aug 31, 2024 at 11:59 AM Alvaro Herrera <alvherre@alvh.no-ip.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.