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

Thanks Alvaro.

On Mon, Apr 7, 2025 at 2:02 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
wrote:

> On 2025-Apr-05, jian he wrote:
>
> > hi.
> > + /* FIXME use CompactAttribute */
> > Form_pg_attribute att = TupleDescAttr(relation->rd_att,
> i - 1);
> > if (att->attnotnull && att->attnotnullvalid &&
> > !att->attisdropped)
> > {
> > NullTest *ntest = makeNode(NullTest);
> > ntest->arg = (Expr *) makeVar(varno,
> > i,
> > att->atttypid,
> > att->atttypmod,
> > att->attcollation,
> > 0);
> > ntest->nulltesttype = IS_NOT_NULL;
> >
> > CompactAttribute doesn't have {atttypmod, attcollation} information,
> > now it is impossible to use CompactAttribute here,
> > so I removed this FIXME in get_relation_constraints.
>
> Ah, good point. In this new patch, I now consult both
> TupleDescCompactAttr (to consult attnullability) and then TupleDescAttr
> (to get collation etc), because now the information we need is split
> between those two places. It feels a bit nasty TBH.
>
> > i am uncomfortable with the change in
> > 'CREATE TABLE dump_test.test_table_generated'
> > so I only added 'CONSTRAINT NOT NULL / INVALID' tests in
> > 002_pg_dump.pl.
> > so I only added a test case 'CONSTRAINT NOT NULL / INVALID'
> > to 002_pg_dump.pl.
>
> Yeah, good call.
>
> > v7-0001 commit message explains what kind of problem
> > MergeWithExistingConstraint is trying to fix.
>
> Umm. I have done away with this again (and the parts in 0002 where you
> handle the equivalent issue for not-null constraints), because it is
> still not clear to me exactly what fails if you don't have them. I
> suggest that we should deal with this separately, and that a patch to
> deal with it should include a test case that fails if the code fix is
> not present.
>
> > v7-0002 bullet points summary about NOT NULL NOT VALID added to the
> > commit message.
>
> Thanks.
>
> > add a test for CREATE TABLE LIKE.
> > CREATE TABLE LIKE will copy the invalid not-null constraint and will
> become
> > valid, i think this is what we want.
>
> Yep, thanks.
>
> I removed the postgres_fdw changes. What I wrote was untested, and
> failed as soon as I added a trivial test. Needs more thought.
>
>
> I also did some more polish, and as I said in another email, it seems to
> me that this approach is also wrong, and that a better approach is to
> determine the value of CompactAttribute->attnullability using the
> pg_constraint scan at the time when the tupdesc is built. I coded this,
> and it almost works, but there are some spots that fail because some
> tuple descriptors are not built correctly. I'm not sure what to do with
> this at this stage; I would love to see this patch across the finish
> line, but it seems a little late now.
>
>
> Note: naturally, patch 0002 in this series would be squashed with 0001
> for commit. Patch 0003 is not for commit, just to show what the
> problems are. If you run the regression tests without 0003, there are
> surprisingly very few failures, and it's very clear that they are
> because of tuple descriptors that don't have the attnullability flag set
> correctly.
>

I reviewed the patch and found that the regression was failing because the
loop over
attributes in RelationBuildTupleDesc() were not executed correctly.

After applying the fix, I noticed one more test was failing, which turned
out to be related
to nullability handling for temporary tables.

Please find attached the 0003 patch, which addresses both issues.

Thanks,

>
> --
> Álvaro Herrera 48°01'N 7°57'E —
> https://www.EnterpriseDB.com/
> "La grandeza es una experiencia transitoria. Nunca es consistente.
> Depende en gran parte de la imaginación humana creadora de mitos"
> (Irulan)
>

--
Rushabh Lathia