Re: cataloguing NOT NULL constraints

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>, Pg Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: cataloguing NOT NULL constraints
Date: 2022-08-31 22:19:10
Message-ID: 20220831221910.uujmlltbaazplqmr@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

So I was wrong in thinking that "this case was simple to implement" as I
replied upthread. Doing that actually required me to rewrite large
parts of the patch. I think it ended up being a good thing, because in
hindsight the approach I was using was somewhat bogus anyway, and the
current one should be better. Please find it attached.

There are still a few problems, sadly. Most notably, I ran out of time
trying to fix a pg_upgrade issue with pg_dump in binary-upgrade mode.
I have to review that again, but I think it'll need a deeper rethink of
how we pg_upgrade inherited constraints. So the pg_upgrade tests are
known to fail. I'm not aware of any other tests failing, but I'm sure
the cfbot will prove me wrong.

I reluctantly added a new ALTER TABLE subcommand type, AT_SetAttNotNull,
to allow setting pg_attribute.attnotnull without adding a CHECK
constraint (only used internally). I would like to find a better way to
go about this, so I may remove it again, therefore it's not fully
implemented.

There are *many* changed regress expect files and I didn't carefully vet
all of them. Mostly it's the addition of CHECK constraints in the
footers of many \d listings and stuff like that. At a quick glance they
appear valid, but I need to review them more carefully still.

We've had pg_constraint.conparentid for a while now, but for some
constraints we continue to use conislocal/coninhcount. I think we
should get rid of that and rely on conparentid completely.

An easily fixed issue is that of constraint naming.
ChooseConstraintName has an argument for passing known constraint names,
but this patch doesn't use it and it must.

One issue that I don't currently know how to fix, is the fact that we
need to know whether a column is a row type or not (because they need a
different null test). At table creation time that's easy to know,
because we have the descriptor already built by the time we add the
constraints; but if you do ALTER TABLE .. ADD COLUMN .., ADD CONSTRAINT
then we don't.

Some ancient code comments suggest that allowing a child table's NOT
NULL constraint acquired from parent shouldn't be independently
droppable. This patch doesn't change that, but it's easy to do if we
decide to. However, that'd be a compatibility break, so I'd rather not
do it in the same patch that introduces the feature.

Overall, there's a lot more work required to get this to a good shape.
That said, I think it's the right direction.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)

Attachment Content-Type Size
notnull-constraints-1.patch text/x-diff 165.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2022-08-31 22:25:01 Solaris "sed" versus pre-v13 plpython tests
Previous Message Peter Geoghegan 2022-08-31 22:05:52 Re: New strategies for freezing, advancing relfrozenxid early