Обсуждение: ALTER TABLE ALTER CONSTRAINT misleading error message
hi.
create table t(a int, constraint cc check(a = 1));
ALTER TABLE t ALTER CONSTRAINT cc not valid;
ERROR: FOREIGN KEY constraints cannot be marked NOT VALID
LINE 1: ALTER TABLE t ALTER CONSTRAINT cc not valid;
^
the error message seems misleading,
should we consider it as a bug for pg18?
the entry point is in gram.y, following part:
| ALTER CONSTRAINT name ConstraintAttributeSpec
{
AlterTableCmd *n = makeNode(AlterTableCmd);
ATAlterConstraint *c = makeNode(ATAlterConstraint);
n->subtype = AT_AlterConstraint;
n->def = (Node *) c;
c->conname = $3;
if ($4 & (CAS_NOT_ENFORCED | CAS_ENFORCED))
c->alterEnforceability = true;
if ($4 & (CAS_DEFERRABLE | CAS_NOT_DEFERRABLE |
CAS_INITIALLY_DEFERRED | CAS_INITIALLY_IMMEDIATE))
c->alterDeferrability = true;
if ($4 & CAS_NO_INHERIT)
c->alterInheritability = true;
processCASbits($4, @4, "FOREIGN KEY",
&c->deferrable,
&c->initdeferred,
&c->is_enforced,
NULL,
&c->noinherit,
yyscanner);
$$ = (Node *) n;
}
jian he <jian.universality@gmail.com> 于2025年5月28日周三 17:10写道：
hi.
create table t(a int, constraint cc check(a = 1));
ALTER TABLE t ALTER CONSTRAINT cc not valid;
ERROR: FOREIGN KEY constraints cannot be marked NOT VALID
LINE 1: ALTER TABLE t ALTER CONSTRAINT cc not valid;
^
the error message seems misleading,
should we consider it as a bug for pg18?
the entry point is in gram.y, following part:
It looks like a bug, because constraint cc is not a FOREIGN KEY constraint.
Thanks,
Tender Wang
On 2025-May-28, jian he wrote: > hi. > > create table t(a int, constraint cc check(a = 1)); > ALTER TABLE t ALTER CONSTRAINT cc not valid; > ERROR: FOREIGN KEY constraints cannot be marked NOT VALID > LINE 1: ALTER TABLE t ALTER CONSTRAINT cc not valid; > ^ > > the error message seems misleading, We discussed this already, didn't we? There's a thread with IIRC three proposed patches for this. I think I liked this one the most: https://postgr.es/m/CAAJ_b97hd-jMTS7AjgU6TDBCzDx_KyuKxG+K-DtYmOieg+giyQ@mail.gmail.com -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Computing is too important to be left to men." (Karen Spärck Jones)
On Wed, May 28, 2025 at 7:59 PM Álvaro Herrera <alvherre@kurilemu.de> wrote: > > On 2025-May-28, jian he wrote: > > > hi. > > > > create table t(a int, constraint cc check(a = 1)); > > ALTER TABLE t ALTER CONSTRAINT cc not valid; > > ERROR: FOREIGN KEY constraints cannot be marked NOT VALID > > LINE 1: ALTER TABLE t ALTER CONSTRAINT cc not valid; > > ^ > > > > the error message seems misleading, > > We discussed this already, didn't we? There's a thread with IIRC three > proposed patches for this. I think I liked this one the most: > > https://postgr.es/m/CAAJ_b97hd-jMTS7AjgU6TDBCzDx_KyuKxG+K-DtYmOieg+giyQ@mail.gmail.com > for ALTER CONSTRAINT, we already handled most error cases in ATExecAlterConstraint. if (cmdcon->alterDeferrability && currcon->contype != CONSTRAINT_FOREIGN) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key constraint", cmdcon->conname, RelationGetRelationName(rel)))); if (cmdcon->alterEnforceability && currcon->contype != CONSTRAINT_FOREIGN) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot alter enforceability of constraint \"%s\" of relation \"%s\"", cmdcon->conname, RelationGetRelationName(rel)))); if (cmdcon->alterInheritability && currcon->contype != CONSTRAINT_NOTNULL) ereport(ERROR, errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("constraint \"%s\" of relation \"%s\" is not a not-null constraint", cmdcon->conname, RelationGetRelationName(rel))); but ATExecAlterConstraint didn't handle "ALTER CONSTRAINT NOT VALID", it was handled in processCASbits. so the attached minimum patch (extract from v2-0001-trial.patch) is fine for PG18, IMHO.
Вложения
On 2025/06/02 12:13, jian he wrote: > On Wed, May 28, 2025 at 7:59 PM Álvaro Herrera <alvherre@kurilemu.de> wrote: >> >> On 2025-May-28, jian he wrote: >> >>> hi. >>> >>> create table t(a int, constraint cc check(a = 1)); >>> ALTER TABLE t ALTER CONSTRAINT cc not valid; >>> ERROR: FOREIGN KEY constraints cannot be marked NOT VALID >>> LINE 1: ALTER TABLE t ALTER CONSTRAINT cc not valid; >>> ^ >>> >>> the error message seems misleading, I also ran into this issue while testing constraints with NOT VALID. >> We discussed this already, didn't we? There's a thread with IIRC three >> proposed patches for this. I think I liked this one the most: >> >> https://postgr.es/m/CAAJ_b97hd-jMTS7AjgU6TDBCzDx_KyuKxG+K-DtYmOieg+giyQ@mail.gmail.com >> > > for ALTER CONSTRAINT, > we already handled most error cases in ATExecAlterConstraint. > > if (cmdcon->alterDeferrability && currcon->contype != CONSTRAINT_FOREIGN) > ereport(ERROR, > (errcode(ERRCODE_WRONG_OBJECT_TYPE), > errmsg("constraint \"%s\" of relation \"%s\" is not a > foreign key constraint", > cmdcon->conname, RelationGetRelationName(rel)))); > if (cmdcon->alterEnforceability && currcon->contype != CONSTRAINT_FOREIGN) > ereport(ERROR, > (errcode(ERRCODE_WRONG_OBJECT_TYPE), > errmsg("cannot alter enforceability of constraint > \"%s\" of relation \"%s\"", > cmdcon->conname, RelationGetRelationName(rel)))); > if (cmdcon->alterInheritability && > currcon->contype != CONSTRAINT_NOTNULL) > ereport(ERROR, > errcode(ERRCODE_WRONG_OBJECT_TYPE), > errmsg("constraint \"%s\" of relation \"%s\" is not a > not-null constraint", > cmdcon->conname, RelationGetRelationName(rel))); > > but ATExecAlterConstraint didn't handle "ALTER CONSTRAINT NOT VALID", > it was handled in processCASbits. > > so the attached minimum patch (extract from v2-0001-trial.patch) > is fine for PG18, IMHO. + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot alter constraint validity"), Since ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID isn't supported, how about making the error message more specific? For example: "ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported" This would make it clearer to users what exactly isn't allowed. Regards, -- Fujii Masao NTT DATA Japan Corporation
On Wed, Jun 11, 2025 at 10:20 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > >> We discussed this already, didn't we? There's a thread with IIRC three > >> proposed patches for this. I think I liked this one the most: > >> > >> https://postgr.es/m/CAAJ_b97hd-jMTS7AjgU6TDBCzDx_KyuKxG+K-DtYmOieg+giyQ@mail.gmail.com > >> > > > > for ALTER CONSTRAINT, > > we already handled most error cases in ATExecAlterConstraint. > > > > if (cmdcon->alterDeferrability && currcon->contype != CONSTRAINT_FOREIGN) > > ereport(ERROR, > > (errcode(ERRCODE_WRONG_OBJECT_TYPE), > > errmsg("constraint \"%s\" of relation \"%s\" is not a > > foreign key constraint", > > cmdcon->conname, RelationGetRelationName(rel)))); > > if (cmdcon->alterEnforceability && currcon->contype != CONSTRAINT_FOREIGN) > > ereport(ERROR, > > (errcode(ERRCODE_WRONG_OBJECT_TYPE), > > errmsg("cannot alter enforceability of constraint > > \"%s\" of relation \"%s\"", > > cmdcon->conname, RelationGetRelationName(rel)))); > > if (cmdcon->alterInheritability && > > currcon->contype != CONSTRAINT_NOTNULL) > > ereport(ERROR, > > errcode(ERRCODE_WRONG_OBJECT_TYPE), > > errmsg("constraint \"%s\" of relation \"%s\" is not a > > not-null constraint", > > cmdcon->conname, RelationGetRelationName(rel))); > > > > but ATExecAlterConstraint didn't handle "ALTER CONSTRAINT NOT VALID", > > it was handled in processCASbits. > > > > so the attached minimum patch (extract from v2-0001-trial.patch) > > is fine for PG18, IMHO. > > + ereport(ERROR, > + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > + errmsg("cannot alter constraint validity"), > > Since ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID isn't supported, > how about making the error message more specific? For example: > > "ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported" > > This would make it clearer to users what exactly isn't allowed. > errmsg("cannot alter constraint validity") can mean ALTER TABLE ... ALTER CONSTRAINT ... VALID ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID even though ALTER TABLE ... ALTER CONSTRAINT ... VALID would yield syntax errors. message saying ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID would make it more explicit. Hence changed to: errmsg("ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported")
Вложения
On 2025/06/13 16:10, jian he wrote: > On Wed, Jun 11, 2025 at 10:20 PM Fujii Masao > <masao.fujii@oss.nttdata.com> wrote: >> >>>> We discussed this already, didn't we? There's a thread with IIRC three >>>> proposed patches for this. I think I liked this one the most: >>>> >>>> https://postgr.es/m/CAAJ_b97hd-jMTS7AjgU6TDBCzDx_KyuKxG+K-DtYmOieg+giyQ@mail.gmail.com >>>> >>> >>> for ALTER CONSTRAINT, >>> we already handled most error cases in ATExecAlterConstraint. >>> >>> if (cmdcon->alterDeferrability && currcon->contype != CONSTRAINT_FOREIGN) >>> ereport(ERROR, >>> (errcode(ERRCODE_WRONG_OBJECT_TYPE), >>> errmsg("constraint \"%s\" of relation \"%s\" is not a >>> foreign key constraint", >>> cmdcon->conname, RelationGetRelationName(rel)))); >>> if (cmdcon->alterEnforceability && currcon->contype != CONSTRAINT_FOREIGN) >>> ereport(ERROR, >>> (errcode(ERRCODE_WRONG_OBJECT_TYPE), >>> errmsg("cannot alter enforceability of constraint >>> \"%s\" of relation \"%s\"", >>> cmdcon->conname, RelationGetRelationName(rel)))); >>> if (cmdcon->alterInheritability && >>> currcon->contype != CONSTRAINT_NOTNULL) >>> ereport(ERROR, >>> errcode(ERRCODE_WRONG_OBJECT_TYPE), >>> errmsg("constraint \"%s\" of relation \"%s\" is not a >>> not-null constraint", >>> cmdcon->conname, RelationGetRelationName(rel))); >>> >>> but ATExecAlterConstraint didn't handle "ALTER CONSTRAINT NOT VALID", >>> it was handled in processCASbits. >>> >>> so the attached minimum patch (extract from v2-0001-trial.patch) >>> is fine for PG18, IMHO. >> >> + ereport(ERROR, >> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), >> + errmsg("cannot alter constraint validity"), >> >> Since ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID isn't supported, >> how about making the error message more specific? For example: >> >> "ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported" >> >> This would make it clearer to users what exactly isn't allowed. >> > > errmsg("cannot alter constraint validity") > can mean > ALTER TABLE ... ALTER CONSTRAINT ... VALID > ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID > > even though ALTER TABLE ... ALTER CONSTRAINT ... VALID would yield > syntax errors. > message saying ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID > would make it more explicit. > > Hence changed to: > errmsg("ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported") Thanks for updating the patch! I had overlooked that commands other than ALTER TABLE can also trigger this error. So mentioning just ALTER TABLE ... might be a bit misleading. Would it be better to use a message like "ALTER action ALTER CONSTRAINT ... NOT VALID is not supported", similar to what we already do in tablecmd.c? Regards, -- Fujii Masao NTT DATA Japan Corporation
On Fri, Jun 27, 2025 at 2:11 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > I had this concern because other commands, like ALTER SEQUENCE ALTER CONSTRAINT NOT VALID, > can also hit this error, and seeing an error message that starts with ALTER TABLE ... > in that context can be confusing. That's why I thought a message like: > > ERROR: ALTER action ALTER CONSTRAINT ... NOT VALID is not supported > > would be clearer. > > However, most ALTER ... commands (other than ALTER TABLE) don't support ALTER CONSTRAINT > at all, not just the NOT VALID clause. So in those cases, I think we should use > the existing error handling for other commands and emit an error like: > > ERROR: ALTER action ALTER CONSTRAINT cannot be performed on relation ... > > Only in the case of ALTER TABLE should we produce a more specific message, like: > > ERROR: ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported > > To make this distinction, I just started thinking it's better to raise the error > in ATExecAlterConstraint() rather than in gram.y. I've attached a draft patch that > follows this approach. > I was thinking ALTER SEQUENCE ALTER CONSTRAINT NOT VALID is a corner case, maybe not worth addressing. With your patch, the error handling in ATExecAlterConstraint. ALTER SEQUENCE ALTER CONSTRAINT NOT VALID behave the same as the master. The only loss is that the error position would not print out, I guess that is fine. one typo: + /* + * Mark whether NOT VALID was specified. If true, an error + * will be raised later in ATExecAlterConstraint(). + * processCASbits() is not used to set skip_validation, + * as it may set it to true for unrelated reasons, even if + * NOT VALID wan't specified. + */ + if ($4 & CAS_NOT_VALID) + c->skip_validation = true; "wan't" should be "wasn't". Other than that, it looks good to me.
On 2025-06-27, Fujii Masao wrote: > To make this distinction, I just started thinking it's better to raise > the error > in ATExecAlterConstraint() rather than in gram.y. I've attached a draft > patch that > follows this approach. Hmm I don't like this very much, it feels very kludgy. I think if we want to consider this in scope for pg18 I would muchprefer to use the patch I mentioned near the beginning of the thread.
On 2025/06/27 22:30, Álvaro Herrera wrote:
> On 2025-06-27, Fujii Masao wrote:
>
>> To make this distinction, I just started thinking it's better to raise
>> the error
>> in ATExecAlterConstraint() rather than in gram.y. I've attached a draft
>> patch that
>> follows this approach.
>
> Hmm I don't like this very much, it feels very kludgy. I think if we want to consider this in scope for pg18 I would
muchprefer to use the patch I mentioned near the beginning of the thread.
Are you referring to v2-0001-trial.patch proposed at [1]?
Regarding this patch:
- c->alterDeferrability = true;
- processCASbits($4, @4, "FOREIGN KEY",
+ processCASbits($4, @4, NULL,
&c->deferrable,
&c->initdeferred,
NULL, NULL, NULL, yyscanner);
+ c->alterDeferrability = ALTER_DEFERRABILITY($4);
+ /* cannot (currently) be changed by this syntax: */
+ if (ALTER_ENFORCEABILITY($4))
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot alter constraint enforceability"),
+ parser_errposition(@4));
+ if (ALTER_VALID($4))
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot alter constraint validity"),
+ parser_errposition(@4));
+ if (ALTER_INHERIT($4))
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot alter constraint inheritability"),
+ parser_errposition(@4));
This patch raises an error if ENFORCED, NOT ENFORCED, INHERIT, or NO INHERIT
is specified. But those options are currently accepted, so these checks seem
unnecessary for now.
Also, the patch raises an error for NOT VALID after calling processCASbits(),
there's no need to call processCASbits() in the first place. It would be
cleaner to check for NOT VALID and raise an error before calling it.
Jian He already proposed this approach in a patch at [2].
{
if (deferrable)
*deferrable = true;
- else
+ else if (constrType)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
/* translator: %s is CHECK, UNIQUE, or similar */
If the NOT VALID check is moved before processCASbits(), the above changes
inside processCASbits() also become unnecessary. Jian's patch doesn't change
processCASbits() this way.
It looks like Jian's patch at [2] is an updated version of the one you referred to,
so you may agree with that approach. Thought?
Just one note: Jian's patch doesn't handle the same issue for TRIGGER case,
so that part might still need to be addressed.
Regards,
[1] https://www.postgresql.org/message-id/CAAJ_b97hd-jMTS7AjgU6TDBCzDx_KyuKxG+K-DtYmOieg+giyQ@mail.gmail.com
[2] https://postgr.es/m/CACJufxH9krMV-rJkC1J=Jvy_FAO_NRVXGMV+DSNm2saHjbuw8Q@mail.gmail.com
--
Fujii Masao
NTT DATA Japan Corporation
On 2025-Jun-27, Fujii Masao wrote: > On 2025/06/27 22:30, Álvaro Herrera wrote: > > On 2025-06-27, Fujii Masao wrote: > > > > > To make this distinction, I just started thinking it's better to raise > > > the error > > > in ATExecAlterConstraint() rather than in gram.y. I've attached a draft > > > patch that > > > follows this approach. > > > > Hmm I don't like this very much, it feels very kludgy. I think if we want to consider this in scope for pg18 I wouldmuch prefer to use the patch I mentioned near the beginning of the thread. > > Are you referring to v2-0001-trial.patch proposed at [1]? Yeah. > This patch raises an error if ENFORCED, NOT ENFORCED, INHERIT, or NO INHERIT > is specified. But those options are currently accepted, so these checks seem > unnecessary for now. Sure, the patch was developed before those options were added, so I meant to reference the general approach rather than the specifics. > Also, the patch raises an error for NOT VALID after calling processCASbits(), > there's no need to call processCASbits() in the first place. It would be > cleaner to check for NOT VALID and raise an error before calling it. Yeah, I remember having this in mind when I read Amul's patch back in the day, too. > Jian He already proposed this approach in a patch at [2]. > > It looks like Jian's patch at [2] is an updated version of the one you referred to, > so you may agree with that approach. Thought? Ah, okay, yes I like this approach better. > Just one note: Jian's patch doesn't handle the same issue for TRIGGER > case, so that part might still need to be addressed. Okay, here's my take on this, wherein I reworded the proposed error message. I also handled the NOT VALID case of a constraint trigger; maybe my patch is too focused on that specific bit and instead we should handle also NO INHERIT and NOT ENFORCED cases, not really sure (it's certainly not an important part of this patch). -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Entristecido, Wutra (canción de Las Barreras) echa a Freyr a rodar y a nosotros al mar"
Вложения
On 2025-Jun-30, Álvaro Herrera wrote: > > Just one note: Jian's patch doesn't handle the same issue for TRIGGER > > case, so that part might still need to be addressed. > > Okay, here's my take on this, wherein I reworded the proposed error > message. I also handled the NOT VALID case of a constraint trigger; maybe my > patch is too focused on that specific bit and instead we should handle > also NO INHERIT and NOT ENFORCED cases, not really sure (it's certainly > not an important part of this patch). For ease of review, here's the three patches. 0001 solves the main problem with ALTER objtype ALTER CONSTRAINT NOT VALID. I propose to put 0001 in 18 and 19, and leave 0002 and 0003 (as a single commit) for 19 only, since it's been like that for ages and there have been zero complaints before my own in the other thread. I put 0002 as a separate one just for review, to show that these errors we throw are nothing new: these commands would also fail if we don't patch this code, they're just using bad grammar, which is then fixed by 0003. I think I should list Amul as the fourth co-author of 0001. That would make the longest list of coauthorship for a patch that small. Or I could just say: "Author: Postgres Global Development Group". -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Вложения
On 2025/07/01 3:27, Álvaro Herrera wrote: > On 2025-Jun-30, Álvaro Herrera wrote: > >>> Just one note: Jian's patch doesn't handle the same issue for TRIGGER >>> case, so that part might still need to be addressed. >> >> Okay, here's my take on this, wherein I reworded the proposed error >> message. I also handled the NOT VALID case of a constraint trigger; maybe my >> patch is too focused on that specific bit and instead we should handle >> also NO INHERIT and NOT ENFORCED cases, not really sure (it's certainly >> not an important part of this patch). > > For ease of review, here's the three patches. 0001 solves the main > problem with ALTER objtype ALTER CONSTRAINT NOT VALID. Thanks for updating the patches! Patch 0001 looks good to me. I have one question though: why didn't you include an error code in the error message? I was thinking it would be fine to use errcode(ERRCODE_FEATURE_NOT_SUPPORTED), like other error messages in processCASbits(), since ALTER CONSTRAINT NOT VALID isn't supported. > I propose to put 0001 in 18 and 19, and leave 0002 and 0003 (as a single > commit) for 19 only, since it's been like that for ages and there have > been zero complaints before my own in the other thread. I put 0002 as a > separate one just for review, to show that these errors we throw are > nothing new: these commands would also fail if we don't patch this code, > they're just using bad grammar, which is then fixed by 0003. > > > I think I should list Amul as the fourth co-author of 0001. That would > make the longest list of coauthorship for a patch that small. Or I > could just say: "Author: Postgres Global Development Group". If the patch is based on Amul's work, I agree it makes sense to add him as a co-author in the commit log. Regards, -- Fujii Masao NTT DATA Japan Corporation
On 2025/07/02 23:31, Fujii Masao wrote:
>
>
> On 2025/07/01 3:27, Álvaro Herrera wrote:
>> On 2025-Jun-30, Álvaro Herrera wrote:
>>
>>>> Just one note: Jian's patch doesn't handle the same issue for TRIGGER
>>>> case, so that part might still need to be addressed.
>>>
>>> Okay, here's my take on this, wherein I reworded the proposed error
>>> message. I also handled the NOT VALID case of a constraint trigger; maybe my
>>> patch is too focused on that specific bit and instead we should handle
>>> also NO INHERIT and NOT ENFORCED cases, not really sure (it's certainly
>>> not an important part of this patch).
>>
>> For ease of review, here's the three patches. 0001 solves the main
>> problem with ALTER objtype ALTER CONSTRAINT NOT VALID.
>
> Thanks for updating the patches! Patch 0001 looks good to me.
>
> I have one question though: why didn't you include an error code in
> the error message? I was thinking it would be fine to use
> errcode(ERRCODE_FEATURE_NOT_SUPPORTED), like other error messages
> in processCASbits(), since ALTER CONSTRAINT NOT VALID isn't supported.
>
>
>> I propose to put 0001 in 18 and 19, and leave 0002 and 0003 (as a single
>> commit) for 19 only, since it's been like that for ages and there have
>> been zero complaints before my own in the other thread.
Agreed.
>> I put 0002 as a
>> separate one just for review, to show that these errors we throw are
>> nothing new: these commands would also fail if we don't patch this code,
>> they're just using bad grammar, which is then fixed by 0003.
Regarding the 0003 patch:
+ if (($11 & CAS_NOT_ENFORCED) != 0)
+ ereport(ERROR,
+ errmsg("constraint triggers cannot be marked %s",
+ "NOT ENFORCED"),
+ parser_errposition(@11));
Shouldn't we also raise an error when CAS_ENFORCED is given?
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
On 2025/07/03 0:31, Álvaro Herrera wrote:
> On 2025-Jul-02, Fujii Masao wrote:
>
>> Regarding the 0003 patch:
>>
>> + if (($11 & CAS_NOT_ENFORCED) != 0)
>> + ereport(ERROR,
>> + errmsg("constraint triggers cannot be marked %s",
>> + "NOT ENFORCED"),
>> + parser_errposition(@11));
>>
>> Shouldn't we also raise an error when CAS_ENFORCED is given?
>
> Great point -- ENFORCED can also throw the bogus error. However, if you
> say ENFORCED, the constraint is going to be enforced, which is the same
> that happens if you don't say anything, so I think we should accept the
> case. So how about this?
Yeah, this approach works for me. TBH I'm fine with either way.
If we go with it, I’m slightly inclined to add [ ENFORCED ] to
the CREATE TRIGGER syntax in the docs. Without that, users might be confused
or raise concerns that CREATE CONSTRAINT TRIGGER accepts an option
(i.e., ENFORCED) that isn't actually documented in the syntax.
But if you think this is overkill, I'm ok not to update the syntax in the docs.
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
On 2025-Jul-03, Fujii Masao wrote: > If we go with it, I’m slightly inclined to add [ ENFORCED ] to > the CREATE TRIGGER syntax in the docs. Without that, users might be confused > or raise concerns that CREATE CONSTRAINT TRIGGER accepts an option > (i.e., ENFORCED) that isn't actually documented in the syntax. > But if you think this is overkill, I'm ok not to update the syntax in the docs. Yeah, makes sense. We document noise words for other commands as well, so I added that and pushed. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Every machine is a smoke machine if you operate it wrong enough." https://twitter.com/libseybieda/status/1541673325781196801