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