bogus error message for ALTER TABLE ALTER CONSTRAINT

Started by Álvaro Herrera10 months ago14 messages

alvherre@alvh.no-ip.org

10 months ago

1 attachment(s)

I just discovered that trying to set a foreign key as NO INHERIT in
ALTER TABLE ALTER CONSTRAINT returns an absurd error message:

create table pk (a int primary key);
create table fk (a int references pk);

alter table fk alter constraint fk_a_fkey deferrable, alter constraint fk_a_fkey no inherit;
ERROR: ALTER CONSTRAINT statement constraints cannot be marked NO INHERIT

The explanation is that somebody misunderstood what must be given to
processCASbits in 2013. The intended message is:
ERROR: FOREIGN KEY constraints cannot be marked NO INHERIT

Here's the fix along with some additional cleanup.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

Attachments:

0001-Fix-ALTER-TABLE-error-message.patchtext/x-diff; charset=utf-8Download

From ba95124d06e337c285a31b0376230702e6b1fc3d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@alvh.no-ip.org>
Date: Tue, 4 Mar 2025 18:48:14 +0100
Subject: [PATCH] Fix ALTER TABLE error message

This bogus error message was introduced in 2013 by commit f177cbfe676d,
because of misunderstanding the processCASbits() API; at the time, no
test cases were added that would be affected by this change.  Only in
ca87c415e2fc was one added (along with a couple of typos), with an XXX
note that the error message was bogus.  Fix the whole, add some test
cases.

Backpatch all the way back.
---
 src/backend/parser/gram.y                 |  2 +-
 src/test/regress/expected/constraints.out |  7 +++----
 src/test/regress/expected/foreign_key.out | 10 +++++++++-
 src/test/regress/sql/constraints.sql      |  3 +--
 src/test/regress/sql/foreign_key.sql      |  4 +++-
 5 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7d99c9355c6..c11a3beff06 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2663,7 +2663,7 @@ alter_table_cmd:
 					n->def = (Node *) c;
 					c->conname = $3;
 					c->alterDeferrability = true;
-					processCASbits($4, @4, "ALTER CONSTRAINT statement",
+					processCASbits($4, @4, "FOREIGN KEY",
 									&c->deferrable,
 									&c->initdeferred,
 									NULL, NULL, NULL, yyscanner);
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index 692a69fe457..4f39100fcdf 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -735,7 +735,7 @@ SELECT * FROM unique_tbl;
  3 | threex
 (5 rows)
 
--- enforcibility cannot be specified or set for unique constrain
+-- enforceability cannot be specified or set for unique constraint
 CREATE TABLE UNIQUE_EN_TBL(i int UNIQUE ENFORCED);
 ERROR:  misplaced ENFORCED clause
 LINE 1: CREATE TABLE UNIQUE_EN_TBL(i int UNIQUE ENFORCED);
@@ -744,13 +744,12 @@ CREATE TABLE UNIQUE_NOTEN_TBL(i int UNIQUE NOT ENFORCED);
 ERROR:  misplaced NOT ENFORCED clause
 LINE 1: CREATE TABLE UNIQUE_NOTEN_TBL(i int UNIQUE NOT ENFORCED);
                                                    ^
--- XXX: error message is misleading here
 ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
-ERROR:  ALTER CONSTRAINT statement constraints cannot be marked ENFORCED
+ERROR:  FOREIGN KEY constraints cannot be marked ENFORCED
 LINE 1: ...TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
                                                               ^
 ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORCED;
-ERROR:  ALTER CONSTRAINT statement constraints cannot be marked NOT ENFORCED
+ERROR:  FOREIGN KEY constraints cannot be marked NOT ENFORCED
 LINE 1: ...ABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORC...
                                                              ^
 DROP TABLE unique_tbl;
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 374dcb266e7..6a3374d5152 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1278,11 +1278,19 @@ DETAIL:  Key (fk)=(20) is not present in table "pktable".
 COMMIT;
 -- try additional syntax
 ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE;
--- illegal option
+-- illegal options
 ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE INITIALLY DEFERRED;
 ERROR:  constraint declared INITIALLY DEFERRED must be DEFERRABLE
 LINE 1: ...e ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE INITIALLY ...
                                                              ^
+ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT;
+ERROR:  FOREIGN KEY constraints cannot be marked NO INHERIT
+LINE 1: ...ER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT...
+                                                             ^
+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;
+                                                             ^
 -- test order of firing of FK triggers when several RI-induced changes need to
 -- be made to the same row.  This was broken by subtransaction-related
 -- changes in 8.0.
diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql
index d6742f83fb9..21ce4177de4 100644
--- a/src/test/regress/sql/constraints.sql
+++ b/src/test/regress/sql/constraints.sql
@@ -531,10 +531,9 @@ COMMIT;
 
 SELECT * FROM unique_tbl;
 
--- enforcibility cannot be specified or set for unique constrain
+-- enforceability cannot be specified or set for unique constraint
 CREATE TABLE UNIQUE_EN_TBL(i int UNIQUE ENFORCED);
 CREATE TABLE UNIQUE_NOTEN_TBL(i int UNIQUE NOT ENFORCED);
--- XXX: error message is misleading here
 ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
 ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORCED;
 
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index bc0adb8cfe9..44945b0453a 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -970,8 +970,10 @@ COMMIT;
 
 -- try additional syntax
 ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE;
--- illegal option
+-- illegal options
 ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE INITIALLY DEFERRED;
+ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT;
+ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NOT VALID;
 
 -- test order of firing of FK triggers when several RI-induced changes need to
 -- be made to the same row.  This was broken by subtransaction-related
-- 
2.39.5

Nathan Bossart

nathandbossart@gmail.com

10 months ago

In reply to: Álvaro Herrera (#1)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

On Tue, Mar 04, 2025 at 07:22:22PM +0100, ï¿½lvaro Herrera wrote:

I just discovered that trying to set a foreign key as NO INHERIT in
ALTER TABLE ALTER CONSTRAINT returns an absurd error message:

create table pk (a int primary key);
create table fk (a int references pk);

alter table fk alter constraint fk_a_fkey deferrable, alter constraint fk_a_fkey no inherit;
ERROR: ALTER CONSTRAINT statement constraints cannot be marked NO INHERIT

The explanation is that somebody misunderstood what must be given to
processCASbits in 2013. The intended message is:
ERROR: FOREIGN KEY constraints cannot be marked NO INHERIT

Here's the fix along with some additional cleanup.

LGTM

--
nathan

Álvaro Herrera

alvherre@alvh.no-ip.org

10 months ago

In reply to: Nathan Bossart (#2)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

On 2025-Mar-04, Nathan Bossart wrote:

On Tue, Mar 04, 2025 at 07:22:22PM +0100, Álvaro Herrera wrote:

I just discovered that trying to set a foreign key as NO INHERIT in
ALTER TABLE ALTER CONSTRAINT returns an absurd error message:

Here's the fix along with some additional cleanup.

LGTM

Many thanks for the quick look. Pushed now.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Thou shalt check the array bounds of all strings (indeed, all arrays), for
surely where thou typest "foo" someone someday shall type
"supercalifragilisticexpialidocious" (5th Commandment for C programmers)

Tom Lane

tgl@sss.pgh.pa.us

10 months ago

In reply to: Álvaro Herrera (#1)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:

I just discovered that trying to set a foreign key as NO INHERIT in
ALTER TABLE ALTER CONSTRAINT returns an absurd error message:
create table pk (a int primary key);
create table fk (a int references pk);
alter table fk alter constraint fk_a_fkey deferrable, alter constraint fk_a_fkey no inherit;
ERROR: ALTER CONSTRAINT statement constraints cannot be marked NO INHERIT

The explanation is that somebody misunderstood what must be given to
processCASbits in 2013. The intended message is:
ERROR: FOREIGN KEY constraints cannot be marked NO INHERIT

Hmm. I agree that "ALTER CONSTRAINT statement" is off the
mark here, but I'm not convinced that "FOREIGN KEY" is entirely
on-point either. The grammar has no way of knowing what kind of
constraint is being targeted. I do see that ATExecAlterConstraint
currently rejects every other kind of constraint, but do we need
to think of a more generic phrase?

regards, tom lane

Álvaro Herrera

alvherre@alvh.no-ip.org

10 months ago

In reply to: Tom Lane (#4)

1 attachment(s)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

On 2025-Mar-04, Tom Lane wrote:

Hmm. I agree that "ALTER CONSTRAINT statement" is off the
mark here, but I'm not convinced that "FOREIGN KEY" is entirely
on-point either. The grammar has no way of knowing what kind of
constraint is being targeted. I do see that ATExecAlterConstraint
currently rejects every other kind of constraint, but do we need
to think of a more generic phrase?

You're right that this is bogus, and looking what to do about it made me
realize that CREATE CONSTRAINT TRIGGER is also saying bogus things such
as:

create constraint trigger foo after insert on pg_class not valid for each row execute procedure test();
ERROR: TRIGGER constraints cannot be marked NOT VALID

create constraint trigger foo after insert on pg_class no inherit for each row execute procedure test();
ERROR: TRIGGER constraints cannot be marked NO INHERIT

So after mulling over this for a while I came up with the idea of adding
an output struct that processCASbits() can optionally be given and fill,
which indicates which flags were seeing while parsing the options. With
that, each of these two callers can throw more appropriate error messages
when a flag is given that they don't like. This is much better,
though arguably the error messages I propose can be wordsmithed still.
Most callers of processCASbits() don't care, so they just give a NULL
and they get the current behavior.

In the current incantation I just pass a "bool dummy" for the bits that
each production doesn't support; processCASbits throws no error and
instead sets the corresponding flag in the 'seen' struct. However,
another option might be to not pass a dummy at all and just
conditionally not throw any errors when the 'seen' struct is given.
This might be cleaner, but it also feels a bit magical. Any
preferences?

By the way, this also made me realize that the addition of a SET keyword
in the commands
ALTER TABLE .. ALTER CONSTRAINT SET NO INHERIT
ALTER TABLE .. ALTER CONSTRAINT SET INHERIT
could be removed easily by taking advantage of this 'seen' struct, and
we'd remove one production from the grammar (or both new ones, if we add
INHERIT to ConstraintAttributeElem).

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Debido a que la velocidad de la luz es mucho mayor que la del sonido,
algunas personas nos parecen brillantes un minuto antes
de escuchar las pelotudeces que dicen." (Roberto Fontanarrosa)

Attachments:

v1-0001-Improve-processCASbits-API-with-a-seen-struct.patchtext/x-diff; charset=utf-8Download

From fa312745d58050ff7a7c65937fa1188245287979 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@alvh.no-ip.org>
Date: Thu, 6 Mar 2025 17:01:22 +0100
Subject: [PATCH v1] Improve processCASbits API with a 'seen' struct

This allows ALTER TABLE .. ALTER CONSTRAINT to be more precise about
operations that are supported or not, as well as the reports from CREATE
CONSTRAINT TRIGGER error messages making more sense.
---
 src/backend/parser/gram.y                 | 118 ++++++++++++++++++----
 src/test/regress/expected/constraints.out |   4 +-
 src/test/regress/expected/foreign_key.out |   4 +-
 src/test/regress/expected/triggers.out    |  17 ++++
 src/test/regress/sql/triggers.sql         |   6 ++
 5 files changed, 127 insertions(+), 22 deletions(-)

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 271ae26cbaf..06d9aa31151 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -146,6 +146,17 @@ typedef struct KeyActions
 #define CAS_NOT_ENFORCED			0x40
 #define CAS_ENFORCED				0x80
 
+/*
+ * We represent whether each set of flags is seen on a command with CAS_flags.
+ */
+typedef struct CAS_flags
+{
+	bool	seen_deferrability;
+	bool	seen_enforced;
+	bool	seen_valid;
+	bool	seen_inherit;
+} CAS_flags;
+
 
 #define parser_yyerror(msg)  scanner_yyerror(msg, yyscanner)
 #define parser_errposition(pos)  scanner_errposition(pos, yyscanner)
@@ -199,7 +210,7 @@ static void SplitColQualList(List *qualList,
 							 core_yyscan_t yyscanner);
 static void processCASbits(int cas_bits, int location, const char *constrType,
 			   bool *deferrable, bool *initdeferred, bool *is_enforced,
-			   bool *not_valid, bool *no_inherit, core_yyscan_t yyscanner);
+			   bool *not_valid, bool *no_inherit, CAS_flags *seen, core_yyscan_t yyscanner);
 static PartitionStrategy parsePartitionStrategy(char *strategy, int location,
 												core_yyscan_t yyscanner);
 static void preprocess_pubobj_list(List *pubobjspec_list,
@@ -2658,15 +2669,37 @@ alter_table_cmd:
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 					ATAlterConstraint *c = makeNode(ATAlterConstraint);
+					CAS_flags	seen;
+					bool	dummy;
 
 					n->subtype = AT_AlterConstraint;
 					n->def = (Node *) c;
 					c->conname = $3;
-					c->alterDeferrability = true;
-					processCASbits($4, @4, "FOREIGN KEY",
+					processCASbits($4, @4, NULL,
 									&c->deferrable,
 									&c->initdeferred,
-									NULL, NULL, NULL, yyscanner);
+									&dummy, &dummy,
+									&dummy,
+									&seen,
+									yyscanner);
+					if (seen.seen_deferrability)
+						c->alterDeferrability = true;
+					/* cannot (currently) be changed by this syntax: */
+					if (seen.seen_enforced)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("cannot alter constraint enforceability"),
+								parser_errposition(@4));
+					if (seen.seen_valid)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("cannot alter constraint validity"),
+								parser_errposition(@4));
+					if (seen.seen_inherit)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("cannot alter constraint inheritability"),
+								parser_errposition(@4));
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER CONSTRAINT SET INHERIT */
@@ -4211,7 +4244,7 @@ ConstraintElem:
 					n->cooked_expr = NULL;
 					processCASbits($5, @5, "CHECK",
 								   NULL, NULL, &n->is_enforced, &n->skip_validation,
-								   &n->is_no_inherit, yyscanner);
+								   &n->is_no_inherit, NULL, yyscanner);
 					n->initially_valid = !n->skip_validation;
 					$$ = (Node *) n;
 				}
