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

On Thu, Mar 20, 2025 at 11:53 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2025-Mar-20, jian he wrote:
>
> > > Is it expected that a child may have VALID constraint but parent has
> > > not valid constraint?
> >
> > but the MergeConstraintsIntoExisting logic is when
> > ALTER TABLE ATTACH PARTITION,
> > it expects the child table to also have an equivalent constraint
> > definition on it.
> > see MergeConstraintsIntoExisting:
> > ereport(ERROR,
> > (errcode(ERRCODE_DATATYPE_MISMATCH),
> > errmsg("child table is missing constraint \"%s\"",
> > NameStr(parent_con->conname))));
> >
> > So I decided not to support it.
>
> > * partitioned table can not have NOT NULL NOT VALID.
>
> I'm not sure I understand what you're saying here. I think a
> partitioned table can be allowed to have a NOT VALID constraint. BUT if
> it does, then all the children must have a corresponding constraint. The
> constraint on children may be valid or may be invalid; the parent
> doesn't force the issue one way or the other. But it has to exist.
> Also, if you run ALTER TABLE VALIDATE CONSTRAINT on the parent, then at
> that point you have to validate that all those corresponding constraints on
> the children are also validated.

* if partitioned table have valid not-null, then partition with
invalid not-null can not attach to the partition tree.
if partitioned table have not valid not-null, we *can* attach a
valid not-null to the partition tree.
(inheritance hierarchy behaves the same).
this part does not require a lot of code changes.
However, to make the pg_dump working with partitioned table we need to
tweak AdjustNotNullInheritance a little bit.

>
> > * one column one NOT NULL, if you want to change status, it's not
> > allowed, it will error out, give you hints.
>
> I think we discussed this already. If you say
> ALTER TABLE .. ALTER COLUMN .. SET NOT NULL
> and an invalid constraint exists, then we can simply validate that
> constraint.
>
> However, if you say
> ALTER TABLE .. ADD CONSTRAINT foobar NOT NULL col;
> and an invalid constraint exists whose name is different from foobar,
> then we should raise an error, because the user's requirement that the
> constraint is named foobar cannot be satisfied. If the constraint is
> named foobar, OR if the user doesn't specify a constraint name
> ALTER TABLE .. ADD NOT NULL col;
> then it's okay to validate that constraint without raising an error.
> The important thing being that the user requirement is satisfied.
>
i changed this accordingly.
ALTER TABLE .. ALTER COLUMN .. SET NOT NULL
will validate not-null and set attnotnull, attinvalidnotnull accordingly.

> > * pg_attribute.attinvalidnotnull meaning: this attnum has a
> > (convalidated == false) NOT NULL pg_constraint entry to it.
> > * if attnotnull is true, then attinvalidnotnull should be false.
> > Conversely, if attinvalidnotnull is true, then attnotnull should be false.
>
> I don't like this. It seems baroque and it will break existing
> applications, because they currently query for attnotnull and assume
> that inserting a null value will work, but in reality it will fail
> because attinvalidnotnull is true (meaning an invalid constraint exists,
> which prevents inserting nulls).
>
> I think the idea should be: attnotnull means that a constraint exists;
> it doesn't imply anything regarding the constraint being valid or not.
> attnotnullvalid will indicate whether the constraint is valid; this
> column can only be true if attnotnull is already true.
>
i basically model NOT NULL NOT VALID == CHECK (x IS NOT NULL).
i think your idea may need more refactoring?
all the "if (attr->attnotnull" need change to "if (attr->attnotnull &&
attr->attnotnullvalid)"
or am i missing something?

Anyway, I will just share my idea first, and will explore your idea later.

in my attached patch, you will only create an not-null not valid
pg_constraint entry
If `if (constr->contype == CONSTR_NOTNULL && constr->skip_validation)`
in ATAddCheckNNConstraint conditions are satisfied.

imho, my approach is less bug-prone, almost no need to refactor current code.
we can even add a assert in InsertPgAttributeTuples:
Assert(!attrs->attinvalidnotnull);

new patch attached:
* Rushabh's pg_dump relation code incorporated into a single one patch.
* pg_dump works fine, mainly by tweak AdjustNotNullInheritance
following the same logic in MergeConstraintsIntoExisting.
if not do it, pg_constraint.conislocal meta info will be wrong.