bogus error message for ALTER TABLE ALTER CONSTRAINT

On Tue, Mar 04, 2025 at 07:22:22PM +0100, �lvaro Herrera wrote:

I just discovered that trying to set a foreign key as NO INHERIT in
ALTER TABLE ALTER CONSTRAINT returns an absurd error message:

create table pk (a int primary key);
create table fk (a int references pk);

alter table fk alter constraint fk_a_fkey deferrable, alter constraint fk_a_fkey no inherit;
ERROR: ALTER CONSTRAINT statement constraints cannot be marked NO INHERIT

The explanation is that somebody misunderstood what must be given to
processCASbits in 2013. The intended message is:
ERROR: FOREIGN KEY constraints cannot be marked NO INHERIT

Here's the fix along with some additional cleanup.

LGTM

--
nathan

On 2025-Mar-04, Nathan Bossart wrote:

On Tue, Mar 04, 2025 at 07:22:22PM +0100, Álvaro Herrera wrote:

I just discovered that trying to set a foreign key as NO INHERIT in
ALTER TABLE ALTER CONSTRAINT returns an absurd error message:

Here's the fix along with some additional cleanup.

LGTM

Many thanks for the quick look. Pushed now.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Thou shalt check the array bounds of all strings (indeed, all arrays), for
surely where thou typest "foo" someone someday shall type
"supercalifragilisticexpialidocious" (5th Commandment for C programmers)

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:

I just discovered that trying to set a foreign key as NO INHERIT in
ALTER TABLE ALTER CONSTRAINT returns an absurd error message:
create table pk (a int primary key);
create table fk (a int references pk);
alter table fk alter constraint fk_a_fkey deferrable, alter constraint fk_a_fkey no inherit;
ERROR: ALTER CONSTRAINT statement constraints cannot be marked NO INHERIT

The explanation is that somebody misunderstood what must be given to
processCASbits in 2013. The intended message is:
ERROR: FOREIGN KEY constraints cannot be marked NO INHERIT

Hmm. I agree that "ALTER CONSTRAINT statement" is off the
mark here, but I'm not convinced that "FOREIGN KEY" is entirely
on-point either. The grammar has no way of knowing what kind of
constraint is being targeted. I do see that ATExecAlterConstraint
currently rejects every other kind of constraint, but do we need
to think of a more generic phrase?

regards, tom lane

On 2025-Mar-04, Tom Lane wrote:

Hmm. I agree that "ALTER CONSTRAINT statement" is off the
mark here, but I'm not convinced that "FOREIGN KEY" is entirely
on-point either. The grammar has no way of knowing what kind of
constraint is being targeted. I do see that ATExecAlterConstraint
currently rejects every other kind of constraint, but do we need
to think of a more generic phrase?

You're right that this is bogus, and looking what to do about it made me
realize that CREATE CONSTRAINT TRIGGER is also saying bogus things such
as:

create constraint trigger foo after insert on pg_class not valid for each row execute procedure test();
ERROR: TRIGGER constraints cannot be marked NOT VALID

create constraint trigger foo after insert on pg_class no inherit for each row execute procedure test();
ERROR: TRIGGER constraints cannot be marked NO INHERIT

So after mulling over this for a while I came up with the idea of adding
an output struct that processCASbits() can optionally be given and fill,
which indicates which flags were seeing while parsing the options. With
that, each of these two callers can throw more appropriate error messages
when a flag is given that they don't like. This is much better,
though arguably the error messages I propose can be wordsmithed still.
Most callers of processCASbits() don't care, so they just give a NULL
and they get the current behavior.

In the current incantation I just pass a "bool dummy" for the bits that
each production doesn't support; processCASbits throws no error and
instead sets the corresponding flag in the 'seen' struct. However,
another option might be to not pass a dummy at all and just
conditionally not throw any errors when the 'seen' struct is given.
This might be cleaner, but it also feels a bit magical. Any
preferences?

By the way, this also made me realize that the addition of a SET keyword
in the commands
ALTER TABLE .. ALTER CONSTRAINT SET NO INHERIT
ALTER TABLE .. ALTER CONSTRAINT SET INHERIT
could be removed easily by taking advantage of this 'seen' struct, and
we'd remove one production from the grammar (or both new ones, if we add
INHERIT to ConstraintAttributeElem).

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Debido a que la velocidad de la luz es mucho mayor que la del sonido,
algunas personas nos parecen brillantes un minuto antes
de escuchar las pelotudeces que dicen." (Roberto Fontanarrosa)

Hello,

I fleshed this out more fully and I think 0001 is good enough to commit.

I then noticed that constraints on domains are giving bogus error
messages as well, and the situation is easily improved -- see 0002. I'm
not so sure about this one, mainly because test coverage appears scant.
I need to look into this one a bit more.