@@ -4225,7 +4258,7 @@ ConstraintElem:
 					/* no NOT VALID support yet */
 					processCASbits($4, @4, "NOT NULL",
 								   NULL, NULL, NULL, NULL,
-								   &n->is_no_inherit, yyscanner);
+								   &n->is_no_inherit, NULL, yyscanner);
 					n->initially_valid = true;
 					$$ = (Node *) n;
 				}
@@ -4245,7 +4278,7 @@ ConstraintElem:
 					n->indexspace = $9;
 					processCASbits($10, @10, "UNIQUE",
 								   &n->deferrable, &n->initdeferred, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, NULL, yyscanner);
 					$$ = (Node *) n;
 				}
 			| UNIQUE ExistingIndex ConstraintAttributeSpec
@@ -4261,7 +4294,7 @@ ConstraintElem:
 					n->indexspace = NULL;
 					processCASbits($3, @3, "UNIQUE",
 								   &n->deferrable, &n->initdeferred, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, NULL, yyscanner);
 					$$ = (Node *) n;
 				}
 			| PRIMARY KEY '(' columnList opt_without_overlaps ')' opt_c_include opt_definition OptConsTableSpace
@@ -4279,7 +4312,7 @@ ConstraintElem:
 					n->indexspace = $9;
 					processCASbits($10, @10, "PRIMARY KEY",
 								   &n->deferrable, &n->initdeferred, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, NULL, yyscanner);
 					$$ = (Node *) n;
 				}
 			| PRIMARY KEY ExistingIndex ConstraintAttributeSpec
@@ -4295,7 +4328,7 @@ ConstraintElem:
 					n->indexspace = NULL;
 					processCASbits($4, @4, "PRIMARY KEY",
 								   &n->deferrable, &n->initdeferred, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, NULL, yyscanner);
 					$$ = (Node *) n;
 				}
 			| EXCLUDE access_method_clause '(' ExclusionConstraintList ')'
@@ -4315,7 +4348,7 @@ ConstraintElem:
 					n->where_clause = $9;
 					processCASbits($10, @10, "EXCLUDE",
 								   &n->deferrable, &n->initdeferred, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, NULL, yyscanner);
 					$$ = (Node *) n;
 				}
 			| FOREIGN KEY '(' columnList optionalPeriodName ')' REFERENCES qualified_name
@@ -4345,7 +4378,7 @@ ConstraintElem:
 					processCASbits($12, @12, "FOREIGN KEY",
 								   &n->deferrable, &n->initdeferred,
 								   NULL, &n->skip_validation, NULL,
-								   yyscanner);
+								   NULL, yyscanner);
 					n->initially_valid = !n->skip_validation;
 					$$ = (Node *) n;
 				}
@@ -4385,7 +4418,7 @@ DomainConstraintElem:
 					n->cooked_expr = NULL;
 					processCASbits($5, @5, "CHECK",
 								   NULL, NULL, NULL, &n->skip_validation,
-								   &n->is_no_inherit, yyscanner);
+								   &n->is_no_inherit, NULL, yyscanner);
 					n->is_enforced = true;
 					n->initially_valid = !n->skip_validation;
 					$$ = (Node *) n;
@@ -4400,7 +4433,7 @@ DomainConstraintElem:
 					/* no NOT VALID, NO INHERIT support */
 					processCASbits($3, @3, "NOT NULL",
 								   NULL, NULL, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, NULL, yyscanner);
 					n->initially_valid = true;
 					$$ = (Node *) n;
 				}
@@ -6043,6 +6076,8 @@ CreateTrigStmt:
 			EXECUTE FUNCTION_or_PROCEDURE func_name '(' TriggerFuncArgs ')'
 				{
 					CreateTrigStmt *n = makeNode(CreateTrigStmt);
+					bool	dummy;
+					CAS_flags	seen;
 
 					n->replace = $2;
 					if (n->replace) /* not supported, see CreateTrigger */
@@ -6061,9 +6096,27 @@ CreateTrigStmt:
 					n->columns = (List *) lsecond($7);
 					n->whenClause = $15;
 					n->transitionRels = NIL;
-					processCASbits($11, @11, "TRIGGER",
-								   &n->deferrable, &n->initdeferred, NULL,
-								   NULL, NULL, yyscanner);
+					processCASbits($11, @11, NULL,
+								   &n->deferrable, &n->initdeferred, &dummy,
+								   &dummy, &dummy, &seen, yyscanner);
+					if (seen.seen_valid)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("constraint triggers cannot be marked %s",
+									   "NOT VALID"),
+								parser_errposition(@11));
+					if (seen.seen_inherit)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("constraint triggers cannot be marked %s",
+									   "INHERIT/NO INHERIT"),
+								parser_errposition(@11));
+					if (seen.seen_enforced)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("constraint triggers cannot be marked %s",
+									   "ENFORCED/NOT ENFORCED"),
+								parser_errposition(@11));
 					n->constrrel = $10;
 					$$ = (Node *) n;
 				}
@@ -19508,11 +19561,19 @@ SplitColQualList(List *qualList,
  * Process result of ConstraintAttributeSpec, and set appropriate bool flags
  * in the output command node.  Pass NULL for any flags the particular
  * command doesn't support.
+ *
+ * We optionally set which options were seen.  This allows to distinguish the
+ * case of an option being given its default value from the case where an
+ * option was not given at all.  In this case, no constrType string must be
+ * passed, and we throw no errors about unsupported flags.  Caller is
+ * responsible for checking unsupported flags having been given and throwing
+ * errors about them.
  */
 static void
 processCASbits(int cas_bits, int location, const char *constrType,
 			   bool *deferrable, bool *initdeferred, bool *is_enforced,
-			   bool *not_valid, bool *no_inherit, core_yyscan_t yyscanner)
+			   bool *not_valid, bool *no_inherit,
+			   CAS_flags *seen, core_yyscan_t yyscanner)
 {
 	/* defaults */
 	if (deferrable)
@@ -19523,6 +19584,12 @@ processCASbits(int cas_bits, int location, const char *constrType,
 		*not_valid = false;
 	if (is_enforced)
 		*is_enforced = true;
+	if (no_inherit)
+		*no_inherit = false;
+
+	Assert((constrType == NULL) ^ (seen == NULL));
+	if (seen)
+		memset(seen, 0, sizeof(CAS_flags));
 
 	if (cas_bits & (CAS_DEFERRABLE | CAS_INITIALLY_DEFERRED))
 	{
@@ -19535,6 +19602,8 @@ processCASbits(int cas_bits, int location, const char *constrType,
 					 errmsg("%s constraints cannot be marked DEFERRABLE",
 							constrType),
 					 parser_errposition(location)));
+		if (seen)
+			seen->seen_deferrability = true;
 	}
 
 	if (cas_bits & CAS_INITIALLY_DEFERRED)
@@ -19548,8 +19617,13 @@ processCASbits(int cas_bits, int location, const char *constrType,
 					 errmsg("%s constraints cannot be marked DEFERRABLE",
 							constrType),
 					 parser_errposition(location)));
+		if (seen)
+			seen->seen_deferrability = true;
 	}
 
+	if (cas_bits & (CAS_NOT_DEFERRABLE) && seen)
+		seen->seen_deferrability = true;
+
 	if (cas_bits & CAS_NOT_VALID)
 	{
 		if (not_valid)
@@ -19561,6 +19635,8 @@ processCASbits(int cas_bits, int location, const char *constrType,
 					 errmsg("%s constraints cannot be marked NOT VALID",
 							constrType),
 					 parser_errposition(location)));
+		if (seen)
+			seen->seen_valid = true;
 	}
 
 	if (cas_bits & CAS_NO_INHERIT)
@@ -19574,6 +19650,8 @@ processCASbits(int cas_bits, int location, const char *constrType,
 					 errmsg("%s constraints cannot be marked NO INHERIT",
 							constrType),
 					 parser_errposition(location)));
+		if (seen)
+			seen->seen_inherit = true;
 	}
 
 	if (cas_bits & CAS_NOT_ENFORCED)
@@ -19596,6 +19674,8 @@ processCASbits(int cas_bits, int location, const char *constrType,
 		 */
 		if (not_valid)
 			*not_valid = true;
+		if (seen)
+			seen->seen_enforced = true;
 	}
 
 	if (cas_bits & CAS_ENFORCED)
@@ -19609,6 +19689,8 @@ processCASbits(int cas_bits, int location, const char *constrType,
 					 errmsg("%s constraints cannot be marked ENFORCED",
 							constrType),
 					 parser_errposition(location)));
+		if (seen)
+			seen->seen_enforced = true;
 	}
 }
 
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index 4f39100fcdf..df2c27dd7e7 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -745,11 +745,11 @@ ERROR:  misplaced NOT ENFORCED clause
 LINE 1: CREATE TABLE UNIQUE_NOTEN_TBL(i int UNIQUE NOT ENFORCED);
                                                    ^
 ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
-ERROR:  FOREIGN KEY constraints cannot be marked ENFORCED
+ERROR:  cannot alter constraint enforceability
 LINE 1: ...TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
                                                               ^
 ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORCED;
-ERROR:  FOREIGN KEY constraints cannot be marked NOT ENFORCED
+ERROR:  cannot alter constraint enforceability
 LINE 1: ...ABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORC...
                                                              ^
 DROP TABLE unique_tbl;
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 6a3374d5152..9d0f91a9039 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1284,11 +1284,11 @@ ERROR:  constraint declared INITIALLY DEFERRED must be DEFERRABLE
 LINE 1: ...e ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE INITIALLY ...
                                                              ^
 ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT;
-ERROR:  FOREIGN KEY constraints cannot be marked NO INHERIT
+ERROR:  cannot alter constraint inheritability
 LINE 1: ...ER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT...
                                                              ^
 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;
                                                              ^
 -- test order of firing of FK triggers when several RI-induced changes need to
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 247c67c32ae..15423749506 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2547,6 +2547,23 @@ select * from parted;
 
 drop table parted;
 drop function parted_trigfunc();
+-- constraint triggers
+create constraint trigger foo after insert on pg_class not valid for each row execute procedure test();
+ERROR:  constraint triggers cannot be marked NOT VALID
+LINE 1: ...e constraint trigger foo after insert on pg_class not valid ...
+                                                             ^
+create constraint trigger foo after insert on pg_class no inherit for each row execute procedure test();
+ERROR:  constraint triggers cannot be marked INHERIT/NO INHERIT
+LINE 1: ...e constraint trigger foo after insert on pg_class no inherit...
+                                                             ^
+create constraint trigger foo after insert on pg_class enforced for each row execute procedure test();
+ERROR:  constraint triggers cannot be marked ENFORCED/NOT ENFORCED
+LINE 1: ...e constraint trigger foo after insert on pg_class enforced f...
+                                                             ^
+create constraint trigger foo after insert on pg_class not enforced for each row execute procedure test();
+ERROR:  constraint triggers cannot be marked ENFORCED/NOT ENFORCED
+LINE 1: ...e constraint trigger foo after insert on pg_class not enforc...
+                                                             ^
 --
 -- Constraint triggers and partitioned tables
 create table parted_constr_ancestor (a int, b text)
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 659972f1135..c10f5eac74e 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1764,6 +1764,12 @@ select * from parted;
 drop table parted;
 drop function parted_trigfunc();
 
+-- constraint triggers
+create constraint trigger foo after insert on pg_class not valid for each row execute procedure test();
+create constraint trigger foo after insert on pg_class no inherit for each row execute procedure test();
+create constraint trigger foo after insert on pg_class enforced for each row execute procedure test();
+create constraint trigger foo after insert on pg_class not enforced for each row execute procedure test();
+
 --
 -- Constraint triggers and partitioned tables
 create table parted_constr_ancestor (a int, b text)
