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

On Thu, Feb 27, 2025 at 3:25 PM Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:

> Thank Alvaro for the fixup patch.
>
>
>
>
> On Fri, Feb 21, 2025 at 11:43 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
> wrote:
>
>> Hello,
>>
>> Thanks!
>>
>> I noticed a typo 'constrint' in several places; fixed in the attached.
>>
>> I discovered that this sequence, taken from added regression tests,
>>
>> CREATE TABLE notnull_tbl1 (a int);
>> ALTER TABLE notnull_tbl1 ADD CONSTRAINT nn_parent not null a not valid;
>> CREATE TABLE notnull_chld (a int);
>> ALTER TABLE notnull_chld ADD CONSTRAINT nn_child not null a not valid;
>> ALTER TABLE notnull_chld INHERIT notnull_tbl1;
>> ALTER TABLE notnull_tbl1 VALIDATE CONSTRAINT nn_parent;
>>
>> does mark the constraint as validated in the child, but only in
>> pg_constraint -- pg_attribute continues to be marked as 'i', so if you
>> try to use it for a PK, it fails:
>>
>> alter table notnull_chld add constraint foo primary key (a);
>> ERROR:  column "a" of table "notnull_chld" is marked as NOT VALID NOT
>> NULL constrint
>>
>> I thought that was going to be a quick fix, so I tried to do so; since
>> we already have a function 'set_attnotnull', I thought it was the
>> perfect tool to changing attnotnull.  However, it's not great, because
>> since that part of the code is already doing the validation, I don't
>> want it to queue the validation again, so the API needs a tweak; I
>> changed it to receiving separately which new value to update attnotnull
>> to, and whether to queue validation.  With that change it works
>> correctly, but it is a bit ugly at the callers' side.  Maybe it works to
>> pass two booleans instead?  Please have a look at whether that can be
>> improved.
>>
>
> I haven't given much thought to improving the API, but I'll look into it
> now. Also,
> I'm considering renaming AdjustNotNullInheritance() since it now also
> checks for invalid NOT NULL constraints. What do you think?
>

I adjusted the set_attnotnull() API and removed the added queue_validation
parameter.  Rather, the function start using wqueue input parameter as a
check.
If wqueue is NULL, skip the queue_validation.  Attaching patch here, but not
sure how clear it is, in term of understanding the API.  Your
thoughts/inputs?

Looking further for AdjustNotNullInheritance() I don't see need of rename
this
API as it's just adding new error check now for an existing NOT NULL
constraint.

JFYI, I can reproduce the failure Ashutosh Bapat reported, while running
the pg_upgrade test through meson commands.  I am investigating that
further.


Thanks,
Rushabh Lathia