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

Thanks Alvaro.

On Thu, Feb 6, 2025 at 9:58 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
wrote:

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

I am open to suggestions here.

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

Sure, will cover this.

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

Even though we are accumulating invalidnotnulloids, we need the condition
for the invalid not null constraint, as there can me multiple not null
constraint
on the table - mix of valid and invalid.

Initially, I tried re-using the code but it was not very clear, so thought
of
making at separate code for check and not null constraint. That way it's
very clear.

>
> I think it'd be good to have NOT VALID NO INHERIT constraints in the
> tests as well.

Sure, will take care in the new version of the patch.

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

yes, I tested this with the patch and pg_dump doing the right this
for that scenario.

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

Okay, I will add tests to pg_dump/t/002_pg_dump.pl.

regards.
Rushabh Lathia
www.EnterpriseDB.com