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