Re: cataloguing NOT NULL constraints

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, Peter Eisentraut <peter(at)eisentraut(dot)org>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: cataloguing NOT NULL constraints
Date: 2023-08-21 18:01:01
Message-ID: 20230821180101.u6mb3oubujsgparq@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Okay, so here's another version of this, where I fixed the creation of
NOT NULLs derived from PKs. It turned out that what I was doing wasn't
doing recursion correctly, so for example if you have NOT NULLs in
grand-child tables they would not be marked as inherited from the PK
(thus wrongly droppable). I had to rewrite it to go through ATPrepCmd
and friends; and we had no way to indicate inheritance that way, so I
had to add an "int inhcount" to the Constraint node. (I think it might
be OK to make it just a "bool inherited" instead).

There is one good thing about this, which is that currently
AddRelationNewConstraints() has a strange "bool is_local" parameter
(added by commit cd902b331d, 2008), which is somewhat strange, and which
we could remove to instead use this new Constraint->inhcount mechanism
to pass down the flag.

Also: it turns out that you can do this
CREATE TABLE parent (a int);
CREATE TABLE child (NOT NULL a) INHERITS (parent);

that is, the column has no local definition on the child, but the
constraint does. This required some special fixes but also works
correctly now AFAICT.

On 2023-Aug-16, Peter Eisentraut wrote:

> I have two small patches that you can integrate into your patch set:
>
> The first just changes the punctuation of "Not-null constraints" in the psql
> output to match what the documentation mostly uses.
>
> The second has some changes to ddl.sgml to reflect that not-null constraints
> are now named and can be operated on like other constraints. You might want
> to read that again to make sure it matches your latest intentions, but I
> think it catches all the places that are required to change.

I've incorporated both of those, verbatim for now; I'll give the docs
another look tomorrow.

On 2023-Aug-11, Alvaro Herrera wrote:

> - ALTER TABLE parent ADD PRIMARY KEY
> needs to create NOT NULL constraints in children. I added this, but
> I'm not yet sure it works correctly (for example, if a child already
> has a NOT NULL constraint, we need to bump its inhcount, but we
> don't.)
> - ALTER TABLE parent ADD PRIMARY KEY USING index
> Not sure if this is just as above or needs separate handling
> - ALTER TABLE DROP PRIMARY KEY
> needs to decrement inhcount or drop the constraint if there are no
> other sources for that constraint to exist. I've adjusted the drop
> constraint code to do this.
> - ALTER TABLE INHERIT
> needs to create a constraint on the new child, if parent has PK. Not
> implemented
> - ALTER TABLE NO INHERIT
> needs to delink any constraints (decrement inhcount, possibly drop
> the constraint).
>
> I also need to add tests for those scenarios, because I think there
> aren't any for most of them.

I've added tests for the ones I caught missing, including leaving some
tables to exercise the pg_upgrade side of things.

> There's also another a pg_upgrade problem: we now get spurious ALTER
> TABLE SET NOT NULL commands in a dump after pg_upgrade for the columns
> that get the constraint from a primary key.

I fixed this too.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

Attachment Content-Type Size
v18-0001-Catalog-NOT-NULL-constraints.patch text/x-diff 284.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2023-08-21 19:14:48 Re: CREATE FUNCTION ... SEARCH { DEFAULT | SYSTEM | SESSION }
Previous Message Jacob Champion 2023-08-21 17:49:16 Re: Logging of matching pg_hba.conf entry during auth skips trust auth, potential security issue