-- 
2.39.5

Álvaro Herrera

alvherre@alvh.no-ip.org

10 months ago

In reply to: Álvaro Herrera (#5)

2 attachment(s)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

Hello,

I fleshed this out more fully and I think 0001 is good enough to commit.

I then noticed that constraints on domains are giving bogus error
messages as well, and the situation is easily improved -- see 0002. I'm
not so sure about this one, mainly because test coverage appears scant.
I need to look into this one a bit more.

Thanks

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

Attachments:

v2-0001-Improve-processCASbits-API-with-a-seen-struct.patchtext/x-diff; charset=utf-8Download

From 83c8a41c7d9d41fb63a82fc14c2dd66f30753a48 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@alvh.no-ip.org>
Date: Thu, 6 Mar 2025 17:01:22 +0100
Subject: [PATCH v2 1/2] Improve processCASbits API with a 'seen' struct

This allows ALTER TABLE .. ALTER CONSTRAINT to be more precise about
operations that are supported or not, as well as the reports from CREATE
CONSTRAINT TRIGGER error messages making more sense.
---
 src/backend/parser/gram.y                 | 152 +++++++++++++++++-----
 src/test/regress/expected/constraints.out |   4 +-
 src/test/regress/expected/foreign_key.out |   4 +-
 src/test/regress/expected/triggers.out    |  17 +++
 src/test/regress/sql/triggers.sql         |   6 +
 5 files changed, 148 insertions(+), 35 deletions(-)

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 271ae26cbaf..50024aabbca 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -146,6 +146,17 @@ typedef struct KeyActions
 #define CAS_NOT_ENFORCED			0x40
 #define CAS_ENFORCED				0x80
 
+/*
+ * We represent whether each set of flags is seen on a command with CAS_flags.
+ */
+typedef struct CAS_flags
+{
+	bool	seen_deferrability;
+	bool	seen_enforced;
+	bool	seen_valid;
+	bool	seen_inherit;
+} CAS_flags;
+
 
 #define parser_yyerror(msg)  scanner_yyerror(msg, yyscanner)
 #define parser_errposition(pos)  scanner_errposition(pos, yyscanner)
@@ -198,8 +209,9 @@ static void SplitColQualList(List *qualList,
 							 List **constraintList, CollateClause **collClause,
 							 core_yyscan_t yyscanner);
 static void processCASbits(int cas_bits, int location, const char *constrType,
-			   bool *deferrable, bool *initdeferred, bool *is_enforced,
-			   bool *not_valid, bool *no_inherit, core_yyscan_t yyscanner);
+						   bool *deferrable, bool *initdeferred, bool *is_enforced,
+						   bool *not_valid, bool *no_inherit, CAS_flags *seen,
+						   core_yyscan_t yyscanner);
 static PartitionStrategy parsePartitionStrategy(char *strategy, int location,
 												core_yyscan_t yyscanner);
 static void preprocess_pubobj_list(List *pubobjspec_list,
@@ -2658,15 +2670,35 @@ alter_table_cmd:
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 					ATAlterConstraint *c = makeNode(ATAlterConstraint);
+					CAS_flags	seen;
 
 					n->subtype = AT_AlterConstraint;
 					n->def = (Node *) c;
 					c->conname = $3;
-					c->alterDeferrability = true;
-					processCASbits($4, @4, "FOREIGN KEY",
-									&c->deferrable,
-									&c->initdeferred,
-									NULL, NULL, NULL, yyscanner);
+					processCASbits($4, @4, NULL,
+								   &c->deferrable,
+								   &c->initdeferred,
+								   NULL, NULL, NULL,
+								   &seen,
+								   yyscanner);
+					if (seen.seen_deferrability)
+						c->alterDeferrability = true;
+					/* cannot (currently) be changed by this syntax: */
+					if (seen.seen_enforced)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("cannot alter constraint enforceability"),
+								parser_errposition(@4));
+					if (seen.seen_valid)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("cannot alter constraint validity"),
+								parser_errposition(@4));
+					if (seen.seen_inherit)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("cannot alter constraint inheritability"),
+								parser_errposition(@4));
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER CONSTRAINT SET INHERIT */
@@ -4211,7 +4243,7 @@ ConstraintElem:
 					n->cooked_expr = NULL;
 					processCASbits($5, @5, "CHECK",
 								   NULL, NULL, &n->is_enforced, &n->skip_validation,
-								   &n->is_no_inherit, yyscanner);
+								   &n->is_no_inherit, NULL, yyscanner);
 					n->initially_valid = !n->skip_validation;
 					$$ = (Node *) n;
 				}
@@ -4225,7 +4257,7 @@ ConstraintElem:
 					/* no NOT VALID support yet */
 					processCASbits($4, @4, "NOT NULL",
 								   NULL, NULL, NULL, NULL,
-								   &n->is_no_inherit, yyscanner);
+								   &n->is_no_inherit, NULL, yyscanner);
 					n->initially_valid = true;
 					$$ = (Node *) n;
 				}
@@ -4245,7 +4277,7 @@ ConstraintElem:
 					n->indexspace = $9;
 					processCASbits($10, @10, "UNIQUE",
 								   &n->deferrable, &n->initdeferred, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, NULL, yyscanner);
 					$$ = (Node *) n;
 				}
 			| UNIQUE ExistingIndex ConstraintAttributeSpec
@@ -4261,7 +4293,7 @@ ConstraintElem:
 					n->indexspace = NULL;
 					processCASbits($3, @3, "UNIQUE",
 								   &n->deferrable, &n->initdeferred, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, NULL, yyscanner);
 					$$ = (Node *) n;
 				}
 			| PRIMARY KEY '(' columnList opt_without_overlaps ')' opt_c_include opt_definition OptConsTableSpace
@@ -4279,7 +4311,7 @@ ConstraintElem:
 					n->indexspace = $9;
 					processCASbits($10, @10, "PRIMARY KEY",
 								   &n->deferrable, &n->initdeferred, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, NULL, yyscanner);
 					$$ = (Node *) n;
 				}
 			| PRIMARY KEY ExistingIndex ConstraintAttributeSpec
@@ -4295,7 +4327,7 @@ ConstraintElem:
 					n->indexspace = NULL;
 					processCASbits($4, @4, "PRIMARY KEY",
 								   &n->deferrable, &n->initdeferred, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, NULL, yyscanner);
 					$$ = (Node *) n;
 				}
 			| EXCLUDE access_method_clause '(' ExclusionConstraintList ')'
@@ -4315,7 +4347,7 @@ ConstraintElem:
 					n->where_clause = $9;
 					processCASbits($10, @10, "EXCLUDE",
 								   &n->deferrable, &n->initdeferred, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, NULL, yyscanner);
 					$$ = (Node *) n;
 				}
 			| FOREIGN KEY '(' columnList optionalPeriodName ')' REFERENCES qualified_name
@@ -4345,7 +4377,7 @@ ConstraintElem:
 					processCASbits($12, @12, "FOREIGN KEY",
 								   &n->deferrable, &n->initdeferred,
 								   NULL, &n->skip_validation, NULL,
-								   yyscanner);
+								   NULL, yyscanner);
 					n->initially_valid = !n->skip_validation;
 					$$ = (Node *) n;
 				}
@@ -4383,9 +4415,9 @@ DomainConstraintElem:
 					n->location = @1;
 					n->raw_expr = $3;
 					n->cooked_expr = NULL;
-					processCASbits($5, @5, "CHECK",
+					processCASbits($5, @5, "CHECK", /* FIXME */
 								   NULL, NULL, NULL, &n->skip_validation,
-								   &n->is_no_inherit, yyscanner);
+								   &n->is_no_inherit, NULL, yyscanner);
 					n->is_enforced = true;
 					n->initially_valid = !n->skip_validation;
 					$$ = (Node *) n;
@@ -4398,9 +4430,9 @@ DomainConstraintElem:
 					n->location = @1;
 					n->keys = list_make1(makeString("value"));
 					/* no NOT VALID, NO INHERIT support */
-					processCASbits($3, @3, "NOT NULL",
+					processCASbits($3, @3, "NOT NULL", /* FIXME */
 								   NULL, NULL, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, NULL, yyscanner);
 					n->initially_valid = true;
 					$$ = (Node *) n;
 				}
