Re: cataloguing NOT NULL constraints

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Alexander Lakhin <exclusion(at)gmail(dot)com>
Cc: Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, Andrew Bille <andrewbille(at)gmail(dot)com>, Peter Eisentraut <peter(at)eisentraut(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: cataloguing NOT NULL constraints
Date: 2024-04-22 10:22:23
Message-ID: 202404221022.nljgb5dwzppl@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Alexander,

On 2024-Apr-18, Alexander Lakhin wrote:

> 18.04.2024 16:39, Alvaro Herrera wrote:
> > I have pushed a fix which should hopefully fix this problem
> > (d9f686a72e). Please give this a look. Thanks for reporting the issue.
>
> Please look at an assertion failure, introduced with d9f686a72:
> CREATE TABLE t(a int, NOT NULL a NO INHERIT);
> CREATE TABLE t2() INHERITS (t);
>
> ALTER TABLE t ADD CONSTRAINT nna NOT NULL a;
> TRAP: failed Assert("lockmode != NoLock || IsBootstrapProcessingMode() ||
> CheckRelationLockedByMe(r, AccessShareLock, true)"), File: "relation.c",
> Line: 67, PID: 2980258

Ah, of course -- we're missing acquiring locks during the prep phase for
the recursive case of ADD CONSTRAINT. So we just need to add
find_all_inheritors() to do so in the AT_AddConstraint case in
ATPrepCmd(). However these naked find_all_inheritors() call look a bit
ugly to me, so I couldn't resist the temptation of adding a static
function ATLockAllDescendants to clean it up a bit. I'll also add your
script to the tests and push shortly.

> On d9f686a72~1 this script results in:
> ERROR:  cannot change NO INHERIT status of inherited NOT NULL constraint "t_a_not_null" on relation "t"

Right. Now I'm beginning to wonder if allowing ADD CONSTRAINT to mutate
a pre-existing NO INHERIT constraint into a inheritable constraint
(while accepting a constraint name in the command that we don't heed) is
really what we want. Maybe we should throw some error when the affected
constraint is the topmost one, and only accept the inheritance status
change when we're recursing.

Also I just noticed that in 9b581c534186 (which introduced this error
message) I used ERRCODE_DATATYPE_MISMATCH ... Is that really appropriate
here?

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL. This is by far the most pleasant management experience of
any database I've worked on." (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php

Attachment Content-Type Size
0001-Acquire-locks-on-children-before-recursing.patch text/x-diff 3.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alexander Korotkov 2024-04-22 10:31:48 Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Previous Message Sriram RK 2024-04-22 10:12:23 Re: AIX support