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
From 4f5f32073d0bbb03effbaae2420f616f70362050 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Mon, 2 Jun 2025 10:23:13 +0800
Subject: [PATCH v1 1/1] disallow ALTER CONSTRAINT NOT VALID
discussion: https://postgr.es/m/
---
src/backend/parser/gram.y | 9 ++++++++-
src/test/regress/expected/foreign_key.out | 2 +-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 0b5652071d1..a1fe5fab1fa 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2669,7 +2669,14 @@ alter_table_cmd:
c->alterDeferrability = true;
if ($4 & CAS_NO_INHERIT)
c->alterInheritability = true;
- processCASbits($4, @4, "FOREIGN KEY",
+
+ if ($4 & CAS_NOT_VALID)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot alter constraint validity"),
+ parser_errposition(@4));
+
+ processCASbits($4, @4, NULL,
&c->deferrable,
&c->initdeferred,
&c->is_enforced,
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 4f3f280a439..60c617e4fe4 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1359,7 +1359,7 @@ LINE 1: ...e ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE INITIALLY ...
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT;
ERROR: constraint "fktable_fk_fkey" of relation "fktable" is not a not-null constraint
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT VALID;
-ERROR: FOREIGN KEY constraints cannot be marked NOT VALID
+ERROR: cannot alter constraint validity
LINE 1: ...ER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT VALID;
^
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey ENFORCED NOT ENFORCED;
--
2.34.1
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
From 2d60af3bcfe4359d79801ddb2d3cfd3f3fee0d4e Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Fri, 13 Jun 2025 15:06:03 +0800
Subject: [PATCH v2 1/1] error handling for ALTER CONSTRAINT NOT VALID
ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported,\
so set appropriate error handling to in gram.y.
discussion: https://postgr.es/m/CACJufxHSp2puxP=q8ZtUGL1F+heapnzqFBZy5ZNGUjUgwjBqTQ@mail.gmail.com
---
src/backend/parser/gram.y | 9 ++++++++-
src/test/regress/expected/foreign_key.out | 2 +-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 46bbdcbc740..4c405c7f8dc 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2686,7 +2686,14 @@ alter_table_cmd:
c->alterDeferrability = true;
if ($4 & CAS_NO_INHERIT)
c->alterInheritability = true;
- processCASbits($4, @4, "FOREIGN KEY",
+
+ if ($4 & CAS_NOT_VALID)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported"),
+ parser_errposition(@4));
+
+ processCASbits($4, @4, NULL,
&c->deferrable,
&c->initdeferred,
&c->is_enforced,
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 6a8f3959345..cd5f91b08d0 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1359,7 +1359,7 @@ LINE 1: ...e ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE INITIALLY ...
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT;
ERROR: constraint "fktable_fk_fkey" of relation "fktable" is not a not-null constraint
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT VALID;
-ERROR: FOREIGN KEY constraints cannot be marked NOT VALID
+ERROR: ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported
LINE 1: ...ER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT VALID;
^
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey ENFORCED NOT ENFORCED;
--
2.34.1
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
From 59b5f5c711331be0e9376f01948412f5079ca424 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Fri, 27 Jun 2025 12:35:21 +0900
Subject: [PATCH v3] Fix error handling of ALTER CONSTRAINT NOT VALID.
---
src/backend/commands/tablecmds.c | 9 +++++++++
src/backend/parser/gram.y | 16 ++++++++++++++--
src/include/nodes/parsenodes.h | 7 +++++++
src/test/regress/expected/foreign_key.out | 4 +---
4 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e2b94c8c609..548473d60a6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12171,6 +12171,15 @@ ATExecAlterConstraint(List **wqueue, Relation rel, ATAlterConstraint *cmdcon,
Form_pg_constraint currcon;
ObjectAddress address;
+ /*
+ * Raise an error if NOT VALID is specified, since it's accepted by the
+ * syntax but not supported by ALTER CONSTRAINT.
+ */
+ if (cmdcon->skip_validation)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported"));
+
/*
* Disallow altering ONLY a partitioned table, as it would make no sense.
* This is okay for legacy inheritance.
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 50f53159d58..5df301a76b7 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2657,6 +2657,7 @@ alter_table_cmd:
{
AlterTableCmd *n = makeNode(AlterTableCmd);
ATAlterConstraint *c = makeNode(ATAlterConstraint);
+ bool dummy;
n->subtype = AT_AlterConstraint;
n->def = (Node *) c;
@@ -2668,11 +2669,22 @@ alter_table_cmd:
c->alterDeferrability = true;
if ($4 & CAS_NO_INHERIT)
c->alterInheritability = true;
- processCASbits($4, @4, "FOREIGN KEY",
+
+ /*
+ * 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;
+
+ processCASbits($4, @4, "",
&c->deferrable,
&c->initdeferred,
&c->is_enforced,
- NULL,
+ &dummy,
&c->noinherit,
yyscanner);
$$ = (Node *) n;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index ba12678d1cb..99169282ed5 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2507,6 +2507,13 @@ typedef struct ATAlterConstraint
bool initdeferred; /* INITIALLY DEFERRED? */
bool alterInheritability; /* changing inheritability properties */
bool noinherit;
+ bool skip_validation; /* skip validation of existing rows? ALTER
+ * CONSTRAINT NOT VALID is allowed by the
+ * syntax, but not actually supported.
+ * This field tracks whether NOT VALID was
+ * specified. If it is set to true, an
+ * error will be raised later in
+ * ATExecAlterConstraint(). */
} ATAlterConstraint;
/* Ad-hoc node for AT_ReplicaIdentity */
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 6a8f3959345..21c1172fdd8 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1359,9 +1359,7 @@ LINE 1: ...e ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE INITIALLY ...
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT;
ERROR: constraint "fktable_fk_fkey" of relation "fktable" is not a not-null constraint
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT VALID;
-ERROR: FOREIGN KEY constraints cannot be marked NOT VALID
-LINE 1: ...ER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT VALID;
- ^
+ERROR: ALTER TABLE ... ALTER CONSTRAINT ... NOT VALID is not supported
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey ENFORCED NOT ENFORCED;
ERROR: conflicting constraint properties
LINE 1: ...fktable ALTER CONSTRAINT fktable_fk_fkey ENFORCED NOT ENFORC...
--
2.49.0
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
From 10b9ac219ba4d01dcdf83c2fb9d7d6b344bd74f2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Mon, 30 Jun 2025 17:07:04 +0200
Subject: [PATCH v4] Fix error message for ALTER CONSTRAINT ... NOT VALID
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Trying to alter a constraint so that it becomes NOT VALID results in an
error that assumes the constraint is a foreign key. This is potentially
wrong, so give a more generic error message.
While at it, give CREATE CONSTRAINT TRIGGER a better error message as
well.
Co-authored-by: jian he <jian.universality@gmail.com>
Co-authored-by: Fujii Masao <masao.fujii@oss.nttdata.com>
Co-authored-by: Ãlvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/CACJufxHSp2puxP=q8ZtUGL1F+heapnzqFBZy5ZNGUjUgwjBqTQ@mail.gmail.com
---
src/backend/parser/gram.y | 9 +++++++++
src/test/regress/expected/constraints.out | 5 +++++
src/test/regress/expected/foreign_key.out | 2 +-
src/test/regress/expected/triggers.out | 9 +++++++++
src/test/regress/sql/constraints.sql | 3 +++
src/test/regress/sql/triggers.sql | 7 +++++++
6 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 50f53159d58..8a1712a099d 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2668,6 +2668,10 @@ alter_table_cmd:
c->alterDeferrability = true;
if ($4 & CAS_NO_INHERIT)
c->alterInheritability = true;
+ if ($4 & CAS_NOT_VALID)
+ ereport(ERROR,
+ errmsg("constraints cannot be altered to be NOT VALID"),
+ parser_errposition(@4));
processCASbits($4, @4, "FOREIGN KEY",
&c->deferrable,
&c->initdeferred,
@@ -6036,6 +6040,11 @@ CreateTrigStmt:
{
CreateTrigStmt *n = makeNode(CreateTrigStmt);
+ if (($11 & CAS_NOT_VALID) != 0)
+ ereport(ERROR,
+ errmsg("cannot create a NOT VALID constraint trigger"),
+ parser_errposition(@11));
+
n->replace = $2;
if (n->replace) /* not supported, see CreateTrigger */
ereport(ERROR,
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index b5592617d97..ccea883cffd 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -748,6 +748,11 @@ ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
ERROR: cannot alter enforceability of constraint "unique_tbl_i_key" of relation "unique_tbl"
ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORCED;
ERROR: cannot alter enforceability of constraint "unique_tbl_i_key" of relation "unique_tbl"
+-- can't make an existing constraint NOT VALID
+ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT VALID;
+ERROR: constraints cannot be altered to be NOT VALID
+LINE 1: ...ABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT VALID;
+ ^
DROP TABLE unique_tbl;
--
-- EXCLUDE constraints
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 6a8f3959345..f9bd252444f 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1359,7 +1359,7 @@ LINE 1: ...e ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE INITIALLY ...
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT;
ERROR: constraint "fktable_fk_fkey" of relation "fktable" is not a not-null constraint
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT VALID;
-ERROR: FOREIGN KEY constraints cannot be marked NOT VALID
+ERROR: constraints cannot be altered to be NOT VALID
LINE 1: ...ER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT VALID;
^
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey ENFORCED NOT ENFORCED;
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 2bf0e77d61e..8f0c6cd1843 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2280,6 +2280,15 @@ select * from parted;
drop table parted;
drop function parted_trigfunc();
--
+-- Constraint triggers
+--
+create constraint trigger crtr
+ after insert on foo not valid
+ for each row execute procedure foo ();
+ERROR: cannot create a NOT VALID constraint trigger
+LINE 2: after insert on foo not valid
+ ^
+--
-- Constraint triggers and partitioned tables
create table parted_constr_ancestor (a int, b text)
partition by range (b);
diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql
index 12668f0e0ce..7487723ab84 100644
--- a/src/test/regress/sql/constraints.sql
+++ b/src/test/regress/sql/constraints.sql
@@ -537,6 +537,9 @@ CREATE TABLE UNIQUE_NOTEN_TBL(i int UNIQUE NOT ENFORCED);
ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORCED;
+-- can't make an existing constraint NOT VALID
+ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT VALID;
+
DROP TABLE unique_tbl;
--
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 9ffd318385f..c85d0da665c 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1576,6 +1576,13 @@ select * from parted;
drop table parted;
drop function parted_trigfunc();
+--
+-- Constraint triggers
+--
+create constraint trigger crtr
+ after insert on foo not valid
+ for each row execute procedure foo ();
+
--
-- Constraint triggers and partitioned tables
create table parted_constr_ancestor (a int, b text)
--
2.39.5
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/
Attachments:
v5-0001-Fix-error-message-for-ALTER-CONSTRAINT-.-NOT-VALI.patchtext/x-diff; charset=utf-8Download
From 484b22d971be13d0c7f5a88d40aff85720a85203 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Mon, 30 Jun 2025 17:07:04 +0200
Subject: [PATCH v5 1/3] Fix error message for ALTER CONSTRAINT ... NOT VALID
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Trying to alter a constraint so that it becomes NOT VALID results in an
error that assumes the constraint is a foreign key. This is potentially
wrong, so give a more generic error message.
While at it, give CREATE CONSTRAINT TRIGGER a better error message as
well.
Co-authored-by: jian he <jian.universality@gmail.com>
Co-authored-by: Fujii Masao <masao.fujii@oss.nttdata.com>
Co-authored-by: Ãlvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/CACJufxHSp2puxP=q8ZtUGL1F+heapnzqFBZy5ZNGUjUgwjBqTQ@mail.gmail.com
---
src/backend/parser/gram.y | 4 ++++
src/test/regress/expected/constraints.out | 5 +++++
src/test/regress/expected/foreign_key.out | 2 +-
src/test/regress/sql/constraints.sql | 3 +++
4 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 50f53159d58..527c7ea75e3 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2668,6 +2668,10 @@ alter_table_cmd:
c->alterDeferrability = true;
if ($4 & CAS_NO_INHERIT)
c->alterInheritability = true;
+ if ($4 & CAS_NOT_VALID)
+ ereport(ERROR,
+ errmsg("constraints cannot be altered to be NOT VALID"),
+ parser_errposition(@4));
processCASbits($4, @4, "FOREIGN KEY",
&c->deferrable,
&c->initdeferred,
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index b5592617d97..ccea883cffd 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -748,6 +748,11 @@ ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
ERROR: cannot alter enforceability of constraint "unique_tbl_i_key" of relation "unique_tbl"
ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORCED;
ERROR: cannot alter enforceability of constraint "unique_tbl_i_key" of relation "unique_tbl"
+-- can't make an existing constraint NOT VALID
+ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT VALID;
+ERROR: constraints cannot be altered to be NOT VALID
+LINE 1: ...ABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT VALID;
+ ^
DROP TABLE unique_tbl;
--
-- EXCLUDE constraints
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 6a8f3959345..f9bd252444f 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1359,7 +1359,7 @@ LINE 1: ...e ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE INITIALLY ...
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT;
ERROR: constraint "fktable_fk_fkey" of relation "fktable" is not a not-null constraint
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT VALID;
-ERROR: FOREIGN KEY constraints cannot be marked NOT VALID
+ERROR: constraints cannot be altered to be NOT VALID
LINE 1: ...ER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT VALID;
^
ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey ENFORCED NOT ENFORCED;
diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql
index 12668f0e0ce..7487723ab84 100644
--- a/src/test/regress/sql/constraints.sql
+++ b/src/test/regress/sql/constraints.sql
@@ -537,6 +537,9 @@ CREATE TABLE UNIQUE_NOTEN_TBL(i int UNIQUE NOT ENFORCED);
ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORCED;
+-- can't make an existing constraint NOT VALID
+ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT VALID;
+
DROP TABLE unique_tbl;
--
--
2.39.5
v5-0002-Tests-to-illustrate-bogus-messages-in-CREATE-CONS.patchtext/x-diff; charset=utf-8Download
From 1a00291b39e915263e38d9841683d11a050970b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Mon, 30 Jun 2025 20:15:08 +0200
Subject: [PATCH v5 2/3] Tests to illustrate bogus messages in CREATE
CONSTRAINT TRIGGER
---
src/test/regress/expected/triggers.out | 21 +++++++++++++++++++++
src/test/regress/sql/triggers.sql | 13 +++++++++++++
2 files changed, 34 insertions(+)
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 2bf0e77d61e..626fabf41be 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2280,6 +2280,27 @@ select * from parted;
drop table parted;
drop function parted_trigfunc();
--
+-- Constraint triggers
+--
+create constraint trigger crtr
+ after insert on foo not valid
+ for each row execute procedure foo ();
+ERROR: TRIGGER constraints cannot be marked NOT VALID
+LINE 2: after insert on foo not valid
+ ^
+create constraint trigger crtr
+ after insert on foo no inherit
+ for each row execute procedure foo ();
+ERROR: TRIGGER constraints cannot be marked NO INHERIT
+LINE 2: after insert on foo no inherit
+ ^
+create constraint trigger crtr
+ after insert on foo not enforced
+ for each row execute procedure foo ();
+ERROR: TRIGGER constraints cannot be marked NOT ENFORCED
+LINE 2: after insert on foo not enforced
+ ^
+--
-- Constraint triggers and partitioned tables
create table parted_constr_ancestor (a int, b text)
partition by range (b);
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 9ffd318385f..75a334ebad5 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1576,6 +1576,19 @@ select * from parted;
drop table parted;
drop function parted_trigfunc();
+--
+-- Constraint triggers
+--
+create constraint trigger crtr
+ after insert on foo not valid
+ for each row execute procedure foo ();
+create constraint trigger crtr
+ after insert on foo no inherit
+ for each row execute procedure foo ();
+create constraint trigger crtr
+ after insert on foo not enforced
+ for each row execute procedure foo ();
+
--
-- Constraint triggers and partitioned tables
create table parted_constr_ancestor (a int, b text)
--
2.39.5
v5-0003-fix-grammar.patchtext/x-diff; charset=utf-8Download
From 072fcc83a92a16982fb93957dbd6ba807f1de5e6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Mon, 30 Jun 2025 20:15:17 +0200
Subject: [PATCH v5 3/3] fix grammar
---
src/backend/parser/gram.y | 16 ++++++++++++++++
src/test/regress/expected/triggers.out | 6 +++---
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 527c7ea75e3..f66fbd20a4d 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -6040,6 +6040,22 @@ CreateTrigStmt:
{
CreateTrigStmt *n = makeNode(CreateTrigStmt);
+ if (($11 & CAS_NOT_VALID) != 0)
+ ereport(ERROR,
+ errmsg("constraint triggers cannot be marked %s",
+ "NOT VALID"),
+ parser_errposition(@11));
+ if (($11 & CAS_NO_INHERIT) != 0)
+ ereport(ERROR,
+ errmsg("constraint triggers cannot be marked %s",
+ "NO INHERIT"),
+ parser_errposition(@11));
+ if (($11 & CAS_NOT_ENFORCED) != 0)
+ ereport(ERROR,
+ errmsg("constraint triggers cannot be marked %s",
+ "NOT ENFORCED"),
+ parser_errposition(@11));
+
n->replace = $2;
if (n->replace) /* not supported, see CreateTrigger */
ereport(ERROR,
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 626fabf41be..69c8dbf0084 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2285,19 +2285,19 @@ drop function parted_trigfunc();
create constraint trigger crtr
after insert on foo not valid
for each row execute procedure foo ();
-ERROR: TRIGGER constraints cannot be marked NOT VALID
+ERROR: constraint triggers cannot be marked NOT VALID
LINE 2: after insert on foo not valid
^
create constraint trigger crtr
after insert on foo no inherit
for each row execute procedure foo ();
-ERROR: TRIGGER constraints cannot be marked NO INHERIT
+ERROR: constraint triggers cannot be marked NO INHERIT
LINE 2: after insert on foo no inherit
^
create constraint trigger crtr
after insert on foo not enforced
for each row execute procedure foo ();
-ERROR: TRIGGER constraints cannot be marked NOT ENFORCED
+ERROR: constraint triggers cannot be marked NOT ENFORCED
LINE 2: after insert on foo not enforced
^
--
--
2.39.5
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
From e4ebadb0131880c2494a31764fc0d5efe4ff9796 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Wed, 2 Jul 2025 17:10:57 +0200
Subject: [PATCH v6] Fix bogus grammar for a CREATE CONSTRAINT TRIGGER error
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
If certain constraint characteristic clauses (NO INHERIT, NOT VALID, NOT
ENFORCED) are given to CREATE CONSTRAINT TRIGGER, the resulting error
message is
ERROR: TRIGGER constraints cannot be marked NO INHERIT
which is a bit silly, because these aren't "constraints of type
TRIGGER". Hardcode a better error message to prevent it. This is a
cosmetic fix for quite a fringe problem with no known complaints from
users, so no backpatch.
While at it, silently accept ENFORCED if given.
Author: Amul Sul <sulamul@gmail.com>
Reviewed-by: jian he <jian.universality@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@oss.nttdata.com>
Reviewed-by: Ãlvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/CAAJ_b97hd-jMTS7AjgU6TDBCzDx_KyuKxG+K-DtYmOieg+giyQ@mail.gmail.com
Discussion: https://postgr.es/m/CACJufxHSp2puxP=q8ZtUGL1F+heapnzqFBZy5ZNGUjUgwjBqTQ@mail.gmail.com
---
src/backend/parser/gram.y | 22 +++++++++++++++++++++-
src/test/regress/expected/triggers.out | 23 ++++++++++++++++++++++-
src/test/regress/sql/triggers.sql | 15 ++++++++++++++-
3 files changed, 57 insertions(+), 3 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a2e084b8f64..f404c77e504 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -6041,6 +6041,26 @@ CreateTrigStmt:
EXECUTE FUNCTION_or_PROCEDURE func_name '(' TriggerFuncArgs ')'
{
CreateTrigStmt *n = makeNode(CreateTrigStmt);
+ bool dummy;
+
+ if (($11 & CAS_NOT_VALID) != 0)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("constraint triggers cannot be marked %s",
+ "NOT VALID"),
+ parser_errposition(@11));
+ if (($11 & CAS_NO_INHERIT) != 0)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("constraint triggers cannot be marked %s",
+ "NO INHERIT"),
+ parser_errposition(@11));
+ if (($11 & CAS_NOT_ENFORCED) != 0)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("constraint triggers cannot be marked %s",
+ "NOT ENFORCED"),
+ parser_errposition(@11));
n->replace = $2;
if (n->replace) /* not supported, see CreateTrigger */
@@ -6060,7 +6080,7 @@ CreateTrigStmt:
n->whenClause = $15;
n->transitionRels = NIL;
processCASbits($11, @11, "TRIGGER",
- &n->deferrable, &n->initdeferred, NULL,
+ &n->deferrable, &n->initdeferred, &dummy,
NULL, NULL, yyscanner);
n->constrrel = $10;
$$ = (Node *) n;
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 2bf0e77d61e..872b9100e1a 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2280,6 +2280,27 @@ select * from parted;
drop table parted;
drop function parted_trigfunc();
--
+-- Constraint triggers
+--
+create constraint trigger crtr
+ after insert on foo not valid
+ for each row execute procedure foo ();
+ERROR: constraint triggers cannot be marked NOT VALID
+LINE 2: after insert on foo not valid
+ ^
+create constraint trigger crtr
+ after insert on foo no inherit
+ for each row execute procedure foo ();
+ERROR: constraint triggers cannot be marked NO INHERIT
+LINE 2: after insert on foo no inherit
+ ^
+create constraint trigger crtr
+ after insert on foo not enforced
+ for each row execute procedure foo ();
+ERROR: constraint triggers cannot be marked NOT ENFORCED
+LINE 2: after insert on foo not enforced
+ ^
+--
-- Constraint triggers and partitioned tables
create table parted_constr_ancestor (a int, b text)
partition by range (b);
@@ -2294,7 +2315,7 @@ create constraint trigger parted_trig after insert on parted_constr_ancestor
deferrable
for each row execute procedure trigger_notice_ab();
create constraint trigger parted_trig_two after insert on parted_constr
- deferrable initially deferred
+ deferrable initially deferred enforced
for each row when (bark(new.b) AND new.a % 2 = 1)
execute procedure trigger_notice_ab();
-- The immediate constraint is fired immediately; the WHEN clause of the
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 9ffd318385f..d674b25c83b 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1576,6 +1576,19 @@ select * from parted;
drop table parted;
drop function parted_trigfunc();
+--
+-- Constraint triggers
+--
+create constraint trigger crtr
+ after insert on foo not valid
+ for each row execute procedure foo ();
+create constraint trigger crtr
+ after insert on foo no inherit
+ for each row execute procedure foo ();
+create constraint trigger crtr
+ after insert on foo not enforced
+ for each row execute procedure foo ();
+
--
-- Constraint triggers and partitioned tables
create table parted_constr_ancestor (a int, b text)
@@ -1591,7 +1604,7 @@ create constraint trigger parted_trig after insert on parted_constr_ancestor
deferrable
for each row execute procedure trigger_notice_ab();
create constraint trigger parted_trig_two after insert on parted_constr
- deferrable initially deferred
+ deferrable initially deferred enforced
for each row when (bark(new.b) AND new.a % 2 = 1)
execute procedure trigger_notice_ab();
--
2.39.5
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