Re: More efficient RI checks - take 2

Hi,

I was looking at this patch with Corey during a patch-review session. So
these are basically our "combined" comments.

On 2020-06-05 17:16:43 +0200, Antonin Houska wrote:
> From 6c1cb8ae7fbf0a8122d8c6637c61b9915bc57223 Mon Sep 17 00:00:00 2001
> From: Antonin Houska <ah(at)cybertec(dot)at>
> Date: Fri, 5 Jun 2020 16:42:34 +0200
> Subject: [PATCH 1/5] Check for RI violation outside ri_PerformCheck().

Probably good to add a short comment to the commit explaining why you're
doing this.

The change makes sense to me. Unless somebody protests I think we should
just apply it regardless of the rest of the series - the code seems
clearer afterwards.

> From 6b09e5598553c8e57b4ef9342912f51adb48f8af Mon Sep 17 00:00:00 2001
> From: Antonin Houska <ah(at)cybertec(dot)at>
> Date: Fri, 5 Jun 2020 16:42:34 +0200
> Subject: [PATCH 2/5] Changed ri_GenerateQual() so it generates the whole
> qualifier.
>
> This way we can use the function to reduce the amount of copy&pasted code a
> bit.

> /*
> - * ri_GenerateQual --- generate a WHERE clause equating two variables
> + * ri_GenerateQual --- generate WHERE/ON clause.
> + *
> + * Note: to avoid unnecessary explicit casts, make sure that the left and
> + * right operands match eq_oprs expect (ie don't swap the left and right
> + * operands accidentally).
> + */
> +static void
> +ri_GenerateQual(StringInfo buf, char *sep, int nkeys,
> + const char *ltabname, Relation lrel,
> + const int16 *lattnums,
> + const char *rtabname, Relation rrel,
> + const int16 *rattnums,
> + const Oid *eq_oprs,
> + GenQualParams params,
> + Oid *paramtypes)
> +{
> + for (int i = 0; i < nkeys; i++)
> + {
> + Oid ltype = RIAttType(lrel, lattnums[i]);
> + Oid rtype = RIAttType(rrel, rattnums[i]);
> + Oid lcoll = RIAttCollation(lrel, lattnums[i]);
> + Oid rcoll = RIAttCollation(rrel, rattnums[i]);
> + char paramname[16];
> + char *latt,
> + *ratt;
> + char *sep_current = i > 0 ? sep : NULL;
> +
> + if (params != GQ_PARAMS_NONE)
> + sprintf(paramname, "$%d", i + 1);
> +
> + if (params == GQ_PARAMS_LEFT)
> + {
> + latt = paramname;
> + paramtypes[i] = ltype;
> + }
> + else
> + latt = ri_ColNameQuoted(ltabname, RIAttName(lrel, lattnums[i]));
> +
> + if (params == GQ_PARAMS_RIGHT)
> + {
> + ratt = paramname;
> + paramtypes[i] = rtype;
> + }
> + else
> + ratt = ri_ColNameQuoted(rtabname, RIAttName(rrel, rattnums[i]));

Why do we need support for having params on left or right side, instead
of just having them on one side?

> + ri_GenerateQualComponent(buf, sep_current, latt, ltype, eq_oprs[i],
> + ratt, rtype);
> +
> + if (lcoll != rcoll)
> + ri_GenerateQualCollation(buf, lcoll);
> + }
> +}
> +
> +/*
> + * ri_GenerateQual --- generate a component of WHERE/ON clause equating two
> + * variables, to be AND-ed to the other components.
> *
> * This basically appends " sep leftop op rightop" to buf, adding casts
> * and schema qualification as needed to ensure that the parser will select
> @@ -1828,17 +1802,86 @@ quoteRelationName(char *buffer, Relation rel)
> * if they aren't variables or parameters.
> */
> static void
> -ri_GenerateQual(StringInfo buf,
> - const char *sep,
> - const char *leftop, Oid leftoptype,
> - Oid opoid,
> - const char *rightop, Oid rightoptype)
> +ri_GenerateQualComponent(StringInfo buf,
> + const char *sep,
> + const char *leftop, Oid leftoptype,
> + Oid opoid,
> + const char *rightop, Oid rightoptype)
> {
> - appendStringInfo(buf, " %s ", sep);
> + if (sep)
> + appendStringInfo(buf, " %s ", sep);
> generate_operator_clause(buf, leftop, leftoptype, opoid,
> rightop, rightoptype);
> }

Why is this handled inside ri_GenerateQualComponent() instead of
ri_GenerateQual()? Especially because the latter now has code to pass in
a different sep into ri_GenerateQualComponent().

