Re: not null constraints, again

Hello, here's a v2 of this patch. I have fixed --I think-- all the
issues you and Tender Wang reported (unless I declined a fix in some
previous email).

It turns out I have not finished cleaning up the regression tests from
now-useless additions, because while doing so last time around I found
some bugs (especially one around comments in not-null constraints, which
weren't being preserved by an ALTER TABLE TYPE command; that required a
new strange hack in RememberConstraintForRebuilding), but also the LIKE
clause again). Also, in this version there's a problem in the
pg_upgrade test, which I hope to fix tomorrow.

This code can also be found at
https://github.com/alvherre/postgres/tree/notnull-init-18
(Please do ignore 89685e691f75309fec882723272c8b17106e6aa2, that was a
merge mistake).

On 2024-Sep-09, jian he wrote:

> 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);

Ah, obvious now that I see it, thanks.

> ============
> 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;

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

Yeah, I think we need to reject NO INHERIT constraints for domains.
I've done so in this new version.

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

True, assuming we reject the case for system columns as you say below.

> 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?

Added explicit rejection here and in a couple of other places.

> foreach(lc, RelationGetNotNullConstraints(RelationGetRelid(relation), false))

> 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).

I'm undecided about this, but seeing that we don't use this pattern
almost anywhere, I've gone ahead and added the extra local variable.

> + /*
> + * 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.

> 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.

Yes, this code is completely unnecessary now. Removed.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/