Horton

Thread

Commits

  1. Fix bogus grammar for a CREATE CONSTRAINT TRIGGER error

  2. Fix ALTER TABLE error message

  1. bogus error message for ALTER TABLE ALTER CONSTRAINT

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-04T18:22:22Z

    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/

  2. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    Nathan Bossart <nathandbossart@gmail.com> — 2025-03-04T18:36:21Z

    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

  3. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-04T19:18:12Z

    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)

  4. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-04T21:50:28Z

    =?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

  5. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-06T17:04:04Z

    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)

  6. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-10T17:58:44Z

    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/

  7. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    Amul Sul <sulamul@gmail.com> — 2025-03-11T04:32:33Z

    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

  8. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    jian he <jian.universality@gmail.com> — 2025-03-11T07:00:53Z

    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.

  9. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-11T08:26:43Z

    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)

  10. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-11T10:21:12Z

    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

  11. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    Amul Sul <sulamul@gmail.com> — 2025-03-11T11:08:20Z

    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

  12. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    jian he <jian.universality@gmail.com> — 2025-03-11T13:06:05Z

    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.

  13. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-11T14:46:52Z

    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/

  14. Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-14T11:16:43Z

    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