Thread: bogus error message for ALTER TABLE ALTER CONSTRAINT
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. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Attachment
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)
Attachment
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/
Attachment
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
Attachment
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