@@ -6043,6 +6075,7 @@ CreateTrigStmt:
 			EXECUTE FUNCTION_or_PROCEDURE func_name '(' TriggerFuncArgs ')'
 				{
 					CreateTrigStmt *n = makeNode(CreateTrigStmt);
+					CAS_flags	seen;
 
 					n->replace = $2;
 					if (n->replace) /* not supported, see CreateTrigger */
@@ -6061,9 +6094,27 @@ CreateTrigStmt:
 					n->columns = (List *) lsecond($7);
 					n->whenClause = $15;
 					n->transitionRels = NIL;
-					processCASbits($11, @11, "TRIGGER",
+					processCASbits($11, @11, NULL,
 								   &n->deferrable, &n->initdeferred, NULL,
-								   NULL, NULL, yyscanner);
+								   NULL, NULL, &seen, yyscanner);
+					if (seen.seen_valid)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("constraint triggers cannot be marked %s",
+									   "NOT VALID"),
+								parser_errposition(@11));
+					if (seen.seen_inherit)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("constraint triggers cannot be marked %s",
+									   "INHERIT/NO INHERIT"),
+								parser_errposition(@11));
+					if (seen.seen_enforced)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("constraint triggers cannot be marked %s",
+									   "ENFORCED/NOT ENFORCED"),
+								parser_errposition(@11));
 					n->constrrel = $10;
 					$$ = (Node *) n;
 				}
@@ -19505,14 +19556,31 @@ SplitColQualList(List *qualList,
 }
 
 /*
- * Process result of ConstraintAttributeSpec, and set appropriate bool flags
- * in the output command node.  Pass NULL for any flags the particular
- * command doesn't support.
+ * Process result of ConstraintAttributeSpec, and set appropriate bool flags.
+ * Any of those flags can be given as NULL pointers, for options that are
+ * unsupported by the particular production being parsed.  If 'seen' is given
+ * as a non NULL pointer, the corresponding boolean there is set for every
+ * option in the command being parsed.
+ *
+ * Unsupported flags for a particular command can be handled in one of two
+ * ways.  Productions that require ad-hoc error reporting (those that don't
+ * know which type of constraint is being parsed or simply require a
+ * different phrasing than what this routine provides) can pass a valid
+ * 'seen' pointer; when that is given, each flag in that struct is set when
+ * a particular type of option appears in the command.  The production-
+ * specific case can inspect the 'seen' flags and complain appropriately if
+ * one option was seen that the command doesn't support.  In this case,
+ * 'constrType' must be given as NULL.
+ *
+ * The other option is to give a NULL 'seen' pointer.  In this case, an
+ * unsuported flag will give rise to an error report using the 'constrType',
+ * which must be given as not NULL.
  */
 static void
 processCASbits(int cas_bits, int location, const char *constrType,
 			   bool *deferrable, bool *initdeferred, bool *is_enforced,
-			   bool *not_valid, bool *no_inherit, core_yyscan_t yyscanner)
+			   bool *not_valid, bool *no_inherit,
+			   CAS_flags *seen, core_yyscan_t yyscanner)
 {
 	/* defaults */
 	if (deferrable)
@@ -19523,70 +19591,90 @@ processCASbits(int cas_bits, int location, const char *constrType,
 		*not_valid = false;
 	if (is_enforced)
 		*is_enforced = true;
+	if (no_inherit)
+		*no_inherit = false;
+
+	Assert((constrType == NULL) ^ (seen == NULL));
+	if (seen)
+		memset(seen, 0, sizeof(CAS_flags));
 
 	if (cas_bits & (CAS_DEFERRABLE | CAS_INITIALLY_DEFERRED))
 	{
 		if (deferrable)
 			*deferrable = true;
-		else
+		else if (!seen)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 			/* translator: %s is CHECK, UNIQUE, or similar */
 					 errmsg("%s constraints cannot be marked DEFERRABLE",
 							constrType),
 					 parser_errposition(location)));
+		if (seen)
+			seen->seen_deferrability = true;
 	}
 
 	if (cas_bits & CAS_INITIALLY_DEFERRED)
 	{
 		if (initdeferred)
 			*initdeferred = true;
-		else
+		else if (!seen)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 			/* translator: %s is CHECK, UNIQUE, or similar */
 					 errmsg("%s constraints cannot be marked DEFERRABLE",
 							constrType),
 					 parser_errposition(location)));
+		if (seen)
+			seen->seen_deferrability = true;
 	}
 
+	/* not deferrable is the default; just report that we saw it */
+	if (cas_bits & (CAS_NOT_DEFERRABLE) && seen)
+		seen->seen_deferrability = true;
+
 	if (cas_bits & CAS_NOT_VALID)
 	{
 		if (not_valid)
 			*not_valid = true;
-		else
+		else if (!seen)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 			/* translator: %s is CHECK, UNIQUE, or similar */
 					 errmsg("%s constraints cannot be marked NOT VALID",
 							constrType),
 					 parser_errposition(location)));
+		if (seen)
+			seen->seen_valid = true;
 	}
 
 	if (cas_bits & CAS_NO_INHERIT)
 	{
 		if (no_inherit)
 			*no_inherit = true;
-		else
+		else if (!seen)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 			/* translator: %s is CHECK, UNIQUE, or similar */
 					 errmsg("%s constraints cannot be marked NO INHERIT",
 							constrType),
 					 parser_errposition(location)));
+		if (seen)
+			seen->seen_inherit = true;
 	}
 
 	if (cas_bits & CAS_NOT_ENFORCED)
 	{
 		if (is_enforced)
 			*is_enforced = false;
-		else
+		else if (!seen)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 					 /* translator: %s is CHECK, UNIQUE, or similar */
 					 errmsg("%s constraints cannot be marked NOT ENFORCED",
 							constrType),
 					 parser_errposition(location)));
+		if (seen)
+			seen->seen_enforced = true;
 
 		/*
 		 * NB: The validated status is irrelevant when the constraint is set to
@@ -19602,13 +19690,15 @@ processCASbits(int cas_bits, int location, const char *constrType,
 	{
 		if (is_enforced)
 			*is_enforced = true;
-		else
+		else if (!seen)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 					 /* translator: %s is CHECK, UNIQUE, or similar */
 					 errmsg("%s constraints cannot be marked ENFORCED",
 							constrType),
 					 parser_errposition(location)));
+		if (seen)
+			seen->seen_enforced = true;
 	}
 }
 
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index 4f39100fcdf..df2c27dd7e7 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -745,11 +745,11 @@ ERROR:  misplaced NOT ENFORCED clause
 LINE 1: CREATE TABLE UNIQUE_NOTEN_TBL(i int UNIQUE NOT ENFORCED);
                                                    ^
 ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
-ERROR:  FOREIGN KEY constraints cannot be marked ENFORCED
+ERROR:  cannot alter constraint enforceability
 LINE 1: ...TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
                                                               ^
 ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORCED;
-ERROR:  FOREIGN KEY constraints cannot be marked NOT ENFORCED
+ERROR:  cannot alter constraint enforceability
 LINE 1: ...ABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORC...
                                                              ^
 DROP TABLE unique_tbl;
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 6a3374d5152..9d0f91a9039 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1284,11 +1284,11 @@ ERROR:  constraint declared INITIALLY DEFERRED must be DEFERRABLE
 LINE 1: ...e ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE INITIALLY ...
                                                              ^
 ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT;
-ERROR:  FOREIGN KEY constraints cannot be marked NO INHERIT
+ERROR:  cannot alter constraint inheritability
 LINE 1: ...ER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT...
                                                              ^
 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;
                                                              ^
 -- test order of firing of FK triggers when several RI-induced changes need to
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 247c67c32ae..15423749506 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2547,6 +2547,23 @@ select * from parted;
 
 drop table parted;
 drop function parted_trigfunc();
+-- constraint triggers
+create constraint trigger foo after insert on pg_class not valid for each row execute procedure test();
+ERROR:  constraint triggers cannot be marked NOT VALID
+LINE 1: ...e constraint trigger foo after insert on pg_class not valid ...
+                                                             ^
+create constraint trigger foo after insert on pg_class no inherit for each row execute procedure test();
+ERROR:  constraint triggers cannot be marked INHERIT/NO INHERIT
+LINE 1: ...e constraint trigger foo after insert on pg_class no inherit...
+                                                             ^
+create constraint trigger foo after insert on pg_class enforced for each row execute procedure test();
+ERROR:  constraint triggers cannot be marked ENFORCED/NOT ENFORCED
+LINE 1: ...e constraint trigger foo after insert on pg_class enforced f...
+                                                             ^
+create constraint trigger foo after insert on pg_class not enforced for each row execute procedure test();
+ERROR:  constraint triggers cannot be marked ENFORCED/NOT ENFORCED
+LINE 1: ...e constraint trigger foo after insert on pg_class not enforc...
+                                                             ^
 --
 -- Constraint triggers and partitioned tables
 create table parted_constr_ancestor (a int, b text)
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 659972f1135..c10f5eac74e 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1764,6 +1764,12 @@ select * from parted;
 drop table parted;
 drop function parted_trigfunc();
 