Thanks

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

On Mon, Mar 10, 2025 at 11:29 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Hello,

I fleshed this out more fully and I think 0001 is good enough to commit.

The approach looks good to me, but instead of adding a CAS_flags struct, could
we use macros like SEEN_DEFERRABILITY(bits), SEEN_ENFORCEABILITY(bits),
etc.? We can simply pass cas_bits to these macros, and to avoid the error
from processCASbits(), we can pass NULL for constrType.

Regards,
Amul

On Tue, Mar 11, 2025 at 1:58 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Hello,

I fleshed this out more fully and I think 0001 is good enough to commit.

I then noticed that constraints on domains are giving bogus error
messages as well, and the situation is easily improved -- see 0002. I'm
not so sure about this one, mainly because test coverage appears scant.
I need to look into this one a bit more.

hi.

this look a little strange?
if (cas_bits & (CAS_NOT_DEFERRABLE) && seen)
seen->seen_deferrability = true;

it should be
if ((cas_bits & CAS_NOT_DEFERRABLE) && seen)
seen->seen_deferrability = true;
?

typedef struct CAS_flags need add to typedefs.list

seems didn't cover "initially immediate" case for domain.
for example:
create domain d_int as int4;
--- the following two cases should fail.
alter domain d_int add constraint nn1 not null initially immediate;
alter domain d_int add constraint cc check(value > 1) initially immediate;

we can add the following into processCASbits to make it error out
if ((cas_bits & CAS_INITIALLY_IMMEDIATE) && seen)
seen->seen_deferrability = true;

create domain d_int as int4;
alter domain d_int add not null no inherit not valid;
ERROR: not-null constraints on domains cannot be marked NOT VALID
LINE 1: alter domain d_int add not null no inherit not valid;
^
If we can report an error like
"ERROR: NOT NULL constraints on domains cannot be marked INHERIT / NOT INHERIT"
That would be great.
just report the first constraint property that is not ok, but seems not doable.

On 2025-Mar-11, Amul Sul wrote:

On Mon, Mar 10, 2025 at 11:29 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I fleshed this out more fully and I think 0001 is good enough to commit.

The approach looks good to me, but instead of adding a CAS_flags struct, could
we use macros like SEEN_DEFERRABILITY(bits), SEEN_ENFORCEABILITY(bits),
etc.? We can simply pass cas_bits to these macros, and to avoid the error
from processCASbits(), we can pass NULL for constrType.

Ah yeah, I thought of this too at first, but didn't actually code it
because I thought it'd be messier. Trying to do it naively doesn't
work, because it's not enough to test whether each bit is true or false
-- what you need to know is whether an option was specified for each
bit, in either direction. So we'd need a separate bitmask, we can't
pass the existing 'bits' mask. And at that point, it's not any better
to have a bitmask, and a stack-allocated struct of booleans is just
easier to write.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos. Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)

On 2025-Mar-11, jian he wrote:

this look a little strange?
if (cas_bits & (CAS_NOT_DEFERRABLE) && seen)
seen->seen_deferrability = true;

it should be
if ((cas_bits & CAS_NOT_DEFERRABLE) && seen)
seen->seen_deferrability = true;

True. And since you mentioned CAS_INITIALLY_IMMEDIATE, it should really
be

/* These are the default values; just report that we saw them */
if ((cas_bits & (CAS_NOT_DEFERRABLE | CAS_INITIALLY_IMMEDIATE)) && seen)
seen->seen_deferrability = true;

typedef struct CAS_flags need add to typedefs.list

True.

seems didn't cover "initially immediate" case for domain.
for example:
create domain d_int as int4;
--- the following two cases should fail.
alter domain d_int add constraint nn1 not null initially immediate;
alter domain d_int add constraint cc check(value > 1) initially immediate;

Yeah, I thought at first you were right, but on further thought, do we
really want to do this? I mean, INITIALLY IMMEDIATE is the default
timing for a constraint, so why should we complain if a user wants to
get a constraint that's declared that way? I'm not sure that we should
do it. Same with NOT DEFERRABLE.
[... looks at the standard doc ...]
And that's indeed what the SQL standard says:

<domain definition> ::=
CREATE DOMAIN <domain name> [ AS ] <predefined type>
[ <default clause> ]
[ <domain constraint>... ]
[ <collate clause> ]

<domain constraint> ::=
[ <constraint name definition> ] <check constraint definition> [ <constraint characteristics> ]

8) For every <domain constraint> specified:
[...]
b) If <constraint characteristics> is not specified, then INITIALLY IMMEDIATE
NOT DEFERRABLE is implicit.

So I think the fix here needs to distinguish those cases and avoid
throwing errors for them.

