Re: Support NOT VALID / VALIDATE constraint options for named NOT NULL constraints - Mailing list pgsql-hackers

On Fri, Apr 25, 2025 at 3:36 AM Alvaro Herrera <alvherre@alvh.no-ip.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.