+-- constraint triggers
+create constraint trigger foo after insert on pg_class not valid for each row execute procedure test();
+create constraint trigger foo after insert on pg_class no inherit for each row execute procedure test();
+create constraint trigger foo after insert on pg_class enforced for each row execute procedure test();
+create constraint trigger foo after insert on pg_class not enforced for each row execute procedure test();
+
 --
 -- Constraint triggers and partitioned tables
 create table parted_constr_ancestor (a int, b text)
-- 
2.39.5

v2-0002-handle-constraints-on-domains-too.patchtext/x-diff; charset=utf-8Download

From 986b0bd3fb45f5c691e5a7affcdd626c67801dbc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@alvh.no-ip.org>
Date: Mon, 10 Mar 2025 18:27:19 +0100
Subject: [PATCH v2 2/2] handle constraints on domains too

---
 src/backend/parser/gram.y            | 51 +++++++++++++++++++++++++---
 src/test/regress/expected/domain.out | 22 ++++++++++--
 src/test/regress/sql/domain.sql      |  8 ++++-
 3 files changed, 72 insertions(+), 9 deletions(-)

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 50024aabbca..517f08fc7f5 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4410,29 +4410,70 @@ DomainConstraintElem:
 			CHECK '(' a_expr ')' ConstraintAttributeSpec
 				{
 					Constraint *n = makeNode(Constraint);
+					CAS_flags seen;
 
 					n->contype = CONSTR_CHECK;
 					n->location = @1;
 					n->raw_expr = $3;
 					n->cooked_expr = NULL;
-					processCASbits($5, @5, "CHECK", /* FIXME */
+					processCASbits($5, @5, NULL,
 								   NULL, NULL, NULL, &n->skip_validation,
-								   &n->is_no_inherit, NULL, yyscanner);
+								   &n->is_no_inherit, &seen, yyscanner);
 					n->is_enforced = true;
+					if (seen.seen_deferrability)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("specifying constraint deferrability not supported for domains"),
+								parser_errposition(@5));
+					if (seen.seen_enforced)
+						ereport(ERROR,
+							   errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							   errmsg("specifying constraint enforceability not supported for domains"),
+							   parser_errposition(@5));
+					if (seen.seen_inherit)
+						ereport(ERROR,
+							   errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							   errmsg("CHECK constraints for domains cannot be marked %s",
+									  "INHERIT / NO INHERIT"),
+							   parser_errposition(@5));
+
 					n->initially_valid = !n->skip_validation;
 					$$ = (Node *) n;
 				}
 			| NOT NULL_P ConstraintAttributeSpec
 				{
 					Constraint *n = makeNode(Constraint);
+					CAS_flags seen;
 
 					n->contype = CONSTR_NOTNULL;
 					n->location = @1;
 					n->keys = list_make1(makeString("value"));
-					/* no NOT VALID, NO INHERIT support */
-					processCASbits($3, @3, "NOT NULL", /* FIXME */
+					/* no DEFERRABLE, NOT VALID, NO INHERIT support */
+					processCASbits($3, @3, NULL,
 								   NULL, NULL, NULL,
-								   NULL, NULL, NULL, yyscanner);
+								   NULL, NULL, &seen, yyscanner);
+					if (seen.seen_valid)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("not-null constraints on domains cannot be marked %s",
+									   "NOT VALID"),
+								parser_errposition(@3));
+					if (seen.seen_deferrability)
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("specifying constraint deferrability not supported for domains"),
+								parser_errposition(@3));
+					if (seen.seen_enforced)
+						ereport(ERROR,
+							   errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							   errmsg("specifying constraint enforceability not supported for domains"),
+							   parser_errposition(@3));
+					if (seen.seen_inherit)
+						ereport(ERROR,
+							   errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							   errmsg("%s constraints on domains cannot be marked %s",
+									  "NOT NULL", "INHERIT / NOT INHERIT"),
+							   parser_errposition(@3));
 					n->initially_valid = true;
 					$$ = (Node *) n;
 				}
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index ba6f05eeb7d..bdb568a9d59 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -1361,15 +1361,31 @@ ERROR:  specifying constraint enforceability not supported for domains
 LINE 1: ...S int CONSTRAINT the_constraint CHECK (value > 0) NOT ENFORC...
                                                              ^
 CREATE DOMAIN constraint_enforced_dom AS int;
--- XXX misleading error messages
 ALTER DOMAIN constraint_enforced_dom ADD CONSTRAINT the_constraint CHECK (value > 0) ENFORCED;
-ERROR:  CHECK constraints cannot be marked ENFORCED
+ERROR:  specifying constraint enforceability not supported for domains
 LINE 1: ...om ADD CONSTRAINT the_constraint CHECK (value > 0) ENFORCED;
                                                               ^
 ALTER DOMAIN constraint_enforced_dom ADD CONSTRAINT the_constraint CHECK (value > 0) NOT ENFORCED;
-ERROR:  CHECK constraints cannot be marked NOT ENFORCED
+ERROR:  specifying constraint enforceability not supported for domains
 LINE 1: ...m ADD CONSTRAINT the_constraint CHECK (value > 0) NOT ENFORC...
                                                              ^
+-- other constraint properties also disallowed
+ALTER DOMAIN constraint_enforced_dom ADD CONSTRAINT the_constraint CHECK (value > 0) DEFERRABLE;
+ERROR:  specifying constraint deferrability not supported for domains
+LINE 1: ...m ADD CONSTRAINT the_constraint CHECK (value > 0) DEFERRABLE...
+                                                             ^
+ALTER DOMAIN constraint_enforced_dom ADD CONSTRAINT the_constraint CHECK (value > 0) NOT DEFERRABLE;
+ERROR:  specifying constraint deferrability not supported for domains
+LINE 1: ...m ADD CONSTRAINT the_constraint CHECK (value > 0) NOT DEFERR...
+                                                             ^
+ALTER DOMAIN constraint_enforced_dom ADD CONSTRAINT the_constraint CHECK (value > 0) INITIALLY DEFERRED;
+ERROR:  specifying constraint deferrability not supported for domains
+LINE 1: ...m ADD CONSTRAINT the_constraint CHECK (value > 0) INITIALLY ...
+                                                             ^
+ALTER DOMAIN constraint_enforced_dom ADD CONSTRAINT the_constraint CHECK (value > 0) NO INHERIT;
+ERROR:  CHECK constraints for domains cannot be marked INHERIT / NO INHERIT
+LINE 1: ...m ADD CONSTRAINT the_constraint CHECK (value > 0) NO INHERIT...
+                                                             ^
 DROP DOMAIN constraint_enforced_dom;
 --
 -- Information schema
diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index b752a63ab5f..c83c136398a 100644
--- a/src/test/regress/sql/domain.sql
+++ b/src/test/regress/sql/domain.sql
@@ -886,9 +886,15 @@ drop domain mytext cascade;
 CREATE DOMAIN constraint_enforced_dom AS int CONSTRAINT the_constraint CHECK (value > 0) ENFORCED;
 CREATE DOMAIN constraint_not_enforced_dom AS int CONSTRAINT the_constraint CHECK (value > 0) NOT ENFORCED;
 CREATE DOMAIN constraint_enforced_dom AS int;
--- XXX misleading error messages
 ALTER DOMAIN constraint_enforced_dom ADD CONSTRAINT the_constraint CHECK (value > 0) ENFORCED;
 ALTER DOMAIN constraint_enforced_dom ADD CONSTRAINT the_constraint CHECK (value > 0) NOT ENFORCED;
+
+-- other constraint properties also disallowed
+ALTER DOMAIN constraint_enforced_dom ADD CONSTRAINT the_constraint CHECK (value > 0) DEFERRABLE;
+ALTER DOMAIN constraint_enforced_dom ADD CONSTRAINT the_constraint CHECK (value > 0) NOT DEFERRABLE;
+ALTER DOMAIN constraint_enforced_dom ADD CONSTRAINT the_constraint CHECK (value > 0) INITIALLY DEFERRED;
+ALTER DOMAIN constraint_enforced_dom ADD CONSTRAINT the_constraint CHECK (value > 0) NO INHERIT;
+
 DROP DOMAIN constraint_enforced_dom;
 
 --
-- 
2.39.5

Amul Sul

sulamul@gmail.com

10 months ago

