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
From 59b5f5c711331be0e9376f01948412f5079ca424 Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email protected]>
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
