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

On Fri, Mar 28, 2025 at 3:25 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2025-Mar-24, jian he wrote:
>
> > hi.
> > you may like the attached. it's based on your idea: attnotnullvalid.
>
> This is quite close to what I was thinking, yeah. I noticed a couple of
> bugs however, and ended up cleaning up the whole thing. Here's what I
> have so far. I'm not sure the pg_dump bits are okay (apart from what
> you report below) -- I think it's losing the constraint names, which is
> of course unacceptable.
>

hi.

/*
* Update constraint/default info has_not_null also include invalid
* not-null constraint
*/
this comment needs a period. it should be:
/*
* Update constraint/default info. has_not_null also include invalid
* not-null constraint
*/

gram.y:
/* no NOT VALID support yet */
processCASbits($4, @4, "NOT NULL",
NULL, NULL, NULL, &n->skip_validation,
&n->is_no_inherit, yyscanner);
n->initially_valid = !n->skip_validation;
$$ = (Node *) n;
comment "/* no NOT VALID support yet */" should be removed?

get_relation_constraints:
- if (att->attnotnull && !att->attisdropped)
+ if (att->attnotnull /* && att->attnotnullvalid */ && !att->attisdropped)
looking at how we deal with check constraints,
i think we need
+ if (att->attnotnull && att->attnotnullvalid && !att->attisdropped)

set_attnotnull comments should say something about attnotnullvalid?
since it will change pg_attribute.attnotnullvalid

ATPrepAddPrimaryKey
+ if (!conForm->convalidated)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("not-null constraint \"%s\" of table \"%s\" has not been validated",
+ NameStr(conForm->conname),
+ RelationGetRelationName(rel)),
+ errhint("You will need to use ALTER TABLE ... VALIDATE CONSTRAINT to
validate it."));

I think the error message is less helpful.
Overall, I think we should say that:
to add the primary key on column x requires a validated not-null
constraint on column x.

------------------------------------------------------------------------
i think your patch messed up with pg_constraint.conislocal.
for example:

CREATE TABLE parted (id bigint default 1,id_abc bigint) PARTITION BY LIST (id);
alter TABLE parted add CONSTRAINT dummy_constr not null id not valid;
CREATE TABLE parted_1 (id bigint default 1,id_abc bigint);
alter TABLE parted_1 add CONSTRAINT dummy_constr not null id;
ALTER TABLE parted ATTACH PARTITION parted_1 FOR VALUES IN ('1');

select conrelid::regclass, conname, conislocal
from pg_constraint where conname = 'dummy_constr';

conrelid | conname | conislocal
----------+--------------+------------
parted | dummy_constr | t
parted_1 | dummy_constr | f
(2 rows)

if you do pg_dump, and execute the pg_dump output
pg_dump --no-statistics --clean --table-and-children=*parted*
--no-owner --verbose --column-inserts --file=dump.sql --no-acl

select conrelid::regclass, conname, conislocal
from pg_constraint where conname = 'dummy_constr';
output is

conrelid | conname | conislocal
----------+--------------+------------
parted | dummy_constr | t
parted_1 | dummy_constr | t
(2 rows)

because pg_dump will produce
CREATE TABLE public.parted (id bigint DEFAULT 1, id_abc bigint )
PARTITION BY LIST (id);
CREATE TABLE public.parted_1 ( id bigint DEFAULT 1 CONSTRAINT
dummy_constr NOT NULL, id_abc bigint);
ALTER TABLE public.parted ADD CONSTRAINT dummy_constr NOT NULL id NOT VALID;
the third sql command didn't override the parted_1 constraint
dummy_constr conislocal value.
you may check my code change in AdjustNotNullInheritance