In reply to: Álvaro Herrera (#6)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

On Mon, Mar 10, 2025 at 11:29 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Hello,

I fleshed this out more fully and I think 0001 is good enough to commit.

The approach looks good to me, but instead of adding a CAS_flags struct, could
we use macros like SEEN_DEFERRABILITY(bits), SEEN_ENFORCEABILITY(bits),
etc.? We can simply pass cas_bits to these macros, and to avoid the error
from processCASbits(), we can pass NULL for constrType.

Regards,
Amul

jian he

jian.universality@gmail.com

10 months ago

In reply to: Álvaro Herrera (#6)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

On Tue, Mar 11, 2025 at 1:58 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Hello,

I fleshed this out more fully and I think 0001 is good enough to commit.

I then noticed that constraints on domains are giving bogus error
messages as well, and the situation is easily improved -- see 0002. I'm
not so sure about this one, mainly because test coverage appears scant.
I need to look into this one a bit more.

hi.

this look a little strange?
if (cas_bits & (CAS_NOT_DEFERRABLE) && seen)
seen->seen_deferrability = true;

it should be
if ((cas_bits & CAS_NOT_DEFERRABLE) && seen)
seen->seen_deferrability = true;
?

typedef struct CAS_flags need add to typedefs.list

seems didn't cover "initially immediate" case for domain.
for example:
create domain d_int as int4;
--- the following two cases should fail.
alter domain d_int add constraint nn1 not null initially immediate;
alter domain d_int add constraint cc check(value > 1) initially immediate;

we can add the following into processCASbits to make it error out
if ((cas_bits & CAS_INITIALLY_IMMEDIATE) && seen)
seen->seen_deferrability = true;

create domain d_int as int4;
alter domain d_int add not null no inherit not valid;
ERROR: not-null constraints on domains cannot be marked NOT VALID
LINE 1: alter domain d_int add not null no inherit not valid;
^
If we can report an error like
"ERROR: NOT NULL constraints on domains cannot be marked INHERIT / NOT INHERIT"
That would be great.
just report the first constraint property that is not ok, but seems not doable.

Álvaro Herrera

alvherre@alvh.no-ip.org

10 months ago

In reply to: Amul Sul (#7)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

On 2025-Mar-11, Amul Sul wrote:

On Mon, Mar 10, 2025 at 11:29 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I fleshed this out more fully and I think 0001 is good enough to commit.

The approach looks good to me, but instead of adding a CAS_flags struct, could
we use macros like SEEN_DEFERRABILITY(bits), SEEN_ENFORCEABILITY(bits),
etc.? We can simply pass cas_bits to these macros, and to avoid the error
from processCASbits(), we can pass NULL for constrType.

Ah yeah, I thought of this too at first, but didn't actually code it
because I thought it'd be messier. Trying to do it naively doesn't
work, because it's not enough to test whether each bit is true or false
-- what you need to know is whether an option was specified for each
bit, in either direction. So we'd need a separate bitmask, we can't
pass the existing 'bits' mask. And at that point, it's not any better
to have a bitmask, and a stack-allocated struct of booleans is just
easier to write.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos. Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)

#10

Álvaro Herrera

alvherre@alvh.no-ip.org

10 months ago

In reply to: jian he (#8)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

On 2025-Mar-11, jian he wrote:

this look a little strange?
if (cas_bits & (CAS_NOT_DEFERRABLE) && seen)
seen->seen_deferrability = true;

it should be
if ((cas_bits & CAS_NOT_DEFERRABLE) && seen)
seen->seen_deferrability = true;

True. And since you mentioned CAS_INITIALLY_IMMEDIATE, it should really
be

/* These are the default values; just report that we saw them */
if ((cas_bits & (CAS_NOT_DEFERRABLE | CAS_INITIALLY_IMMEDIATE)) && seen)
seen->seen_deferrability = true;

typedef struct CAS_flags need add to typedefs.list

True.

seems didn't cover "initially immediate" case for domain.
for example:
create domain d_int as int4;
--- the following two cases should fail.
alter domain d_int add constraint nn1 not null initially immediate;
alter domain d_int add constraint cc check(value > 1) initially immediate;

Yeah, I thought at first you were right, but on further thought, do we
really want to do this? I mean, INITIALLY IMMEDIATE is the default
timing for a constraint, so why should we complain if a user wants to
get a constraint that's declared that way? I'm not sure that we should
do it. Same with NOT DEFERRABLE.
[... looks at the standard doc ...]
And that's indeed what the SQL standard says:

<domain definition> ::=
CREATE DOMAIN <domain name> [ AS ] <predefined type>
[ <default clause> ]
[ <domain constraint>... ]
[ <collate clause> ]

<domain constraint> ::=
[ <constraint name definition> ] <check constraint definition> [ <constraint characteristics> ]

8) For every <domain constraint> specified:
[...]
b) If <constraint characteristics> is not specified, then INITIALLY IMMEDIATE
NOT DEFERRABLE is implicit.

So I think the fix here needs to distinguish those cases and avoid
throwing errors for them.

(Note also it does not say that INITIALLY DEFERRED or DEFERRABLE are
disallowed, which means that we're failing to fully implement the
standard-mandated behavior by prohibiting those.)

create domain d_int as int4;
alter domain d_int add not null no inherit not valid;
ERROR: not-null constraints on domains cannot be marked NOT VALID
LINE 1: alter domain d_int add not null no inherit not valid;
^
If we can report an error like
"ERROR: NOT NULL constraints on domains cannot be marked INHERIT / NOT INHERIT"
That would be great.
just report the first constraint property that is not ok, but seems not doable.

Yeah, I don't think this can be made to work. Maybe we'd have to change
the way ConstraintAttributeSpec is parsed completely.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL. This is by far the most pleasant management experience of
any database I've worked on." (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php

#11

Amul Sul

sulamul@gmail.com

10 months ago

In reply to: Álvaro Herrera (#9)

1 attachment(s)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

On Tue, Mar 11, 2025 at 1:56 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2025-Mar-11, Amul Sul wrote:

On Mon, Mar 10, 2025 at 11:29 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I fleshed this out more fully and I think 0001 is good enough to commit.

The approach looks good to me, but instead of adding a CAS_flags struct, could
we use macros like SEEN_DEFERRABILITY(bits), SEEN_ENFORCEABILITY(bits),
etc.? We can simply pass cas_bits to these macros, and to avoid the error
from processCASbits(), we can pass NULL for constrType.

Ah yeah, I thought of this too at first, but didn't actually code it
because I thought it'd be messier. Trying to do it naively doesn't
work, because it's not enough to test whether each bit is true or false
-- what you need to know is whether an option was specified for each
bit, in either direction. So we'd need a separate bitmask, we can't
pass the existing 'bits' mask. And at that point, it's not any better
to have a bitmask, and a stack-allocated struct of booleans is just
easier to write.

I was thinking of something like the attached, which includes your
test cases from 0001. Perhaps the macro name could be improved.

Regards,
Amul

Attachments:

v2-0001-trial.patchapplication/octet-stream; name=v2-0001-trial.patchDownload

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 271ae26cbaf..06b434a9445 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -146,6 +146,17 @@ typedef struct KeyActions
 #define CAS_NOT_ENFORCED			0x40
 #define CAS_ENFORCED				0x80
 
+#define ALTER_DEFERRABILITY(bits) \
+	(((bits) & (CAS_NOT_DEFERRABLE | CAS_DEFERRABLE | CAS_INITIALLY_IMMEDIATE  | \
+ 			CAS_INITIALLY_DEFERRED)) != 0)
+
+#define ALTER_VALID(bits) (((bits) & CAS_NOT_VALID) != 0)
+
+#define ALTER_INHERIT(bits) (((bits) & CAS_NO_INHERIT) != 0)
+
+#define ALTER_ENFORCEABILITY(bits) \
+	(((bits) & (CAS_NOT_ENFORCED | CAS_ENFORCED)) != 0)
+
 
 #define parser_yyerror(msg)  scanner_yyerror(msg, yyscanner)
 #define parser_errposition(pos)  scanner_errposition(pos, yyscanner)
@@ -2662,11 +2673,27 @@ alter_table_cmd:
 					n->subtype = AT_AlterConstraint;
 					n->def = (Node *) c;
 					c->conname = $3;
-					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));
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER CONSTRAINT SET INHERIT */
@@ -6061,9 +6088,27 @@ CreateTrigStmt:
 					n->columns = (List *) lsecond($7);
 					n->whenClause = $15;
 					n->transitionRels = NIL;
-					processCASbits($11, @11, "TRIGGER",
+					processCASbits($11, @11, NULL,
 								   &n->deferrable, &n->initdeferred, NULL,
 								   NULL, NULL, yyscanner);
+					if (ALTER_ENFORCEABILITY($11))
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("constraint triggers cannot be marked %s",
+									   "ENFORCED/NOT ENFORCED"),
+								parser_errposition(@11));
+					if (ALTER_VALID($11))
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("constraint triggers cannot be marked %s",
+									   "NOT VALID"),
+								parser_errposition(@11));
+					if (ALTER_INHERIT($11))
+						ereport(ERROR,
+								errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								errmsg("constraint triggers cannot be marked %s",
+									   "INHERIT/NO INHERIT"),
+								parser_errposition(@11));
 					n->constrrel = $10;
 					$$ = (Node *) n;
 				}
