From: Álvaro Herrera <alvherre@alvh.no-ip.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: bogus error message for ALTER TABLE ALTER CONSTRAINT
Date: Thu, 6 Mar 2025 18:04:04 +0100
Message-ID: <202503061704.jtff22lxpho2@alvherre.pgsql> (raw)
In-Reply-To: <195582.1741125028@sss.pgh.pa.us>
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.patch (text/x-diff, 15.7K) download
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.yindex 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.outindex 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.outindex 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.outindex 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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: bogus error message for ALTER TABLE ALTER CONSTRAINT
In-Reply-To: <202503061704.jtff22lxpho2@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox