On Fri, Feb 22, 2019 at 1:12 PM Corey Huinker <corey.huin...@gmail.com>
wrote:
> On Fri, Feb 22, 2019 at 11:05 AM Peter Eisentraut <
> peter.eisentr...@2ndquadrant.com> wrote:
>
>> ri_triggers.c is endlessly long and repetitive. I want to clean it up a
>> bit (more).
>>
>
> Having just been down this road, I agree that a lot of cleanup is needed
> and possible.
>
>
>> I looked into all these switch cases for the unimplemented MATCH PARTIAL
>> option. I toyed around with how a MATCH PARTIAL implementation would
>> actually look like, and it likely wouldn't use the existing code
>> structure anyway, so let's just simplify this for now.
>>
>
> +1
>
>
>
>> Attached are some patches.
>
>
> I intend to look this over in much greater detail, but I did skim the code
> and it seems like you left the SET DEFAULT and SET NULL paths separate. In
> my attempt at statement level triggers I realized that they only differed
> by the one literal value, and parameterized the function.
>
>
I've looked it over more closely now and I think that it's a nice
improvement.
As I suspected, the code for SET NULL and SET DEFAULT are highly similar
(see .diff), the major difference being two constants, the order of some
variable declarations, and the recheck in the set-default case.
The changes were so simple that I felt remiss not adding the patch for you
(see .patch).
Passes make check.
diff --git a/set_null.c b/set_default.c
index bc323ec..b2dd91d 100644
--- a/set_null.c
+++ b/set_default.c
@@ -1,10 +1,10 @@
/*
- * ri_setnull -
+ * ri_setdefault -
*
- * Common code for ON DELETE SET NULL and ON UPDATE SET NULL
+ * Common code for ON DELETE SET DEFAULT and ON UPDATE SET DEFAULT
*/
static Datum
-ri_setnull(TriggerData *trigdata)
+ri_setdefault(TriggerData *trigdata)
{
const RI_ConstraintInfo *riinfo;
Relation fk_rel;
@@ -30,10 +30,10 @@ ri_setnull(TriggerData *trigdata)
elog(ERROR, "SPI_connect failed");
/*
- * Fetch or prepare a saved plan for the set null operation (it's
- * the same query for delete and update cases)
+ * Fetch or prepare a saved plan for the set default operation
+ * (it's the same query for delete and update cases)
*/
- ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_SETNULL_DOUPDATE);
+ ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_SETDEFAULT_DOUPDATE);
if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL)
{
@@ -44,12 +44,12 @@ ri_setnull(TriggerData *trigdata)
char paramname[16];
const char *querysep;
const char *qualsep;
- const char *fk_only;
Oid queryoids[RI_MAX_NUMKEYS];
+ const char *fk_only;
/* ----------
* The query string built is
- * UPDATE [ONLY] <fktable> SET fkatt1 = NULL [, ...]
+ * UPDATE [ONLY] <fktable> SET fkatt1 = DEFAULT [, ...]
* WHERE $1 = fkatt1 [AND ...]
* The type id's for the $ parameters are those of the
* corresponding PK attributes.
@@ -57,9 +57,9 @@ ri_setnull(TriggerData *trigdata)
*/
initStringInfo(&querybuf);
initStringInfo(&qualbuf);
+ quoteRelationName(fkrelname, fk_rel);
fk_only = fk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ?
"" : "ONLY ";
- quoteRelationName(fkrelname, fk_rel);
appendStringInfo(&querybuf, "UPDATE %s%s SET",
fk_only, fkrelname);
querysep = "";
@@ -72,9 +72,10 @@ ri_setnull(TriggerData *trigdata)
quoteOneName(attname,
RIAttName(fk_rel, riinfo->fk_attnums[i]));
appendStringInfo(&querybuf,
- "%s %s = NULL",
+ "%s %s = DEFAULT",
querysep, attname);
sprintf(paramname, "$%d", i + 1);
+ sprintf(paramname, "$%d", i + 1);
ri_GenerateQual(&qualbuf, qualsep,
paramname, pk_type,
riinfo->pf_eq_oprs[i],
@@ -104,5 +105,20 @@ ri_setnull(TriggerData *trigdata)
table_close(fk_rel, RowExclusiveLock);
- return PointerGetDatum(NULL);
+ /*
+ * If we just deleted or updated the PK row whose key was equal to
+ * the FK columns' default values, and a referencing row exists in
+ * the FK table, we would have updated that row to the same values
+ * it already had --- and RI_FKey_fk_upd_check_required would
+ * hence believe no check is necessary. So we need to do another
+ * lookup now and in case a reference still exists, abort the
+ * operation. That is already implemented in the NO ACTION
+ * trigger, so just run it. (This recheck is only needed in the
+ * SET DEFAULT case, since CASCADE would remove such rows in case
+ * of a DELETE operation or would change the FK key values in case
+ * of an UPDATE, while SET NULL is certain to result in rows that
+ * satisfy the FK constraint.)
+ */
+ return ri_restrict(trigdata, true);
}
+
From 0331d9e68bdf6c6c564ab1c13893690c1abd094b Mon Sep 17 00:00:00 2001
From: Corey Huinker <corey.huin...@gmail.com>
Date: Sat, 23 Feb 2019 23:29:29 +0000
Subject: [PATCH 4/4] Combine ri_setdefault and ri_setnull
---
src/backend/utils/adt/ri_triggers.c | 173 ++++++----------------------
1 file changed, 34 insertions(+), 139 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index bbea2e458f..d42e5a68fd 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -176,8 +176,7 @@ static bool ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel,
HeapTuple old_row,
const RI_ConstraintInfo *riinfo);
static Datum ri_restrict(TriggerData *trigdata, bool is_no_action);
-static Datum ri_setnull(TriggerData *trigdata);
-static Datum ri_setdefault(TriggerData *trigdata);
+static Datum ri_set(TriggerData *trigdata, bool set_to_null);
static void quoteOneName(char *buffer, const char *name);
static void quoteRelationName(char *buffer, Relation rel);
static void ri_GenerateQual(StringInfo buf,
@@ -955,7 +954,7 @@ RI_FKey_setnull_del(PG_FUNCTION_ARGS)
ri_CheckTrigger(fcinfo, "RI_FKey_setnull_del", RI_TRIGTYPE_DELETE);
/* Share code with UPDATE case */
- return ri_setnull((TriggerData *) fcinfo->context);
+ return ri_set((TriggerData *) fcinfo->context, true);
}
/*
@@ -970,16 +969,16 @@ RI_FKey_setnull_upd(PG_FUNCTION_ARGS)
ri_CheckTrigger(fcinfo, "RI_FKey_setnull_upd", RI_TRIGTYPE_UPDATE);
/* Share code with DELETE case */
- return ri_setnull((TriggerData *) fcinfo->context);
+ return ri_set((TriggerData *) fcinfo->context, true);
}
/*
- * ri_setnull -
+ * ri_set -
*
- * Common code for ON DELETE SET NULL and ON UPDATE SET NULL
+ * Common code for ON (DELETE|UPDATE) SET (NULL|DEFAULT)
*/
static Datum
-ri_setnull(TriggerData *trigdata)
+ri_set(TriggerData *trigdata, bool set_to_null)
{
const RI_ConstraintInfo *riinfo;
Relation fk_rel;
@@ -1005,10 +1004,12 @@ ri_setnull(TriggerData *trigdata)
elog(ERROR, "SPI_connect failed");
/*
- * Fetch or prepare a saved plan for the set null operation (it's
+ * Fetch or prepare a saved plan for the set null/default operation (it's
* the same query for delete and update cases)
*/
- ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_SETNULL_DOUPDATE);
+ ri_BuildQueryKey(&qkey, riinfo,
+ (set_to_null) ? RI_PLAN_SETNULL_DOUPDATE
+ : RI_PLAN_SETDEFAULT_DOUPDATE);
if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL)
{
@@ -1024,7 +1025,7 @@ ri_setnull(TriggerData *trigdata)
/* ----------
* The query string built is
- * UPDATE [ONLY] <fktable> SET fkatt1 = NULL [, ...]
+ * UPDATE [ONLY] <fktable> SET fkatt1 = (NULL|DEFAULT) [, ...]
* WHERE $1 = fkatt1 [AND ...]
* The type id's for the $ parameters are those of the
* corresponding PK attributes.
@@ -1047,8 +1048,9 @@ ri_setnull(TriggerData *trigdata)
quoteOneName(attname,
RIAttName(fk_rel, riinfo->fk_attnums[i]));
appendStringInfo(&querybuf,
- "%s %s = NULL",
- querysep, attname);
+ "%s %s = %s",
+ querysep, attname,
+ (set_to_null) ? "NULL" : "DEFAULT");
sprintf(paramname, "$%d", i + 1);
ri_GenerateQual(&qualbuf, qualsep,
paramname, pk_type,
@@ -1079,7 +1081,24 @@ ri_setnull(TriggerData *trigdata)
table_close(fk_rel, RowExclusiveLock);
- return PointerGetDatum(NULL);
+ if (set_to_null)
+ return PointerGetDatum(NULL);
+
+ /*
+ * If we just deleted or updated the PK row whose key was equal to
+ * the FK columns' default values, and a referencing row exists in
+ * the FK table, we would have updated that row to the same values
+ * it already had --- and RI_FKey_fk_upd_check_required would
+ * hence believe no check is necessary. So we need to do another
+ * lookup now and in case a reference still exists, abort the
+ * operation. That is already implemented in the NO ACTION
+ * trigger, so just run it. (This recheck is only needed in the
+ * SET DEFAULT case, since CASCADE would remove such rows in case
+ * of a DELETE operation or would change the FK key values in case
+ * of an UPDATE, while SET NULL is certain to result in rows that
+ * satisfy the FK constraint.)
+ */
+ return ri_restrict(trigdata, true);
}
@@ -1095,7 +1114,7 @@ RI_FKey_setdefault_del(PG_FUNCTION_ARGS)
ri_CheckTrigger(fcinfo, "RI_FKey_setdefault_del", RI_TRIGTYPE_DELETE);
/* Share code with UPDATE case */
- return ri_setdefault((TriggerData *) fcinfo->context);
+ return ri_set((TriggerData *) fcinfo->context, false);
}
/*
@@ -1110,133 +1129,9 @@ RI_FKey_setdefault_upd(PG_FUNCTION_ARGS)
ri_CheckTrigger(fcinfo, "RI_FKey_setdefault_upd", RI_TRIGTYPE_UPDATE);
/* Share code with DELETE case */
- return ri_setdefault((TriggerData *) fcinfo->context);
+ return ri_set((TriggerData *) fcinfo->context, false);
}
-/*
- * ri_setdefault -
- *
- * Common code for ON DELETE SET DEFAULT and ON UPDATE SET DEFAULT
- */
-static Datum
-ri_setdefault(TriggerData *trigdata)
-{
- const RI_ConstraintInfo *riinfo;
- Relation fk_rel;
- Relation pk_rel;
- HeapTuple old_row;
- RI_QueryKey qkey;
- SPIPlanPtr qplan;
-
- riinfo = ri_FetchConstraintInfo(trigdata->tg_trigger,
- trigdata->tg_relation, true);
-
- /*
- * Get the relation descriptors of the FK and PK tables and the old tuple.
- *
- * fk_rel is opened in RowExclusiveLock mode since that's what our
- * eventual UPDATE will get on it.
- */
- fk_rel = table_open(riinfo->fk_relid, RowExclusiveLock);
- pk_rel = trigdata->tg_relation;
- old_row = trigdata->tg_trigtuple;
-
- if (SPI_connect() != SPI_OK_CONNECT)
- elog(ERROR, "SPI_connect failed");
-
- /*
- * Fetch or prepare a saved plan for the set default operation
- * (it's the same query for delete and update cases)
- */
- ri_BuildQueryKey(&qkey, riinfo, RI_PLAN_SETDEFAULT_DOUPDATE);
-
- if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL)
- {
- StringInfoData querybuf;
- StringInfoData qualbuf;
- char fkrelname[MAX_QUOTED_REL_NAME_LEN];
- char attname[MAX_QUOTED_NAME_LEN];
- char paramname[16];
- const char *querysep;
- const char *qualsep;
- Oid queryoids[RI_MAX_NUMKEYS];
- const char *fk_only;
-
- /* ----------
- * The query string built is
- * UPDATE [ONLY] <fktable> SET fkatt1 = DEFAULT [, ...]
- * WHERE $1 = fkatt1 [AND ...]
- * The type id's for the $ parameters are those of the
- * corresponding PK attributes.
- * ----------
- */
- initStringInfo(&querybuf);
- initStringInfo(&qualbuf);
- quoteRelationName(fkrelname, fk_rel);
- fk_only = fk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ?
- "" : "ONLY ";
- appendStringInfo(&querybuf, "UPDATE %s%s SET",
- fk_only, fkrelname);
- querysep = "";
- 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]);
-
- quoteOneName(attname,
- RIAttName(fk_rel, riinfo->fk_attnums[i]));
- appendStringInfo(&querybuf,
- "%s %s = DEFAULT",
- querysep, attname);
- sprintf(paramname, "$%d", i + 1);
- ri_GenerateQual(&qualbuf, qualsep,
- paramname, pk_type,
- riinfo->pf_eq_oprs[i],
- attname, fk_type);
- querysep = ",";
- qualsep = "AND";
- queryoids[i] = pk_type;
- }
- appendStringInfoString(&querybuf, qualbuf.data);
-
- /* Prepare and save the plan */
- qplan = ri_PlanCheck(querybuf.data, riinfo->nkeys, queryoids,
- &qkey, fk_rel, pk_rel, true);
- }
-
- /*
- * We have a plan now. Run it to update the existing references.
- */
- ri_PerformCheck(riinfo, &qkey, qplan,
- fk_rel, pk_rel,
- old_row, NULL,
- true, /* must detect new rows */
- SPI_OK_UPDATE);
-
- if (SPI_finish() != SPI_OK_FINISH)
- elog(ERROR, "SPI_finish failed");
-
- table_close(fk_rel, RowExclusiveLock);
-
- /*
- * If we just deleted or updated the PK row whose key was equal to
- * the FK columns' default values, and a referencing row exists in
- * the FK table, we would have updated that row to the same values
- * it already had --- and RI_FKey_fk_upd_check_required would
- * hence believe no check is necessary. So we need to do another
- * lookup now and in case a reference still exists, abort the
- * operation. That is already implemented in the NO ACTION
- * trigger, so just run it. (This recheck is only needed in the
- * SET DEFAULT case, since CASCADE would remove such rows in case
- * of a DELETE operation or would change the FK key values in case
- * of an UPDATE, while SET NULL is certain to result in rows that
- * satisfy the FK constraint.)
- */
- return ri_restrict(trigdata, true);
-}
-
-
/*
* RI_FKey_pk_upd_check_required -
*
--
2.17.1