> +/*
> + * ri_ColNameQuoted() --- return column name, with both table and column name
> + * quoted.
> + */
> +static char *
> +ri_ColNameQuoted(const char *tabname, const char *attname)
> +{
> + char quoted[MAX_QUOTED_NAME_LEN];
> + StringInfo result = makeStringInfo();
> +
> + if (tabname && strlen(tabname) > 0)
> + {
> + quoteOneName(quoted, tabname);
> + appendStringInfo(result, "%s.", quoted);
> + }
> +
> + quoteOneName(quoted, attname);
> + appendStringInfoString(result, quoted);
> +
> + return result->data;
> +}

Why does this new function accept a NULL / zero length string? I guess
that's because we currently don't qualify in all places?

> +/*
> + * Check that RI trigger function was called in expected context
> + */
> +static void
> +ri_CheckTrigger(FunctionCallInfo fcinfo, const char *funcname, int tgkind)
> +{
> + TriggerData *trigdata = (TriggerData *) fcinfo->context;
> +
> + if (!CALLED_AS_TRIGGER(fcinfo))
> + ereport(ERROR,
> + (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
> + errmsg("function \"%s\" was not called by trigger manager", funcname)));
> +
> + /*
> + * Check proper event
> + */
> + if (!TRIGGER_FIRED_AFTER(trigdata->tg_event) ||
> + !TRIGGER_FIRED_FOR_ROW(trigdata->tg_event))
> + ereport(ERROR,
> + (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
> + errmsg("function \"%s\" must be fired AFTER ROW", funcname)));
> +
> + switch (tgkind)
> + {
> + case RI_TRIGTYPE_INSERT:
> + if (!TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
> + ereport(ERROR,
> + (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
> + errmsg("function \"%s\" must be fired for INSERT", funcname)));
> + break;
> + case RI_TRIGTYPE_UPDATE:
> + if (!TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
> + ereport(ERROR,
> + (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
> + errmsg("function \"%s\" must be fired for UPDATE", funcname)));
> + break;
> +
> + case RI_TRIGTYPE_DELETE:
> + if (!TRIGGER_FIRED_BY_DELETE(trigdata->tg_event))
> + ereport(ERROR,
> + (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
> + errmsg("function \"%s\" must be fired for DELETE", funcname)));
> + break;
> + }
> +}
> +

Why did you move this around, as part of this commit?

> From 208c733d759592402901599446b4f7e7197c1777 Mon Sep 17 00:00:00 2001
> From: Antonin Houska <ah(at)cybertec(dot)at>
> Date: Fri, 5 Jun 2020 16:42:34 +0200
> Subject: [PATCH 4/5] Introduce infrastructure for batch processing RI events.
>
> Separate storage is used for the RI trigger events because the "transient
> table" that we provide to statement triggers would not be available for
> deferred constraints. Also, the regular statement level trigger is not ideal
> for the RI checks because it requires the query execution to complete before
> the RI checks even start. On the other hand, if we use batches of row trigger
> events, we only need to tune the batch size so that user gets RI violation
> error rather soon.
>
> This patch only introduces the infrastructure, however the trigger function is
> still called per event. This is just to reduce the size of the diffs.
> ---
> src/backend/commands/tablecmds.c | 68 +-
> src/backend/commands/trigger.c | 406 ++++++--
> src/backend/executor/spi.c | 16 +-
> src/backend/utils/adt/ri_triggers.c | 1385 +++++++++++++++++++--------
> src/include/commands/trigger.h | 25 +
> 5 files changed, 1381 insertions(+), 519 deletions(-)

My first comment here is that this is too large a change and should be
broken up.

I think there's also not enough explanation in here what the new design
is. I can infer some of that from the code, but that's imo shifting work
to the reviewer / reader unnecessarily.

> +static void AfterTriggerExecuteRI(EState *estate,
> + ResultRelInfo *relInfo,
> + FmgrInfo *finfo,
> + Instrumentation *instr,
> + TriggerData *trig_last,
> + MemoryContext batch_context);
> static AfterTriggersTableData *GetAfterTriggersTableData(Oid relid,
> CmdType cmdType);
> static void AfterTriggerFreeQuery(AfterTriggersQueryData *qs);
> @@ -3807,13 +3821,16 @@ afterTriggerDeleteHeadEventChunk(AfterTriggersQueryData *qs)
> * fmgr lookup cache space at the caller level. (For triggers fired at
> * the end of a query, we can even piggyback on the executor's state.)
> *
> - * event: event currently being fired.
> + * event: event currently being fired. Pass NULL if the current batch of RI
> + * trigger events should be processed.
> * rel: open relation for event.
> * trigdesc: working copy of rel's trigger info.
> * finfo: array of fmgr lookup cache entries (one per trigger in trigdesc).
> * instr: array of EXPLAIN ANALYZE instrumentation nodes (one per trigger),
> * or NULL if no instrumentation is wanted.
> + * trig_last: trigger info used for the last trigger execution.
> * per_tuple_context: memory context to call trigger function in.
> + * batch_context: memory context to store tuples for RI triggers.
> * trig_tuple_slot1: scratch slot for tg_trigtuple (foreign tables only)
> * trig_tuple_slot2: scratch slot for tg_newtuple (foreign tables only)
> * ----------
> @@ -3824,39 +3841,55 @@ AfterTriggerExecute(EState *estate,
> ResultRelInfo *relInfo,
> TriggerDesc *trigdesc,
> FmgrInfo *finfo, Instrumentation *instr,
> + TriggerData *trig_last,
> MemoryContext per_tuple_context,
> + MemoryContext batch_context,
> TupleTableSlot *trig_tuple_slot1,
> TupleTableSlot *trig_tuple_slot2)
> {
> Relation rel = relInfo->ri_RelationDesc;
> AfterTriggerShared evtshared = GetTriggerSharedData(event);
> Oid tgoid = evtshared->ats_tgoid;
> - TriggerData LocTriggerData = {0};
> HeapTuple rettuple;
> - int tgindx;
> bool should_free_trig = false;
> bool should_free_new = false;
> + bool is_new = false;
>
> - /*
> - * Locate trigger in trigdesc.
> - */
> - for (tgindx = 0; tgindx < trigdesc->numtriggers; tgindx++)
> + if (trig_last->tg_trigger == NULL)
> {
> - if (trigdesc->triggers[tgindx].tgoid == tgoid)
> + int tgindx;
> +
> + /*
> + * Locate trigger in trigdesc.
> + */
> + for (tgindx = 0; tgindx < trigdesc->numtriggers; tgindx++)
> {
> - LocTriggerData.tg_trigger = &(trigdesc->triggers[tgindx]);
> - break;
> + if (trigdesc->triggers[tgindx].tgoid == tgoid)
> + {
> + trig_last->tg_trigger = &(trigdesc->triggers[tgindx]);
> + trig_last->tgindx = tgindx;
> + break;
> + }
> }
> + if (trig_last->tg_trigger == NULL)
> + elog(ERROR, "could not find trigger %u", tgoid);
> +
> + if (RI_FKey_trigger_type(trig_last->tg_trigger->tgfoid) !=
> + RI_TRIGGER_NONE)
> + trig_last->is_ri_trigger = true;
> +
> + is_new = true;
> }
> - if (LocTriggerData.tg_trigger == NULL)
> - elog(ERROR, "could not find trigger %u", tgoid);
> +
> + /* trig_last for non-RI trigger should always be initialized again. */
> + Assert(trig_last->is_ri_trigger || is_new);
>
> /*
> * If doing EXPLAIN ANALYZE, start charging time to this trigger. We want
> * to include time spent re-fetching tuples in the trigger cost.
> */
> - if (instr)
> - InstrStartNode(instr + tgindx);
> + if (instr && !trig_last->is_ri_trigger)
> + InstrStartNode(instr + trig_last->tgindx);

I'm pretty unhappy about the amount of new infrastructure this adds to
trigger.c. We're now going to have a third copy of the tuples (for a
time). trigger.c is already a pretty complicated / archaic piece of
infrastructure, and this patchset seems to make that even worse. We'll
grow yet another separate representation of tuples, there's a lot new
branches (less concerned about the runtime costs, more about the code
complexity) etc.

> +/* ----------
> + * Construct the query to check inserted/updated rows of the FK table.
> + *
> + * If "insert" is true, the rows are inserted, otherwise they are updated.
> + *
> + * The query string built is
> + * SELECT t.fkatt1 [, ...]
> + * FROM <tgtable> t LEFT JOIN LATERAL
> + * (SELECT t.fkatt1 [, ...]
> + * FROM [ONLY] <pktable> p
> + * WHERE t.fkatt1 = p.pkatt1 [AND ...]
> + * FOR KEY SHARE OF p) AS m
> + * ON t.fkatt1 = m.fkatt1 [AND ...]
> + * WHERE m.fkatt1 ISNULL
> + * LIMIT 1
> + *

Why do we need the lateral query here?

Greetings,

Andres Freund