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:
/messages/by-id/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:/messages/by-id/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.
Attachments:
v1-0001-disallow-ALTER-CONSTRAINT-NOT-VALID.patchtext/x-patch; charset=US-ASCII; name=v1-0001-disallow-ALTER-CONSTRAINT-NOT-VALID.patchDownload+9-3
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:/messages/by-id/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:/messages/by-id/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")
Attachments:
v2-0001-error-handling-for-ALTER-CONSTRAINT-NOT-VALID.no-cfbotapplication/octet-stream; name=v2-0001-error-handling-for-ALTER-CONSTRAINT-NOT-VALID.no-cfbotDownload+9-3
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:/messages/by-id/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 VALIDeven 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 2025/06/18 1:00, Fujii Masao wrote:
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?
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.
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
Attachments:
v3-0001-Fix-error-handling-of-ALTER-CONSTRAINT-NOT-VALID.patchtext/plain; charset=UTF-8; name=v3-0001-Fix-error-handling-of-ALTER-CONSTRAINT-NOT-VALID.patchDownload+31-6
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 much prefer 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 much prefer to use the patch I mentioned near the beginning of the thread.
Are you referring to v2-0001-trial.patch proposed at [1]/messages/by-id/CAAJ_b97hd-jMTS7AjgU6TDBCzDx_KyuKxG+K-DtYmOieg+giyQ@mail.gmail.com?
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]/messages/by-id/CACJufxH9krMV-rJkC1J=Jvy_FAO_NRVXGMV+DSNm2saHjbuw8Q@mail.gmail.com.
{
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]/messages/by-id/CACJufxH9krMV-rJkC1J=Jvy_FAO_NRVXGMV+DSNm2saHjbuw8Q@mail.gmail.com 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]: /messages/by-id/CAAJ_b97hd-jMTS7AjgU6TDBCzDx_KyuKxG+K-DtYmOieg+giyQ@mail.gmail.com
[2]: /messages/by-id/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 would much 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"
Attachments:
v4-0001-Fix-error-message-for-ALTER-CONSTRAINT-.-NOT-VALI.patchtext/x-diff; charset=utf-8Download+34-2
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-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?
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"This is a foot just waiting to be shot" (Andrew Dunstan)
Attachments:
v6-0001-Fix-bogus-grammar-for-a-CREATE-CONSTRAINT-TRIGGER.patchtext/x-diff; charset=utf-8Download+57-4
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