From 98a3fc16f49dea24d34d1af257c5017281bfe27a Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Thu, 11 Nov 2021 11:07:02 +0100 Subject: [PATCH 2/2] Review changes --- doc/src/sgml/ref/create_table.sgml | 6 +-- src/backend/catalog/pg_constraint.c | 10 +++-- src/backend/commands/tablecmds.c | 29 +++++++----- src/backend/nodes/outfuncs.c | 12 +++-- src/backend/parser/gram.y | 54 ++++++++++++----------- src/backend/utils/adt/ri_triggers.c | 32 ++++++++------ src/backend/utils/adt/ruleutils.c | 13 +++--- src/include/catalog/pg_constraint.h | 2 +- src/include/nodes/parsenodes.h | 1 - src/test/regress/expected/foreign_key.out | 2 +- 10 files changed, 90 insertions(+), 71 deletions(-) diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml index c6a9072ca0..eee837d86e 100644 --- a/doc/src/sgml/ref/create_table.sgml +++ b/doc/src/sgml/ref/create_table.sgml @@ -106,7 +106,7 @@ { column_name | ( expression ) } [ opclass ] [ ASC | DESC ] [ NULLS { FIRST | LAST } ] -referential_action in an EXCLUDE constraint is: +referential_action in a FOREIGN KEY/REFERENCES constraint is: { NO ACTION | RESTRICT | CASCADE | SET NULL [ ( column_name [, ... ] ) ] | SET DEFAULT [ ( column_name [, ... ] ) ] } @@ -1175,7 +1175,7 @@ Parameters Set all of the referencing columns, or a specified subset of the referencing columns, to null. A subset of columns can only be - specified for ON DELETE triggers. + specified for ON DELETE actions. @@ -1186,7 +1186,7 @@ Parameters Set all of the referencing columns, or a specified subset of the referencing columns, to their default values. A subset of columns - can only be specified for ON DELETE triggers. + can only be specified for ON DELETE actions. (There must be a row in the referenced table matching the default values, if they are not null, or the operation will fail.) diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 00af9930f4..35496984b9 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -139,6 +139,7 @@ CreateConstraintEntry(const char *constraintName, fkdatums[i] = ObjectIdGetDatum(ffEqOp[i]); conffeqopArray = construct_array(fkdatums, foreignNKeys, OIDOID, sizeof(Oid), true, TYPALIGN_INT); + // FIXME: use null instead of empty array for standard behavior for (i = 0; i < numFkDeleteSetCols; i++) fkdatums[i] = Int16GetDatum(fkDeleteSetCols[i]); confdelsetcolsArray = construct_array(fkdatums, numFkDeleteSetCols, @@ -1170,8 +1171,9 @@ get_primary_key_attnos(Oid relid, bool deferrableOk, Oid *constraintOid) /* * Extract data from the pg_constraint tuple of a foreign-key constraint. * - * All arguments save the first are output arguments; fields other than - * numfks, conkey and confkey can be passed as NULL if caller doesn't need them. + * All arguments save the first are output arguments. All output arguments + * other than numfks, conkey and confkey can be passed as NULL if caller + * doesn't need them. */ void DeconstructFkConstraintRow(HeapTuple tuple, int *numfks, @@ -1277,10 +1279,12 @@ DeconstructFkConstraintRow(HeapTuple tuple, int *numfks, if (fk_del_set_cols) { int num_delete_cols = 0; + + // FIXME: use null instead of empty array for standard behavior adatum = SysCacheGetAttr(CONSTROID, tuple, Anum_pg_constraint_confdelsetcols, &isNull); if (isNull) - elog(ERROR, "null confdelsetcols for foreign-key constraint %u", constrId); + elog(ERROR, "null confdelsetcols for constraint %u", constrId); arr = DatumGetArrayTypeP(adatum); /* ensure not toasted */ if (ARR_NDIM(arr) != 0) { diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3ef5d6d285..d158fef975 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -484,8 +484,8 @@ static ObjectAddress addFkRecurseReferenced(List **wqueue, Constraint *fkconstra Oid *pfeqoperators, Oid *ppeqoperators, Oid *ffeqoperators, int numfkdelsetcols, int16 *fkdelsetcols, bool old_check_ok); -static void validateFkOnDeleteSetColumns(int numfks, int16 *fkattnums, - int numfksetcols, int16 *fksetcolsattnums, +static void validateFkOnDeleteSetColumns(int numfks, const int16 *fkattnums, + int numfksetcols, const int16 *fksetcolsattnums, List *fksetcols); static void addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, Relation pkrel, Oid indexOid, Oid parentConstr, @@ -9385,22 +9385,27 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * Verifies that columns used in ON DELETE SET NULL/DEFAULT (...) * column lists are valid. */ -void validateFkOnDeleteSetColumns( - int numfks, int16 *fkattnums, - int numfksetcols, int16 *fksetcolsattnums, - List *fksetcols) +void +validateFkOnDeleteSetColumns(int numfks, const int16 *fkattnums, + int numfksetcols, const int16 *fksetcolsattnums, + List *fksetcols) { - for (int i = 0; i < numfksetcols; i++) { - int setcol_attnum = fksetcolsattnums[i]; + for (int i = 0; i < numfksetcols; i++) + { + int16 setcol_attnum = fksetcolsattnums[i]; bool seen = false; - for (int j = 0; j < numfks; j++) { - if (fkattnums[j] == setcol_attnum) { + + for (int j = 0; j < numfks; j++) + { + if (fkattnums[j] == setcol_attnum) + { seen = true; break; } } - if (!seen) { + if (!seen) + { char *col = strVal(list_nth(fksetcols, i)); ereport(ERROR, (errcode(ERRCODE_INVALID_COLUMN_REFERENCE), @@ -10871,7 +10876,7 @@ ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, /* * transformColumnNameList - transform list of column names * - * Lookup each name and return its attnum and, if needed, type OID + * Lookup each name and return its attnum and, optionally, type OID */ static int transformColumnNameList(Oid relId, List *colList, diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 7225434494..58bebbaa2d 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -2871,10 +2871,11 @@ _outPLAssignStmt(StringInfo str, const PLAssignStmt *node) WRITE_LOCATION_FIELD(location); } +#if 0 static void _outAlterTableStmt(StringInfo str, const AlterTableStmt *node) { - WRITE_NODE_TYPE("ALTERTABLE"); + WRITE_NODE_TYPE("ALTERTABLESTMT"); WRITE_NODE_FIELD(relation); WRITE_NODE_FIELD(cmds); @@ -2885,7 +2886,7 @@ _outAlterTableStmt(StringInfo str, const AlterTableStmt *node) static void _outAlterTableCmd(StringInfo str, const AlterTableCmd *node) { - WRITE_NODE_TYPE("ALTERTABLE_CMD"); + WRITE_NODE_TYPE("ALTERTABLECMD"); WRITE_ENUM_FIELD(subtype, AlterTableType); WRITE_STRING_FIELD(name); @@ -2899,12 +2900,13 @@ _outAlterTableCmd(StringInfo str, const AlterTableCmd *node) static void _outRoleSpec(StringInfo str, const RoleSpec *node) { - WRITE_NODE_TYPE("ROLE_SPEC"); + WRITE_NODE_TYPE("ROLESPEC"); WRITE_ENUM_FIELD(roletype, RoleSpecType); WRITE_STRING_FIELD(rolename); - WRITE_INT_FIELD(location); + WRITE_LOCATION_FIELD(location); } +#endif static void _outFuncCall(StringInfo str, const FuncCall *node) @@ -4408,6 +4410,7 @@ outNode(StringInfo str, const void *obj) case T_PLAssignStmt: _outPLAssignStmt(str, obj); break; +#if 0 case T_AlterTableStmt: _outAlterTableStmt(str, obj); break; @@ -4417,6 +4420,7 @@ outNode(StringInfo str, const void *obj) case T_RoleSpec: _outRoleSpec(str, obj); break; +#endif case T_ColumnDef: _outColumnDef(str, obj); break; diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 7ee0414e0b..2a319eecda 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -3708,11 +3708,6 @@ ColConstraintElem: n->pk_attrs = $3; n->fk_matchtype = $4; n->fk_upd_action = ($5)->updateAction->action; - if (($5)->updateAction->cols != NIL) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("SET NULL/DEFAULT only supported for ON DELETE triggers"), - parser_errposition(@5))); n->fk_del_action = ($5)->deleteAction->action; n->fk_del_set_cols = ($5)->deleteAction->cols; n->skip_validation = false; @@ -3925,11 +3920,6 @@ ConstraintElem: n->pk_attrs = $8; n->fk_matchtype = $9; n->fk_upd_action = ($10)->updateAction->action; - if (($10)->updateAction->cols != NIL) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("SET NULL/DEFAULT only supported for ON DELETE triggers"), - parser_errposition(@10))); n->fk_del_action = ($10)->deleteAction->action; n->fk_del_set_cols = ($10)->deleteAction->cols; processCASbits($11, @11, "FOREIGN KEY", @@ -4012,17 +4002,17 @@ OptWhereClause: key_actions: key_update { - KeyActions *n = (KeyActions *) palloc(sizeof(KeyActions)); + KeyActions *n = palloc(sizeof(KeyActions)); n->updateAction = $1; - n->deleteAction = (KeyAction *) palloc(sizeof(KeyAction)); + n->deleteAction = palloc(sizeof(KeyAction)); n->deleteAction->action = FKCONSTR_ACTION_NOACTION; n->deleteAction->cols = NIL; $$ = n; } | key_delete { - KeyActions *n = (KeyActions *) palloc(sizeof(KeyActions)); - n->updateAction = (KeyAction *) palloc(sizeof(KeyAction)); + KeyActions *n = palloc(sizeof(KeyActions)); + n->updateAction = palloc(sizeof(KeyAction)); n->updateAction->action = FKCONSTR_ACTION_NOACTION; n->updateAction->cols = NIL; n->deleteAction = $1; @@ -4030,69 +4020,81 @@ key_actions: } | key_update key_delete { - KeyActions *n = (KeyActions *) palloc(sizeof(KeyActions)); + KeyActions *n = palloc(sizeof(KeyActions)); n->updateAction = $1; n->deleteAction = $2; $$ = n; } | key_delete key_update { - KeyActions *n = (KeyActions *) palloc(sizeof(KeyActions)); + KeyActions *n = palloc(sizeof(KeyActions)); n->updateAction = $2; n->deleteAction = $1; $$ = n; } | /*EMPTY*/ { - KeyActions *n = (KeyActions *) palloc(sizeof(KeyActions)); - n->updateAction = (KeyAction *) palloc(sizeof(KeyAction)); + KeyActions *n = palloc(sizeof(KeyActions)); + n->updateAction = palloc(sizeof(KeyAction)); n->updateAction->action = FKCONSTR_ACTION_NOACTION; n->updateAction->cols = NIL; - n->deleteAction = (KeyAction *) palloc(sizeof(KeyAction)); + n->deleteAction = palloc(sizeof(KeyAction)); n->deleteAction->action = FKCONSTR_ACTION_NOACTION; n->deleteAction->cols = NIL; $$ = n; } ; -key_update: ON UPDATE key_action { $$ = $3; } +key_update: ON UPDATE key_action + { + if (($3)->cols) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("a column list with %s is only supported for ON DELETE actions", + ($3)->action == FKCONSTR_ACTION_SETNULL ? "SET NULL" : "SET DEFAULT"), + parser_errposition(@1))); + $$ = $3; + } ; -key_delete: ON DELETE_P key_action { $$ = $3; } +key_delete: ON DELETE_P key_action + { + $$ = $3; + } ; key_action: NO ACTION { - KeyAction *n = (KeyAction *) palloc(sizeof(KeyAction)); + KeyAction *n = palloc(sizeof(KeyAction)); n->action = FKCONSTR_ACTION_NOACTION; n->cols = NIL; $$ = n; } | RESTRICT { - KeyAction *n = (KeyAction *) palloc(sizeof(KeyAction)); + KeyAction *n = palloc(sizeof(KeyAction)); n->action = FKCONSTR_ACTION_RESTRICT; n->cols = NIL; $$ = n; } | CASCADE { - KeyAction *n = (KeyAction *) palloc(sizeof(KeyAction)); + KeyAction *n = palloc(sizeof(KeyAction)); n->action = FKCONSTR_ACTION_CASCADE; n->cols = NIL; $$ = n; } | SET NULL_P opt_column_list { - KeyAction *n = (KeyAction *) palloc(sizeof(KeyAction)); + KeyAction *n = palloc(sizeof(KeyAction)); n->action = FKCONSTR_ACTION_SETNULL; n->cols = $3; $$ = n; } | SET DEFAULT opt_column_list { - KeyAction *n = (KeyAction *) palloc(sizeof(KeyAction)); + KeyAction *n = palloc(sizeof(KeyAction)); n->action = FKCONSTR_ACTION_SETDEFAULT; n->cols = $3; $$ = n; diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index a088622b8b..dc53ce6cad 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -72,15 +72,15 @@ #define RI_PLAN_CHECK_LOOKUPPK 1 #define RI_PLAN_CHECK_LOOKUPPK_FROM_PK 2 #define RI_PLAN_LAST_ON_PK RI_PLAN_CHECK_LOOKUPPK_FROM_PK -/* these queries are executed against the FK (referencing) table. */ -#define RI_PLAN_ONDELETE_CASCADE 3 -#define RI_PLAN_ONUPDATE_CASCADE 4 +/* these queries are executed against the FK (referencing) table: */ +#define RI_PLAN_ONDELETE_CASCADE 3 +#define RI_PLAN_ONUPDATE_CASCADE 4 /* the same plan can be used for both ON DELETE and ON UPDATE triggers. */ -#define RI_PLAN_ONTRIGGER_RESTRICT 5 -#define RI_PLAN_ONDELETE_SETNULL 6 -#define RI_PLAN_ONUPDATE_SETNULL 7 -#define RI_PLAN_ONDELETE_SETDEFAULT 8 -#define RI_PLAN_ONUPDATE_SETDEFAULT 9 +#define RI_PLAN_ONTRIGGER_RESTRICT 5 // XXX confusing name +#define RI_PLAN_ONDELETE_SETNULL 6 +#define RI_PLAN_ONUPDATE_SETNULL 7 +#define RI_PLAN_ONDELETE_SETDEFAULT 8 +#define RI_PLAN_ONUPDATE_SETDEFAULT 9 #define MAX_QUOTED_NAME_LEN (NAMEDATALEN*2+3) #define MAX_QUOTED_REL_NAME_LEN (MAX_QUOTED_NAME_LEN*2) @@ -1128,7 +1128,9 @@ ri_set(TriggerData *trigdata, bool is_set_null, int tgkind) appendStringInfo(&querybuf, "UPDATE %s%s SET", fk_only, fkrelname); - // Add assignment clauses + /* + * Add assignment clauses + */ querysep = ""; for (int i = 0; i < num_cols_to_set; i++) { @@ -1140,14 +1142,16 @@ ri_set(TriggerData *trigdata, bool is_set_null, int tgkind) querysep = ","; } - // Add WHERE clause + /* + * Add WHERE clause + */ qualsep = "WHERE"; for (int i = 0; i < riinfo->nkeys; i++) { - Oid pk_type = RIAttType(pk_rel, riinfo->pk_attnums[i]); - Oid fk_type = RIAttType(fk_rel, riinfo->fk_attnums[i]); - Oid pk_coll = RIAttCollation(pk_rel, riinfo->pk_attnums[i]); - Oid fk_coll = RIAttCollation(fk_rel, riinfo->fk_attnums[i]); + Oid pk_type = RIAttType(pk_rel, riinfo->pk_attnums[i]); + Oid fk_type = RIAttType(fk_rel, riinfo->fk_attnums[i]); + Oid pk_coll = RIAttCollation(pk_rel, riinfo->pk_attnums[i]); + Oid fk_coll = RIAttCollation(fk_rel, riinfo->fk_attnums[i]); quoteOneName(attname, RIAttName(fk_rel, riinfo->fk_attnums[i])); diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index d5d5320c10..062a5b7051 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -2262,12 +2262,6 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand, if (string) appendStringInfo(&buf, " ON UPDATE %s", string); - val = SysCacheGetAttr(CONSTROID, tup, - Anum_pg_constraint_confdelsetcols, &isnull); - if (isnull) - elog(ERROR, "null confdelsetcols for foreign key constraint %u", - constraintId); - switch (conForm->confdeltype) { case FKCONSTR_ACTION_NOACTION: @@ -2295,6 +2289,13 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand, appendStringInfo(&buf, " ON DELETE %s", string); /* Add columns specified to SET NULL or SET DEFAULT if provided. */ + // FIXME: use null instead of empty array for standard behavior + val = SysCacheGetAttr(CONSTROID, tup, + Anum_pg_constraint_confdelsetcols, &isnull); + if (isnull) + elog(ERROR, "null confdelsetcols for constraint %u", + constraintId); + if (ARR_NDIM(DatumGetArrayTypeP(val)) > 0) { appendStringInfo(&buf, " ("); diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h index 4e256d4d89..dd0d975468 100644 --- a/src/include/catalog/pg_constraint.h +++ b/src/include/catalog/pg_constraint.h @@ -142,7 +142,7 @@ CATALOG(pg_constraint,2606,ConstraintRelationId) * If a foreign key with an ON DELETE SET NULL/DEFAULT action, the subset * of conkey to updated. If empty, all columns should be updated. */ - Oid confdelsetcols[1]; + int16 confdelsetcols[1]; /* * If an exclusion constraint, the OIDs of the exclusion operators for diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index b682e9abb1..4c5a8a39bf 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -2302,7 +2302,6 @@ typedef struct Constraint char fk_upd_action; /* ON UPDATE action */ char fk_del_action; /* ON DELETE action */ List *fk_del_set_cols; /* ON DELETE SET NULL/DEFAULT (col1, col2) */ - List *old_conpfeqop; /* pg_constraint.conpfeqop of my former self */ Oid old_pktable_oid; /* pg_constraint.confrelid of my former * self */ diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index a2680dbc9d..4c5274983d 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -764,7 +764,7 @@ ERROR: column "bar" referenced in foreign key constraint does not exist CREATE TABLE FKTABLE (tid int, id int, foo int, FOREIGN KEY (tid, id) REFERENCES PKTABLE ON DELETE SET NULL (foo)); ERROR: column "foo" referenced in ON DELETE SET action must be part of foreign key CREATE TABLE FKTABLE (tid int, id int, foo int, FOREIGN KEY (tid, foo) REFERENCES PKTABLE ON UPDATE SET NULL (foo)); -ERROR: SET NULL/DEFAULT only supported for ON DELETE triggers +ERROR: a column list with SET NULL is only supported for ON DELETE actions LINE 1: ...oo int, FOREIGN KEY (tid, foo) REFERENCES PKTABLE ON UPDATE ... ^ CREATE TABLE FKTABLE ( -- 2.33.1