Re: Support NOT VALID / VALIDATE constraint options for named NOT NULL constraints

On Fri, Apr 25, 2025 at 3:36 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2025-Apr-09, jian he wrote:
>
> > hi.
> >
> > attached patch is for address pg_dump inconsistency
> > when parent is "not null not valid" while child is "not null".
>
> Here's my take on this patch. We don't really need the
> notnull_parent_invalid flag; in flagInhAttrs we can just set "islocal"
> to convince getTableAttrs to print the constraint. This also fixes the
> bug that in getTableAttrs() you handled the case where
> shouldPrintColumn() is true and not the other one.
>

Your patch is simpler than me. we indeed do not need the
notnull_parent_invalid flag.
I am wondering if we need to change the following comments in getTableAttrs.

* We track in notnull_islocal whether the constraint was defined directly
* in this table or via an ancestor, for binary upgrade. flagInhAttrs
* might modify this later for servers older than 18; it's also in charge
* of determining the correct inhcount.

since we also changed notnull_islocal for pg18.

Also do we need to adjust the following comments in determineNotNullFlags?
* In a child table that inherits from a parent already containing NOT NULL
* constraints and the columns in the child don't have their own NOT NULL
* declarations, we suppress printing constraints in the child: the
* constraints are acquired at the point where the child is attached to the
* parent. This is tracked in ->notnull_islocal (which is set in flagInhAttrs
* for servers pre-18).

>
> Looking at the surrounding code in flagInhAttrs I noticed that we're
> mishandling this case:
>
> create table parent1 (a int);
> create table parent2 (a int);
> create table child () inherits (parent1, parent2);
> alter table parent1 add not null a;
> alter table parent2 add not null a not valid;
>
> We print the constraint for table child for no apparent reason.
>
> Patch 0002 is a part of your proposed patch that I don't think we need,
> but I'm open to hearing arguments about why we do, preferrably with some
> test cases.
>

------------
CREATE TABLE inhnn (a int);
insert into inhnn values(NULL);
ALTER TABLE inhnn ADD CONSTRAINT cc not null a NOT VALID;
CREATE TABLE inhnn_cc(a INTEGER) INHERITS(inhnn);
CREATE TABLE inhnn_cc_1(a INTEGER) INHERITS(inhnn_cc, inhnn);
--------
For the above sql scripts, the following query QUERYA, before and
after dump (--clean --table-and-children=*inhnn* )
the results are the same.

select conrelid::regclass::text, conname, convalidated, coninhcount,
conislocal, conparentid, contype
from pg_constraint
where conrelid::regclass::text = ANY('{inhnn, inhnn_cc, inhnn_cc_1}')
order by 1,2;

without patch 0002:

table before_dump;
conrelid | conname | convalidated | coninhcount | conislocal |
conparentid | contype
------------+---------+--------------+-------------+------------+-------------+---------
inhnn | cc | f | 0 | t |
0 | n
inhnn_cc | cc | t | 1 | f |
0 | n
inhnn_cc_1 | cc | t | 2 | f |
0 | n

table after_dump;

conrelid | conname | convalidated | coninhcount | conislocal |
conparentid | contype
------------+---------+--------------+-------------+------------+-------------+---------
inhnn | cc | f | 0 | t |
0 | n
inhnn_cc | cc | t | 1 | t |
0 | n
inhnn_cc_1 | cc | t | 2 | t |
0 | n

pg_dump --no-statistics --clean --table-and-children=*inhnn*
--no-owner --verbose --column-inserts --file=dump.sql --no-acl
in psql execute file "dump.sql", table after_dump is QUERYA's output
using CTAS.

As you can see, "conislocal" is not consistent, maybe in practical it
does not matter,
but here we can make pg_dump, "conislocal" value being consistent.