(Note also it does not say that INITIALLY DEFERRED or DEFERRABLE are
disallowed, which means that we're failing to fully implement the
standard-mandated behavior by prohibiting those.)

create domain d_int as int4;
alter domain d_int add not null no inherit not valid;
ERROR: not-null constraints on domains cannot be marked NOT VALID
LINE 1: alter domain d_int add not null no inherit not valid;
^
If we can report an error like
"ERROR: NOT NULL constraints on domains cannot be marked INHERIT / NOT INHERIT"
That would be great.
just report the first constraint property that is not ok, but seems not doable.

Yeah, I don't think this can be made to work. Maybe we'd have to change
the way ConstraintAttributeSpec is parsed completely.

--
Álvaro Herrera Breisgau, Deutschland — 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

On Tue, Mar 11, 2025 at 1:56 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2025-Mar-11, Amul Sul wrote:

On Mon, Mar 10, 2025 at 11:29 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I fleshed this out more fully and I think 0001 is good enough to commit.

The approach looks good to me, but instead of adding a CAS_flags struct, could
we use macros like SEEN_DEFERRABILITY(bits), SEEN_ENFORCEABILITY(bits),
etc.? We can simply pass cas_bits to these macros, and to avoid the error
from processCASbits(), we can pass NULL for constrType.

Ah yeah, I thought of this too at first, but didn't actually code it
because I thought it'd be messier. Trying to do it naively doesn't
work, because it's not enough to test whether each bit is true or false
-- what you need to know is whether an option was specified for each
bit, in either direction. So we'd need a separate bitmask, we can't
pass the existing 'bits' mask. And at that point, it's not any better
to have a bitmask, and a stack-allocated struct of booleans is just
easier to write.

I was thinking of something like the attached, which includes your
test cases from 0001. Perhaps the macro name could be improved.

Regards,
Amul

On Tue, Mar 11, 2025 at 6:21 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

seems didn't cover "initially immediate" case for domain.
for example:
create domain d_int as int4;
--- the following two cases should fail.
alter domain d_int add constraint nn1 not null initially immediate;
alter domain d_int add constraint cc check(value > 1) initially immediate;

Yeah, I thought at first you were right, but on further thought, do we
really want to do this? I mean, INITIALLY IMMEDIATE is the default
timing for a constraint, so why should we complain if a user wants to
get a constraint that's declared that way? I'm not sure that we should
do it. Same with NOT DEFERRABLE.
[... looks at the standard doc ...]
And that's indeed what the SQL standard says:

<domain definition> ::=
CREATE DOMAIN <domain name> [ AS ] <predefined type>
[ <default clause> ]
[ <domain constraint>... ]
[ <collate clause> ]

<domain constraint> ::=
[ <constraint name definition> ] <check constraint definition> [ <constraint characteristics> ]

8) For every <domain constraint> specified:
[...]
b) If <constraint characteristics> is not specified, then INITIALLY IMMEDIATE
NOT DEFERRABLE is implicit.

So I think the fix here needs to distinguish those cases and avoid
throwing errors for them.

(Note also it does not say that INITIALLY DEFERRED or DEFERRABLE are
disallowed, which means that we're failing to fully implement the
standard-mandated behavior by prohibiting those.)

I don't have a huge opinion though.
but it's better to align CREATE DOMAIN with ALTER DOMAIN.
For example, the following two logic should behave the same.

create domain d_int as int4 constraint nn1 not null initially immediate;
alter domain d_int add constraint nn1 not null initially immediate;

Also if we do not error out, then in the create_domain.sgml, alter_domain.sgml
<synopsis> section we should include these "useless" keywords.

On 2025-Mar-11, jian he wrote:

but it's better to align CREATE DOMAIN with ALTER DOMAIN.
For example, the following two logic should behave the same.

create domain d_int as int4 constraint nn1 not null initially immediate;
alter domain d_int add constraint nn1 not null initially immediate;

Sure, they should.

Also if we do not error out, then in the create_domain.sgml, alter_domain.sgml
<synopsis> section we should include these "useless" keywords.

Yeah, I guess we should do that.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On 2025-Mar-11, Amul Sul wrote:

I was thinking of something like the attached, which includes your
test cases from 0001. Perhaps the macro name could be improved.

FWIW I like this general idea. I don't like the proposed names much
though, especially the abuse of ALL_CAPS; and because they operate on
the given bits themselves rather than the output of processCASbits(), I
would have these checks before doing anything else. (Also, for nicer
code layout I would perhaps make the macros static inline functions.)

I'm going to stay away from this for a bit, as I think this is of
somewhat secondary importance.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Every machine is a smoke machine if you operate it wrong enough."
https://twitter.com/libseybieda/status/1541673325781196801