@@ -19528,7 +19573,7 @@ processCASbits(int cas_bits, int location, const char *constrType,
 	{
 		if (deferrable)
 			*deferrable = true;
-		else
+		else if (constrType)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 			/* translator: %s is CHECK, UNIQUE, or similar */
@@ -19541,7 +19586,7 @@ processCASbits(int cas_bits, int location, const char *constrType,
 	{
 		if (initdeferred)
 			*initdeferred = true;
-		else
+		else if (constrType)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 			/* translator: %s is CHECK, UNIQUE, or similar */
@@ -19554,7 +19599,7 @@ processCASbits(int cas_bits, int location, const char *constrType,
 	{
 		if (not_valid)
 			*not_valid = true;
-		else
+		else if (constrType)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 			/* translator: %s is CHECK, UNIQUE, or similar */
@@ -19567,7 +19612,7 @@ processCASbits(int cas_bits, int location, const char *constrType,
 	{
 		if (no_inherit)
 			*no_inherit = true;
-		else
+		else if (constrType)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 			/* translator: %s is CHECK, UNIQUE, or similar */
@@ -19580,7 +19625,7 @@ processCASbits(int cas_bits, int location, const char *constrType,
 	{
 		if (is_enforced)
 			*is_enforced = false;
-		else
+		else if (constrType)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 					 /* translator: %s is CHECK, UNIQUE, or similar */
@@ -19602,7 +19647,7 @@ processCASbits(int cas_bits, int location, const char *constrType,
 	{
 		if (is_enforced)
 			*is_enforced = true;
-		else
+		else if (constrType)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 					 /* translator: %s is CHECK, UNIQUE, or similar */
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index 4f39100fcdf..df2c27dd7e7 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -745,11 +745,11 @@ ERROR:  misplaced NOT ENFORCED clause
 LINE 1: CREATE TABLE UNIQUE_NOTEN_TBL(i int UNIQUE NOT ENFORCED);
                                                    ^
 ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
-ERROR:  FOREIGN KEY constraints cannot be marked ENFORCED
+ERROR:  cannot alter constraint enforceability
 LINE 1: ...TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key ENFORCED;
                                                               ^
 ALTER TABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORCED;
-ERROR:  FOREIGN KEY constraints cannot be marked NOT ENFORCED
+ERROR:  cannot alter constraint enforceability
 LINE 1: ...ABLE unique_tbl ALTER CONSTRAINT unique_tbl_i_key NOT ENFORC...
                                                              ^
 DROP TABLE unique_tbl;
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 6a3374d5152..9d0f91a9039 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1284,11 +1284,11 @@ ERROR:  constraint declared INITIALLY DEFERRED must be DEFERRABLE
 LINE 1: ...e ALTER CONSTRAINT fktable_fk_fkey NOT DEFERRABLE INITIALLY ...
                                                              ^
 ALTER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT;
-ERROR:  FOREIGN KEY constraints cannot be marked NO INHERIT
+ERROR:  cannot alter constraint inheritability
 LINE 1: ...ER TABLE fktable ALTER CONSTRAINT fktable_fk_fkey NO INHERIT...
                                                              ^
 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;
                                                              ^
 -- test order of firing of FK triggers when several RI-induced changes need to
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 247c67c32ae..15423749506 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2547,6 +2547,23 @@ select * from parted;
 
 drop table parted;
 drop function parted_trigfunc();
+-- constraint triggers
+create constraint trigger foo after insert on pg_class not valid for each row execute procedure test();
+ERROR:  constraint triggers cannot be marked NOT VALID
+LINE 1: ...e constraint trigger foo after insert on pg_class not valid ...
+                                                             ^
+create constraint trigger foo after insert on pg_class no inherit for each row execute procedure test();
+ERROR:  constraint triggers cannot be marked INHERIT/NO INHERIT
+LINE 1: ...e constraint trigger foo after insert on pg_class no inherit...
+                                                             ^
+create constraint trigger foo after insert on pg_class enforced for each row execute procedure test();
+ERROR:  constraint triggers cannot be marked ENFORCED/NOT ENFORCED
+LINE 1: ...e constraint trigger foo after insert on pg_class enforced f...
+                                                             ^
+create constraint trigger foo after insert on pg_class not enforced for each row execute procedure test();
+ERROR:  constraint triggers cannot be marked ENFORCED/NOT ENFORCED
+LINE 1: ...e constraint trigger foo after insert on pg_class not enforc...
+                                                             ^
 --
 -- Constraint triggers and partitioned tables
 create table parted_constr_ancestor (a int, b text)
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 659972f1135..c10f5eac74e 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1764,6 +1764,12 @@ select * from parted;
 drop table parted;
 drop function parted_trigfunc();
 
+-- constraint triggers
+create constraint trigger foo after insert on pg_class not valid for each row execute procedure test();
+create constraint trigger foo after insert on pg_class no inherit for each row execute procedure test();
+create constraint trigger foo after insert on pg_class enforced for each row execute procedure test();
+create constraint trigger foo after insert on pg_class not enforced for each row execute procedure test();
+
 --
 -- Constraint triggers and partitioned tables
 create table parted_constr_ancestor (a int, b text)

#12

jian he

jian.universality@gmail.com

10 months ago

In reply to: Álvaro Herrera (#10)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

On Tue, Mar 11, 2025 at 6:21 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

seems didn't cover "initially immediate" case for domain.
for example:
create domain d_int as int4;
--- the following two cases should fail.
alter domain d_int add constraint nn1 not null initially immediate;
alter domain d_int add constraint cc check(value > 1) initially immediate;
Yeah, I thought at first you were right, but on further thought, do we
really want to do this? I mean, INITIALLY IMMEDIATE is the default
timing for a constraint, so why should we complain if a user wants to
get a constraint that's declared that way? I'm not sure that we should
do it. Same with NOT DEFERRABLE.
[... looks at the standard doc ...]
And that's indeed what the SQL standard says:

<domain definition> ::=
CREATE DOMAIN <domain name> [ AS ] <predefined type>
[ <default clause> ]
[ <domain constraint>... ]
[ <collate clause> ]

<domain constraint> ::=
[ <constraint name definition> ] <check constraint definition> [ <constraint characteristics> ]

8) For every <domain constraint> specified:
[...]
b) If <constraint characteristics> is not specified, then INITIALLY IMMEDIATE
NOT DEFERRABLE is implicit.

So I think the fix here needs to distinguish those cases and avoid
throwing errors for them.

(Note also it does not say that INITIALLY DEFERRED or DEFERRABLE are
disallowed, which means that we're failing to fully implement the
standard-mandated behavior by prohibiting those.)

I don't have a huge opinion though.
but it's better to align CREATE DOMAIN with ALTER DOMAIN.
For example, the following two logic should behave the same.

create domain d_int as int4 constraint nn1 not null initially immediate;
alter domain d_int add constraint nn1 not null initially immediate;

Also if we do not error out, then in the create_domain.sgml, alter_domain.sgml
<synopsis> section we should include these "useless" keywords.

#13

Álvaro Herrera

alvherre@alvh.no-ip.org

10 months ago

In reply to: jian he (#12)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

On 2025-Mar-11, jian he wrote:

but it's better to align CREATE DOMAIN with ALTER DOMAIN.
For example, the following two logic should behave the same.

create domain d_int as int4 constraint nn1 not null initially immediate;
alter domain d_int add constraint nn1 not null initially immediate;

Sure, they should.

Also if we do not error out, then in the create_domain.sgml, alter_domain.sgml
<synopsis> section we should include these "useless" keywords.

Yeah, I guess we should do that.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

#14

Álvaro Herrera

alvherre@alvh.no-ip.org

10 months ago

In reply to: Amul Sul (#11)

Re: bogus error message for ALTER TABLE ALTER CONSTRAINT

On 2025-Mar-11, Amul Sul wrote:

I was thinking of something like the attached, which includes your
test cases from 0001. Perhaps the macro name could be improved.

FWIW I like this general idea. I don't like the proposed names much
though, especially the abuse of ALL_CAPS; and because they operate on
the given bits themselves rather than the output of processCASbits(), I
would have these checks before doing anything else. (Also, for nicer
code layout I would perhaps make the macros static inline functions.)

I'm going to stay away from this for a bit, as I think this is of
somewhat secondary importance.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Every machine is a smoke machine if you operate it wrong enough."
https://twitter.com/libseybieda/status/1541673325781196801