Feedback-ID: ia2694551:Fastmail
Date: Thu, 6 Feb 2025 17:28:43 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Rushabh Lathia <rushabh.lathia@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Support NOT VALID / VALIDATE constraint options for named NOT
 NULL constraints
Message-ID: <202502061628.duexh6aq7a6p@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAGPqQf0KitkNack4F5CFkFi-9Dqvp29Ro=EpcWt=4_hs-Rt+bQ@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/202502061628.duexh6aq7a6p%40alvherre.pgsql>
Precedence: bulk

Hello Rushabh,

On 2025-Feb-06, Rushabh Lathia wrote:

> Commit 14e87ffa5c543b5f30ead7413084c25f7735039f
> <https://github.com/postgres/postgres/commit/14e87ffa5c543b5f30ead7413084c25f7735039f>
> added the support for named NOT NULL constraints.   We can now support
> the NOT VALID/VALID named NOT NULL constraints.
> 
> This patch supports the NOT VALID and VALIDATE CONSTRAINT for name NOT
> NULL constraints.  In order to achieve this patch,

Thank you very much for working on this.

> 1) Converted the pg_attribute.attnotnull to CHAR type, so that it can
> hold the INVALID flag for the constraint.

This looks good to me.  It'll have implications for client-side queries,
but I think they will need to adapt.  One school of thought says we
should rename the column, so that every tool is forced to think about
the issue and adapt accordingly, instead of only realizing the problem
the first time they break.

> 4) Added related testcases.

Please remember to add test cases for tables with not-valid constraint
that are not dropped at the end.  That way, the pg_upgrade test will try
to process that table and we'll know if the roundtrip via pg_dump works
correctly.

I haven't looked at 0002 too closely, but I think it has the right
shape.

> 3) Support for pg_dump, where we now dumping the INVALID NOT NULL as
> separate from table schemes, just like CHECK Constraints.

I think you copied a little bit too much of the code for check
constraints.  If a constraint is accumulated in invalidnotnulloids, you
already know that it's not validated and needs to be dumped separately.
So your new query doesn't need to bring convalidated (we know it's
false).  This would simplify a few lines in this new code.  Also, the
pg_log_info() line is mistaken about what this block is doing.

I think it'd be good to have NOT VALID NO INHERIT constraints in the
tests as well.  Also consider the case where the child table is created
first with a valid constraint, then the parent table is created later
with a not valid constraint -- if the pg_dump table scans find the child
first, does pg_dump do the right thing or does it try to create the
parent constraint first?  Also, what if the constraint in the child has
a different name from the constraint in the parent?  This should be
pg_dump round-tripped as well.  (I bet there are tons of other corner
cases here that should be verified.)  Please add something to
pg_dump/t/002_pg_dump.pl.

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