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 <fu...@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

Reply via email to