Re: not null constraints, again

By the way, the v3 failed applying on Head(d35e293878)
git am v3-0001-Catalog-not-null-constraints.patch
Applying: Catalog not-null constraints
error: patch failed: doc/src/sgml/ref/create_table.sgml:77
error: doc/src/sgml/ref/create_table.sgml: patch does not apply
error: patch failed: src/backend/commands/tablecmds.c:4834
error: src/backend/commands/tablecmds.c: patch does not apply
error: patch failed: src/backend/parser/gram.y:4141
error: src/backend/parser/gram.y: patch does not apply
error: patch failed: src/backend/parser/parse_utilcmd.c:2385
error: src/backend/parser/parse_utilcmd.c: patch does not apply
Patch failed at 0001 Catalog not-null constraints

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> 于2024年9月17日周二 01:47写道：

> Sadly, there were some other time-wasting events that I failed to
> consider, but here's now v3 which has fixed (AFAICS) all the problems
> you reported.
>
> On 2024-Sep-11, jian he wrote:
>
> > after applying your changes.
> >
> > in ATExecAddConstraint, ATAddCheckNNConstraint.
> > ATAddCheckNNConstraint(wqueue, tab, rel,
> > newConstraint, recurse, false, is_readd,
> > lockmode);
> > if passed to ATAddCheckNNConstraint rel is a partitioned table.
> > ATAddCheckNNConstraint itself can recurse to create not-null
> pg_constraint
> > for itself and it's partitions (children table).
> > This is fine as long as we only call ATExecAddConstraint once.
> >
> > but ATExecAddConstraint itself will recurse, it will call
> > the partitioned table and each of the partitions.
>
> Yeah, this is because ATPrepAddPrimaryKey was queueing SetNotNull nodes
> for each column on each children, which is repetitive and causes the
> problem you see. That was a leftover from the previous way we handled
> PKs; we no longer need it to work that way. I have changed it so that
> it queues one constraint addition per column, on the same table that
> receives the PK. It now works correctly as far as I can tell.
>
> Sadly, there's one more pg_dump issue, which causes the pg_upgrade tests
> to fail. The problem is that if you have this sequence (taken from
> constraints.sql):
>
> CREATE TABLE notnull_tbl4 (a INTEGER PRIMARY KEY INITIALLY DEFERRED);
> CREATE TABLE notnull_tbl4_cld2 (PRIMARY KEY (a) DEFERRABLE) INHERITS
> (notnull_tbl4);
>
> this is dumped by pg_dump in this other way:
>
> CREATE TABLE public.notnull_tbl4 (a integer NOT NULL);
> CREATE TABLE public.notnull_tbl4_cld2 () INHERITS (public.notnull_tbl4);
> ALTER TABLE ONLY public.notnull_tbl4_cld2 ADD CONSTRAINT
> notnull_tbl4_cld2_pkey PRIMARY KEY (a) DEFERRABLE;
> ALTER TABLE ONLY public.notnull_tbl4 ADD CONSTRAINT notnull_tbl4_pkey
> PRIMARY KEY (a) DEFERRABLE INITIALLY DEFERRED;
>
> This is almost exactly the same, except that the PK for
> notnull_tbl4_cld2 is created in a separate command ... and IIUC this
> causes the not-null constraint to obtain a different name, or a
> different inheritance characteristic, and then from the
> restored-by-pg_upgrade database, it's dumped by pg_dump separately.
> This is what causes the pg_upgrade test to fail.
>
> Anyway, this made me realize that there is a more general problem, to
> wit, that pg_dump is not dumping not-null constraint names correctly --
> sometimes they just not dumped, which is Not Good. I'll have to look
> into that once more.
>
>
> (Also: there are still a few additional test stanzas in regress/ that
> ought to be removed; also, I haven't re-tested sepgsql, so it's probably
> broken ATM.)
>
> --
> Álvaro Herrera PostgreSQL Developer —
> https://www.EnterpriseDB.com/
>

--
Thanks,
Tender Wang