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/
>From 484b22d971be13d0c7f5a88d40aff85720a85203 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvhe...@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.universal...@gmail.com> Co-authored-by: Fujii Masao <masao.fu...@oss.nttdata.com> Co-authored-by: Álvaro Herrera <alvhe...@kurilemu.de> Discussion: https://postgr.es/m/CACJufxHSp2puxP=q8ztugl1f+heapnzqfbzy5zngujugwjb...@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
>From 1a00291b39e915263e38d9841683d11a050970b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvhe...@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
>From 072fcc83a92a16982fb93957dbd6ba807f1de5e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvhe...@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