Hello. Since the last discussion about it (http://www.postgresql.org/message-id/cadyhksugp6ojb1pybtimop3s5fg_yokguto-7rbcltnvaj5...@mail.gmail.com), I finally managed to implement row triggers for foreign tables.
The attached patch does not contain regression tests or documentation yet, a revised patch will include those sometime during the week. I'll add it to the commitfest then. A simple test scenario using postgres_fdw is however included as an attachment. The attached patch implements triggers for foreign tables according to the design discussed in the link above. For statement-level triggers, nothing has changed since the last patch. Statement-level triggers are just allowed in the command checking. For row-level triggers, it works as follows: - during rewrite phase, every attribute of the foreign table is duplicated as a resjunk entry if a trigger is defined on the relation. These entries are then used to store the old values for a tuple. There is room for improvement here: we could check if any trigger is in fact a row-level trigger - during execution phase, the before triggers are fired exactly like triggers on regular tables, except that old tuples are not fetched using their ctid, but rebuilt using the previously-stored resjunk attributes. - for after triggers, the whole queuing mechanism is bypassed for foreign tables. This is IMO acceptable, since foreign tables cannot have constraints or constraints triggers, and thus have not need for deferrable execution. This design avoids the need for storing and retrieving/identifying remote tuples until the query or transaction end. - the duplicated resjunk attributes are identified by being: - marked as resjunk in the targetlist - not being system or whole-row attributes (varno > 0) There is still one small issue with the attached patch: modifications to the tuple performed by the foreign data wrapper (via the returned TupleTableSlot in ExecForeignUpdate and ExecForeignInsert hooks) are not visible to the AFTER trigger. This could be fixed by merging the planslot containing the resjunk columns with the returned slot before calling the trigger, but I'm not really sure how to safely perform that. Any advice ? Many thanks to Kohei Kaigai for taking the time to help with the design. -- Ronan Dunklau http://dalibo.com - http://dalibo.org
test_with_postgres_fdw.sql
Description: application/sql
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index b9cd88d..a509595 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -3150,13 +3150,16 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, /* No command-specific prep needed */ break; case AT_EnableTrig: /* ENABLE TRIGGER variants */ - case AT_EnableAlwaysTrig: - case AT_EnableReplicaTrig: case AT_EnableTrigAll: case AT_EnableTrigUser: case AT_DisableTrig: /* DISABLE TRIGGER variants */ case AT_DisableTrigAll: case AT_DisableTrigUser: + ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE); + pass = AT_PASS_MISC; + break; + case AT_EnableReplicaTrig: + case AT_EnableAlwaysTrig: case AT_EnableRule: /* ENABLE/DISABLE RULE variants */ case AT_EnableAlwaysRule: case AT_EnableReplicaRule: diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 4eff184..12870eb 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -75,6 +75,14 @@ static HeapTuple GetTupleForTrigger(EState *estate, ItemPointer tid, LockTupleMode lockmode, TupleTableSlot **newSlot); + +static HeapTuple ExtractOldTuple(TupleTableSlot * mixedtupleslot, + ResultRelInfo * relinfo); +static void ExecARForeignTrigger(EState * estate, + TriggerData LocTriggerData, + ResultRelInfo *relinfo, + int triggerEvent, int triggerType); + static bool TriggerEnabled(EState *estate, ResultRelInfo *relinfo, Trigger *trigger, TriggerEvent event, Bitmapset *modifiedCols, @@ -184,12 +192,22 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, RelationGetRelationName(rel)), errdetail("Views cannot have TRUNCATE triggers."))); } + else if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) + { + if(stmt->isconstraint) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("\"%s\" is a foreign table", + RelationGetRelationName(rel)), + errdetail("Foreign Tables cannot have constraint triggers."))); + } else + { ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("\"%s\" is not a table or view", RelationGetRelationName(rel)))); - + } if (!allowSystemTableMods && IsSystemRelation(rel)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), @@ -1062,10 +1080,11 @@ RemoveTriggerById(Oid trigOid) rel = heap_open(relid, AccessExclusiveLock); if (rel->rd_rel->relkind != RELKIND_RELATION && - rel->rd_rel->relkind != RELKIND_VIEW) + rel->rd_rel->relkind != RELKIND_VIEW && + rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is not a table or view", + errmsg("\"%s\" is not a table, view or foreign table", RelationGetRelationName(rel)))); if (!allowSystemTableMods && IsSystemRelation(rel)) @@ -1166,10 +1185,11 @@ RangeVarCallbackForRenameTrigger(const RangeVar *rv, Oid relid, Oid oldrelid, form = (Form_pg_class) GETSTRUCT(tuple); /* only tables and views can have triggers */ - if (form->relkind != RELKIND_RELATION && form->relkind != RELKIND_VIEW) + if (form->relkind != RELKIND_RELATION && form->relkind != RELKIND_VIEW && + form->relkind != RELKIND_FOREIGN_TABLE) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is not a table or view", rv->relname))); + errmsg("\"%s\" is not a table, view or foreign table", rv->relname))); /* you must own the table to rename one of its triggers */ if (!pg_class_ownercheck(relid, GetUserId())) @@ -1844,9 +1864,7 @@ ExecCallTriggerFunc(TriggerData *trigdata, */ InitFunctionCallInfoData(fcinfo, finfo, 0, InvalidOid, (Node *) trigdata, NULL); - pgstat_init_function_usage(&fcinfo, &fcusage); - MyTriggerDepth++; PG_TRY(); { @@ -1946,10 +1964,10 @@ ExecASInsertTriggers(EState *estate, ResultRelInfo *relinfo) TupleTableSlot * ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo, - TupleTableSlot *slot) + TupleTableSlot *tupleslot) { TriggerDesc *trigdesc = relinfo->ri_TrigDesc; - HeapTuple slottuple = ExecMaterializeSlot(slot); + HeapTuple slottuple = ExecMaterializeSlot(tupleslot); HeapTuple newtuple = slottuple; HeapTuple oldtuple; TriggerData LocTriggerData; @@ -2003,9 +2021,9 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo, if (newslot->tts_tupleDescriptor != tupdesc) ExecSetSlotDescriptor(newslot, tupdesc); ExecStoreTuple(newtuple, newslot, InvalidBuffer, false); - slot = newslot; + tupleslot = newslot; } - return slot; + return tupleslot; } void @@ -2015,8 +2033,26 @@ ExecARInsertTriggers(EState *estate, ResultRelInfo *relinfo, TriggerDesc *trigdesc = relinfo->ri_TrigDesc; if (trigdesc && trigdesc->trig_insert_after_row) + { + Relation rel = relinfo->ri_RelationDesc; + + if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE){ + TriggerData LocTriggerData; + LocTriggerData.tg_trigtuple = trigtuple; + LocTriggerData.tg_newtuplebuf = InvalidBuffer; + LocTriggerData.tg_trigtuplebuf = InvalidBuffer; + ExecARForeignTrigger(estate, + LocTriggerData, + relinfo, + TRIGGER_EVENT_INSERT, + TRIGGER_TYPE_INSERT); + } + else + { AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_INSERT, true, NULL, trigtuple, recheckIndexes, NULL); + } + } } TupleTableSlot * @@ -2146,7 +2182,8 @@ ExecASDeleteTriggers(EState *estate, ResultRelInfo *relinfo) bool ExecBRDeleteTriggers(EState *estate, EPQState *epqstate, ResultRelInfo *relinfo, - ItemPointer tupleid) + ItemPointer tupleid, + TupleTableSlot *tupleslot) { TriggerDesc *trigdesc = relinfo->ri_TrigDesc; bool result = true; @@ -2155,9 +2192,17 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate, HeapTuple newtuple; TupleTableSlot *newSlot; int i; - - trigtuple = GetTupleForTrigger(estate, epqstate, relinfo, tupleid, - LockTupleExclusive, &newSlot); + Relation rel = relinfo->ri_RelationDesc; + if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) + { + newSlot = NULL; + trigtuple = ExtractOldTuple(epqstate->origslot, relinfo); + } + else + { + trigtuple = GetTupleForTrigger(estate, epqstate, relinfo, tupleid, + LockTupleExclusive, &newSlot); + } if (trigtuple == NULL) return false; @@ -2204,22 +2249,43 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate, void ExecARDeleteTriggers(EState *estate, ResultRelInfo *relinfo, - ItemPointer tupleid) + ItemPointer tupleid, + TupleTableSlot *tupleslot) { TriggerDesc *trigdesc = relinfo->ri_TrigDesc; - if (trigdesc && trigdesc->trig_delete_after_row) { - HeapTuple trigtuple = GetTupleForTrigger(estate, + Relation rel = relinfo->ri_RelationDesc; + HeapTuple trigtuple; + /* + * For FOREIGN Table, after triggers are fired immediately, since there + * cannot be any constraint triggers. + */ + if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE){ + TriggerData LocTriggerData; + trigtuple = ExtractOldTuple(tupleslot, relinfo); + LocTriggerData.tg_trigtuple = trigtuple; + LocTriggerData.tg_newtuple = NULL; + LocTriggerData.tg_trigtuplebuf = InvalidBuffer; + LocTriggerData.tg_newtuplebuf = InvalidBuffer; + ExecARForeignTrigger(estate, + LocTriggerData, + relinfo, + TRIGGER_EVENT_DELETE, + TRIGGER_TYPE_DELETE); + } + else + { + trigtuple = GetTupleForTrigger(estate, NULL, relinfo, tupleid, LockTupleExclusive, NULL); - - AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_DELETE, - true, trigtuple, NULL, NIL, NULL); - heap_freetuple(trigtuple); + AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_DELETE, + true, trigtuple, NULL, NIL, NULL); + heap_freetuple(trigtuple); + } } } @@ -2335,16 +2401,18 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo) TupleTableSlot * ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, ResultRelInfo *relinfo, - ItemPointer tupleid, TupleTableSlot *slot) + ItemPointer tupleid, + TupleTableSlot *tupleslot) { TriggerDesc *trigdesc = relinfo->ri_TrigDesc; - HeapTuple slottuple = ExecMaterializeSlot(slot); + HeapTuple slottuple = ExecMaterializeSlot(tupleslot); HeapTuple newtuple = slottuple; TriggerData LocTriggerData; HeapTuple trigtuple; HeapTuple oldtuple; TupleTableSlot *newSlot; int i; + Relation relation = relinfo->ri_RelationDesc; Bitmapset *modifiedCols; Bitmapset *keyCols; LockTupleMode lockmode; @@ -2355,7 +2423,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, * concurrency. */ modifiedCols = GetModifiedColumns(relinfo, estate); - keyCols = RelationGetIndexAttrBitmap(relinfo->ri_RelationDesc, + keyCols = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_KEY); if (bms_overlap(keyCols, modifiedCols)) lockmode = LockTupleExclusive; @@ -2363,8 +2431,16 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, lockmode = LockTupleNoKeyExclusive; /* get a copy of the on-disk tuple we are planning to update */ - trigtuple = GetTupleForTrigger(estate, epqstate, relinfo, tupleid, - lockmode, &newSlot); + if (relation->rd_rel->relkind == RELKIND_FOREIGN_TABLE) + { + newSlot = NULL; + trigtuple = ExtractOldTuple(epqstate->origslot, relinfo); + } + else + { + trigtuple = GetTupleForTrigger(estate, epqstate, relinfo, tupleid, + lockmode, &newSlot); + } if (trigtuple == NULL) return NULL; /* cancel the update action */ @@ -2381,8 +2457,8 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, */ if (newSlot != NULL) { - slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot); - slottuple = ExecMaterializeSlot(slot); + tupleslot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot); + slottuple = ExecMaterializeSlot(tupleslot); newtuple = slottuple; } @@ -2439,31 +2515,54 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, if (newslot->tts_tupleDescriptor != tupdesc) ExecSetSlotDescriptor(newslot, tupdesc); ExecStoreTuple(newtuple, newslot, InvalidBuffer, false); - slot = newslot; + tupleslot = newslot; } - return slot; + return tupleslot; } void ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo, - ItemPointer tupleid, HeapTuple newtuple, + ItemPointer tupleid, + HeapTuple newtuple, + TupleTableSlot *tupleslot, List *recheckIndexes) { TriggerDesc *trigdesc = relinfo->ri_TrigDesc; if (trigdesc && trigdesc->trig_update_after_row) { - HeapTuple trigtuple = GetTupleForTrigger(estate, - NULL, - relinfo, - tupleid, - LockTupleExclusive, - NULL); + Relation rel = relinfo->ri_RelationDesc; + HeapTuple trigtuple; + /* + * For FOREIGN Table, after triggers are fired immediately, since there + * cannot be any constraint triggers. + */ + if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE){ + TriggerData LocTriggerData; + trigtuple = ExtractOldTuple(tupleslot, relinfo); + LocTriggerData.tg_newtuple = newtuple; + LocTriggerData.tg_trigtuple = trigtuple; + LocTriggerData.tg_newtuplebuf = InvalidBuffer; + ExecARForeignTrigger(estate, + LocTriggerData, + relinfo, + TRIGGER_EVENT_UPDATE, + TRIGGER_TYPE_UPDATE); + } + else + { + trigtuple = GetTupleForTrigger(estate, + NULL, + relinfo, + tupleid, + LockTupleExclusive, + NULL); + AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE, + true, trigtuple, newtuple, recheckIndexes, + GetModifiedColumns(relinfo, estate)); + heap_freetuple(trigtuple); - AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE, - true, trigtuple, newtuple, recheckIndexes, - GetModifiedColumns(relinfo, estate)); - heap_freetuple(trigtuple); + } } } @@ -2604,7 +2703,11 @@ GetTupleForTrigger(EState *estate, HeapTupleData tuple; HeapTuple result; Buffer buffer; - + /* + * Foreign tables tuples are NOT stored on the heap. + * Instead, old attributes values are stored as resjunk attributes in the + * original tuple. So, we build a new tuple from those attributes here. + */ if (newSlot != NULL) { HTSU_Result test; @@ -2731,6 +2834,90 @@ ltrmark:; } /* + * Get an old tuple from a "mixed tuple", containing both the new values as well + * as well as the old ones as resjunk columns. + */ +static HeapTuple +ExtractOldTuple(TupleTableSlot *planSlot, + ResultRelInfo *relinfo) +{ + Relation relation = relinfo->ri_RelationDesc; + TupleDesc base_tuple_desc = relation->rd_att; + Datum * datum = palloc0(sizeof(Datum) * base_tuple_desc->natts); + bool * isnull = palloc0(sizeof(bool) * base_tuple_desc->natts); + HeapTuple tuple; + /* + * NewSlot is not null: this indicates a BEFORE trigger. + */ + JunkFilter * junk_filter = relinfo->ri_junkFilter; + List * target_list = junk_filter->jf_targetList; + ListCell * lc; + foreach(lc, target_list) + { + TargetEntry *tle = (TargetEntry *) lfirst(lc); + /* + * Only consider previously added resjunk entries + */ + if(tle->resjunk) + { + if(IsA(tle->expr, Var)) + { + Var * junkvar = (Var * )tle->expr; + /* + * Do not copy system identifiers. + */ + if(junkvar->varoattno > 0) + { + datum[junkvar->varoattno - 1] = slot_getattr(planSlot, + tle->resno, + &(isnull[junkvar->varoattno - 1])); + } + } + } + } + tuple = heap_form_tuple(base_tuple_desc, datum, isnull); + return tuple; +} + +/* + * Fire a trigger for a trigger on a foreign table + */ +static void +ExecARForeignTrigger(EState * estate, + TriggerData LocTriggerData, + ResultRelInfo *relinfo, int triggerEvent, + int triggerType) +{ + int i = 0; + Relation rel = relinfo->ri_RelationDesc; + TriggerDesc *trigdesc = relinfo->ri_TrigDesc; + LocTriggerData.type = T_TriggerData; + LocTriggerData.tg_relation = rel; + LocTriggerData.tg_event = triggerEvent | + TRIGGER_EVENT_ROW | + TRIGGER_EVENT_AFTER; + for (i = 0; i < trigdesc->numtriggers; i++) + { + Trigger *trigger = &trigdesc->triggers[i]; + + if (!TRIGGER_TYPE_MATCHES(trigger->tgtype, + TRIGGER_TYPE_ROW, + TRIGGER_TYPE_AFTER, + triggerType)) + continue; + if (!TriggerEnabled(estate, relinfo, trigger, LocTriggerData.tg_event, + NULL, LocTriggerData.tg_trigtuple, NULL)) + continue; + LocTriggerData.tg_trigger = trigger; + ExecCallTriggerFunc(&LocTriggerData, + i, + relinfo->ri_TrigFunctions, + relinfo->ri_TrigInstrument, + GetPerTupleMemoryContext(estate)); + } +} + +/* * Is trigger enabled to fire? */ static bool diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 189df71..c5a25d2 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -342,7 +342,8 @@ ExecDelete(ItemPointer tupleid, bool dodelete; dodelete = ExecBRDeleteTriggers(estate, epqstate, resultRelInfo, - tupleid); + tupleid, + epqstate->origslot); if (!dodelete) /* "do nothing" */ return NULL; @@ -488,7 +489,7 @@ ldelete:; (estate->es_processed)++; /* AFTER ROW DELETE Triggers */ - ExecARDeleteTriggers(estate, resultRelInfo, tupleid); + ExecARDeleteTriggers(estate, resultRelInfo, tupleid, planSlot); /* Process RETURNING if present */ if (resultRelInfo->ri_projectReturning) @@ -609,7 +610,8 @@ ExecUpdate(ItemPointer tupleid, resultRelInfo->ri_TrigDesc->trig_update_before_row) { slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo, - tupleid, slot); + tupleid, + slot); if (slot == NULL) /* "do nothing" */ return NULL; @@ -788,7 +790,7 @@ lreplace:; (estate->es_processed)++; /* AFTER ROW UPDATE Triggers */ - ExecARUpdateTriggers(estate, resultRelInfo, tupleid, tuple, + ExecARUpdateTriggers(estate, resultRelInfo, tupleid, tuple, planSlot, recheckIndexes); list_free(recheckIndexes); diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 50cb753..deb1fae 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -1199,13 +1199,43 @@ rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte, * Let the foreign table's FDW add whatever junk TLEs it wants. */ FdwRoutine *fdwroutine; + TupleDesc desc = target_relation->rd_att; + int i; fdwroutine = GetFdwRoutineForRelation(target_relation, false); if (fdwroutine->AddForeignUpdateTargets != NULL) fdwroutine->AddForeignUpdateTargets(parsetree, target_rte, target_relation); - + /* + * If there is any row-level trigger on the table, add junk attributes + * for the old ones. + */ + if (target_relation->trigdesc != NULL) + { + for (i = 0; i < desc->natts; i++) + { + Form_pg_attribute att = desc->attrs[i]; + if(!att->attisdropped) + { + TargetEntry *tle; + Var * var = makeVar(parsetree->resultRelation, + att->attnum, + att->atttypid, + att->atttypmod, + att->attcollation, + 0); + /* + * Identify those newly created vars with a varoattno at -1 + */ + tle = makeTargetEntry((Expr *) var, + list_length(parsetree->targetList) + 1, + strdup(NameStr(att->attname)), + true); + parsetree->targetList = lappend(parsetree->targetList, tle); + } + } + } return; } else diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index 411a66d..079bbac 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -147,10 +147,12 @@ extern void ExecASDeleteTriggers(EState *estate, extern bool ExecBRDeleteTriggers(EState *estate, EPQState *epqstate, ResultRelInfo *relinfo, - ItemPointer tupleid); + ItemPointer tupleid, + TupleTableSlot *tupleslot); extern void ExecARDeleteTriggers(EState *estate, ResultRelInfo *relinfo, - ItemPointer tupleid); + ItemPointer tupleid, + TupleTableSlot *tupleslot); extern bool ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo, HeapTuple trigtuple); @@ -162,11 +164,12 @@ extern TupleTableSlot *ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, ResultRelInfo *relinfo, ItemPointer tupleid, - TupleTableSlot *slot); + TupleTableSlot *tupleslot); extern void ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo, ItemPointer tupleid, HeapTuple newtuple, + TupleTableSlot *tupleslot, List *recheckIndexes); extern TupleTableSlot *ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
signature.asc
Description: This is a digitally signed message part.