On Wed, Jan 19, 2022 at 4:13 PM Amit Langote <[email protected]> wrote: > On Wed, Jan 19, 2022 at 7:29 AM Alvaro Herrera <[email protected]> > wrote: > > On 2022-Jan-18, Alvaro Herrera wrote: > > > On 2022-Jan-18, Amit Langote wrote: > > > > > > > Would you like me to update the patch with the above renumbering or > > > > are you working on a new version yourself? > > > > > > I have a few very minor changes apart from that. Let me see if I can > > > get this pushed soon; if I'm unable to (there are parts I don't fully > > > grok yet), I'll post what I have. > > > > Here's v13, a series with your patch as 0001 and a few changes from me; > > the bulk is just pgindent, and there are a few stylistic changes and an > > unrelated typo fix and I added a couple of comments to your new code. > > Thank you. > > > I don't like the API change to ExecInsert(). You're adding two output > > arguments: > > - the tuple being inserted. But surely you have this already, because > > it's in the 'slot' argument you passed in. ExecInsert is even careful to > > set the ->tts_tableOid argument there. So ExecInsert's caller doesn't > > need to receive the inserted tuple as an argument, it can just read > > 'slot'. > > Hmm, ExecInsert()'s input slot belongs to either the source partition > or the "root" target relation, the latter due to the following stanza > in ExecCrossPartitionUpdate(): > > /* > * resultRelInfo is one of the per-relation resultRelInfos. So we should > * convert the tuple into root's tuple descriptor if needed, since > * ExecInsert() starts the search from root. > */ > tupconv_map = ExecGetChildToRootMap(resultRelInfo); > if (tupconv_map != NULL) > slot = execute_attr_map_slot(tupconv_map->attrMap, > slot, > mtstate->mt_root_tuple_slot); > > Though the slot whose tuple ExecInsert() ultimately inserts may be > destination partition's ri_PartitionTupleSlot due to the following > stanza in it: > > /* > * If the input result relation is a partitioned table, find the leaf > * partition to insert the tuple into. > */ > if (proute) > { > ResultRelInfo *partRelInfo; > > slot = ExecPrepareTupleRouting(mtstate, estate, proute, > resultRelInfo, slot, > &partRelInfo); > resultRelInfo = partRelInfo; > } > > It's not great that ExecInsert()'s existing header comment doesn't > mention that the slot whose tuple is actually inserted may not be the > slot it receives from the caller :-(. > > > - the relation to which the tuple was inserted. Can't this be obtained > > by looking at slot->tts_tableOid? We should be able to use > > ExecLookupResultRelByOid() to obtain it, no? (I suppose you may claim > > that this is wasteful, but I think this is not a common case anyway and > > it's worth keeping ExecInsert()'s API clean for the sake of the 99.99% > > of its other calls). > > ExecLookupResultRelByOid() is only useful when *all* relevant leaf > partitions are present in the ModifyTableState.resultRelInfo array > (due to being present in ModifyTable.resultRelations). Leaf > partitions that are only initialized by tuple routing (such as > destination partitions of cross-partition updates) are only present in > ModifyTableState.mt_partition_tuple_routing.partitions[] that are not > discoverable by ExecLookupResultRelByOid(). > > > I think the argument definition of ExecCrossPartitionUpdateForeignKey is > > a bit messy. I propose to move mtstate,estate as two first arguments; > > IIRC the whole executor does it that way. > > Okay, done. > > > AfterTriggerSaveEvent determines maybe_crosspart_update (by looking at > > mtstate->operation -- why doesn't it look at 'event' instead?) and later > > it determines new_event.ate_flags. Why can't it use > > maybe_crosspart_update to simplify part of that? Or maybe the other way > > around, not sure. It looks like something could be made simpler there. > > Actually, I remember disliking maybe_crosspart_update for sounding a > bit fuzzy and also having to add mtstate to a bunch of trigger.c > interface functions only to calculate that. > > I now wonder if passing an is_crosspart_update through > ExecAR{Update|Delete}Triggers() would not be better. Both > ExecDelete() and ExecCrossPartitionUpdateForeignKey() know for sure if > their ExecAR{Update|Delete}Triggers() invocation is for a > cross-partition update, so better to just pass that information down > to AfterTriggerSaveEvent() than pass 'mtstate' and have the latter > reverse-engineer only a fuzzy guess of whether that's the case. > > I like that interface better and have implemented it in the updated version. > > I've also merged your changes and made some of my own as mentioned > above to end up with the attached v14. I'm also attaching a delta > patch over v13 (0001+0002) to easily see the changes I made to get > v14.
Oops, broke the cfbot's patch-applier again. Delta-diff reattached as txt. -- Amit Langote EDB: http://www.enterprisedb.com
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index e6aa36a9d8..c2a643e5a8 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -94,15 +94,14 @@ static HeapTuple ExecCallTriggerFunc(TriggerData *trigdata,
FmgrInfo *finfo,
Instrumentation *instr,
MemoryContext per_tuple_context);
-static void AfterTriggerSaveEvent(EState *estate,
-
ModifyTableState *mtstate,
- ResultRelInfo
*relinfo,
+static void AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
ResultRelInfo
*src_partinfo,
ResultRelInfo
*dst_partinfo,
int event,
bool row_trigger,
TupleTableSlot *oldtup, TupleTableSlot *newtup,
List
*recheckIndexes, Bitmapset *modifiedCols,
-
TransitionCaptureState *transition_capture);
+
TransitionCaptureState *transition_capture,
+ bool
is_crosspart_update);
static void AfterTriggerEnlargeQueryState(void);
static bool before_stmt_triggers_fired(Oid relid, CmdType cmdType);
@@ -2462,10 +2461,10 @@ ExecASInsertTriggers(EState *estate, ResultRelInfo
*relinfo,
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
if (trigdesc && trigdesc->trig_insert_after_statement)
- AfterTriggerSaveEvent(estate, NULL, relinfo,
- NULL, NULL,
+ AfterTriggerSaveEvent(estate, relinfo, NULL, NULL,
TRIGGER_EVENT_INSERT,
- false, NULL, NULL,
NIL, NULL, transition_capture);
+ false, NULL, NULL,
NIL, NULL, transition_capture,
+ false);
}
bool
@@ -2553,12 +2552,12 @@ ExecARInsertTriggers(EState *estate, ResultRelInfo
*relinfo,
if ((trigdesc && trigdesc->trig_insert_after_row) ||
(transition_capture &&
transition_capture->tcs_insert_new_table))
- AfterTriggerSaveEvent(estate, NULL, relinfo,
- NULL, NULL,
+ AfterTriggerSaveEvent(estate, relinfo, NULL, NULL,
TRIGGER_EVENT_INSERT,
true, NULL, slot,
recheckIndexes, NULL,
- transition_capture);
+ transition_capture,
+ false);
}
bool
@@ -2680,10 +2679,10 @@ ExecASDeleteTriggers(EState *estate, ResultRelInfo
*relinfo,
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
if (trigdesc && trigdesc->trig_delete_after_statement)
- AfterTriggerSaveEvent(estate, NULL, relinfo,
- NULL, NULL,
+ AfterTriggerSaveEvent(estate, relinfo, NULL, NULL,
TRIGGER_EVENT_DELETE,
- false, NULL, NULL,
NIL, NULL, transition_capture);
+ false, NULL, NULL,
NIL, NULL, transition_capture,
+ false);
}
/*
@@ -2778,12 +2777,17 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
return result;
}
+/*
+ * Note: is_crosspart_update must be true if the DELETE is being performed
+ * as part of a cross-partition update.
+ */
void
-ExecARDeleteTriggers(EState *estate, ModifyTableState *mtstate,
+ExecARDeleteTriggers(EState *estate,
ResultRelInfo *relinfo,
ItemPointer tupleid,
HeapTuple fdw_trigtuple,
- TransitionCaptureState
*transition_capture)
+ TransitionCaptureState
*transition_capture,
+ bool is_crosspart_update)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
@@ -2804,11 +2808,11 @@ ExecARDeleteTriggers(EState *estate, ModifyTableState
*mtstate,
else
ExecForceStoreHeapTuple(fdw_trigtuple, slot, false);
- AfterTriggerSaveEvent(estate, mtstate, relinfo,
- NULL, NULL,
+ AfterTriggerSaveEvent(estate, relinfo, NULL, NULL,
TRIGGER_EVENT_DELETE,
true, slot, NULL,
NIL, NULL,
- transition_capture);
+ transition_capture,
+ is_crosspart_update);
}
}
@@ -2927,12 +2931,12 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo
*relinfo,
Assert(relinfo->ri_RootResultRelInfo == NULL);
if (trigdesc && trigdesc->trig_update_after_statement)
- AfterTriggerSaveEvent(estate, NULL, relinfo,
- NULL, NULL,
+ AfterTriggerSaveEvent(estate, relinfo, NULL, NULL,
TRIGGER_EVENT_UPDATE,
false, NULL, NULL,
NIL,
ExecGetAllUpdatedCols(relinfo, estate),
- transition_capture);
+ transition_capture,
+ false);
}
bool
@@ -3068,24 +3072,25 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
}
/*
- * 'src_partinfo' and 'dst_partinfo', when non-NULL, refer to the source and
- * destination partitions, respectively, of a cross-partition update of the
- * root partitioned table mentioned in the query, given by 'relinfo'.
+ * Note: 'src_partinfo' and 'dst_partinfo', when non-NULL, refer to the source
+ * and destination partitions, respectively, of a cross-partition update of
+ * the root partitioned table mentioned in the query, given by 'relinfo'.
* 'tupleid' in that case refers to the ctid of the "old" tuple in the source
* partition, and 'newslot' contains the "new" tuple in the destination
* partition. This interface allows to support the requirements of
- * ExecCrossPartitionUpdateForeignKey().
+ * ExecCrossPartitionUpdateForeignKey(); is_crosspart_update must be true in
+ * that case.
*/
void
-ExecARUpdateTriggers(EState *estate, ModifyTableState *mtstate,
- ResultRelInfo *relinfo,
+ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
ResultRelInfo *src_partinfo,
ResultRelInfo *dst_partinfo,
ItemPointer tupleid,
HeapTuple fdw_trigtuple,
TupleTableSlot *newslot,
List *recheckIndexes,
- TransitionCaptureState
*transition_capture)
+ TransitionCaptureState
*transition_capture,
+ bool is_crosspart_update)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
@@ -3103,6 +3108,9 @@ ExecARUpdateTriggers(EState *estate, ModifyTableState
*mtstate,
TupleTableSlot *oldslot;
ResultRelInfo *tupsrc;
+ Assert((src_partinfo != NULL && dst_partinfo != NULL) ||
+ !is_crosspart_update);
+
tupsrc = src_partinfo ? src_partinfo : relinfo;
oldslot = ExecGetTriggerOldSlot(estate, tupsrc);
@@ -3119,12 +3127,14 @@ ExecARUpdateTriggers(EState *estate, ModifyTableState
*mtstate,
else
ExecClearTuple(oldslot);
- AfterTriggerSaveEvent(estate, mtstate, relinfo,
+ AfterTriggerSaveEvent(estate, relinfo,
src_partinfo,
dst_partinfo,
TRIGGER_EVENT_UPDATE,
- true, oldslot,
newslot, recheckIndexes,
+ true,
+ oldslot, newslot,
recheckIndexes,
ExecGetAllUpdatedCols(relinfo, estate),
- transition_capture);
+ transition_capture,
+ is_crosspart_update);
}
}
@@ -3247,10 +3257,11 @@ ExecASTruncateTriggers(EState *estate, ResultRelInfo
*relinfo)
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
if (trigdesc && trigdesc->trig_truncate_after_statement)
- AfterTriggerSaveEvent(estate, NULL, relinfo,
+ AfterTriggerSaveEvent(estate, relinfo,
NULL, NULL,
TRIGGER_EVENT_TRUNCATE,
- false, NULL, NULL,
NIL, NULL, NULL);
+ false, NULL, NULL,
NIL, NULL, NULL,
+ false);
}
@@ -5829,24 +5840,26 @@ AfterTriggerPendingOnRel(Oid relid)
* UPDATE; in this case, this function is called with relinfo as the
* partitioned table, and src_partinfo and dst_partinfo referring to the
* source and target leaf partitions, respectively.
+ *
+ * is_crosspart_update is true either when a DELETE event is fired on the
+ * source partition (which is to be ignored) or an UPDATE event is fired on
+ * the root partitioned table.
* ----------
*/
static void
-AfterTriggerSaveEvent(EState *estate, ModifyTableState *mtstate,
- ResultRelInfo *relinfo,
+AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
ResultRelInfo *src_partinfo,
ResultRelInfo *dst_partinfo,
int event, bool row_trigger,
TupleTableSlot *oldslot,
TupleTableSlot *newslot,
List *recheckIndexes, Bitmapset
*modifiedCols,
- TransitionCaptureState
*transition_capture)
+ TransitionCaptureState
*transition_capture,
+ bool is_crosspart_update)
{
Relation rel = relinfo->ri_RelationDesc;
- Relation rootRel;
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
AfterTriggerEventData new_event;
AfterTriggerSharedData new_shared;
- bool maybe_crosspart_update;
char relkind = rel->rd_rel->relkind;
int tgtype_event;
int tgtype_level;
@@ -5957,16 +5970,9 @@ AfterTriggerSaveEvent(EState *estate, ModifyTableState
*mtstate,
* queue an update event on the root target partitioned table, also
* passing the source and destination partitions and their tuples.
*/
- rootRel = relinfo->ri_RootResultRelInfo ?
- relinfo->ri_RootResultRelInfo->ri_RelationDesc : NULL;
- maybe_crosspart_update =
- (row_trigger && mtstate &&
- mtstate->operation == CMD_UPDATE &&
- (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
- (rootRel && rootRel->rd_rel->relkind ==
RELKIND_PARTITIONED_TABLE)));
Assert(!row_trigger ||
rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE ||
- (maybe_crosspart_update &&
+ (is_crosspart_update &&
TRIGGER_FIRED_BY_UPDATE(event) &&
src_partinfo != NULL && dst_partinfo != NULL));
@@ -6161,7 +6167,7 @@ AfterTriggerSaveEvent(EState *estate, ModifyTableState
*mtstate,
* (partitioned) target table will be
used to perform the
* necessary foreign key enforcement
action.
*/
- if (maybe_crosspart_update &&
+ if (is_crosspart_update &&
TRIGGER_FIRED_BY_DELETE(event)
&&
trigger->tgisclone)
continue;
diff --git a/src/backend/executor/execReplication.c
b/src/backend/executor/execReplication.c
index e2a338ba33..f978e28ba9 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -516,10 +516,10 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
NULL, NIL);
/* AFTER ROW UPDATE Triggers */
- ExecARUpdateTriggers(estate, NULL, resultRelInfo,
+ ExecARUpdateTriggers(estate, resultRelInfo,
NULL, NULL,
tid, NULL, slot,
- recheckIndexes, NULL);
+ recheckIndexes, NULL,
false);
list_free(recheckIndexes);
}
@@ -557,8 +557,8 @@ ExecSimpleRelationDelete(ResultRelInfo *resultRelInfo,
simple_table_tuple_delete(rel, tid, estate->es_snapshot);
/* AFTER ROW DELETE Triggers */
- ExecARDeleteTriggers(estate, NULL, resultRelInfo,
- tid, NULL, NULL);
+ ExecARDeleteTriggers(estate, resultRelInfo,
+ tid, NULL, NULL,
false);
}
}
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index 6a16d0e673..204126a29f 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -962,13 +962,14 @@ ExecInsert(ModifyTableState *mtstate,
if (mtstate->operation == CMD_UPDATE && mtstate->mt_transition_capture
&& mtstate->mt_transition_capture->tcs_update_new_table)
{
- ExecARUpdateTriggers(estate, mtstate, resultRelInfo,
+ ExecARUpdateTriggers(estate, resultRelInfo,
NULL, NULL,
NULL,
NULL,
slot,
NULL,
-
mtstate->mt_transition_capture);
+
mtstate->mt_transition_capture,
+ false);
/*
* We've already captured the NEW TABLE row, so make sure any AR
@@ -1359,13 +1360,14 @@ ldelete:;
if (mtstate->operation == CMD_UPDATE && mtstate->mt_transition_capture
&& mtstate->mt_transition_capture->tcs_update_old_table)
{
- ExecARUpdateTriggers(estate, mtstate, resultRelInfo,
+ ExecARUpdateTriggers(estate, resultRelInfo,
NULL, NULL,
tupleid,
oldtuple,
NULL,
NULL,
-
mtstate->mt_transition_capture);
+
mtstate->mt_transition_capture,
+ false);
/*
* We've already captured the NEW TABLE row, so make sure any AR
@@ -1375,8 +1377,8 @@ ldelete:;
}
/* AFTER ROW DELETE Triggers */
- ExecARDeleteTriggers(estate, mtstate, resultRelInfo, tupleid, oldtuple,
- ar_delete_trig_tcs);
+ ExecARDeleteTriggers(estate, resultRelInfo, tupleid, oldtuple,
+ ar_delete_trig_tcs,
changingPart);
/* Process RETURNING if present and if requested */
if (processReturning && resultRelInfo->ri_projectReturning)
@@ -1642,13 +1644,13 @@ GetAncestorResultRels(ResultRelInfo *resultRelInfo)
* keys pointing into it.
*/
static void
-ExecCrossPartitionUpdateForeignKey(ResultRelInfo *sourcePartInfo,
+ExecCrossPartitionUpdateForeignKey(ModifyTableState *mtstate,
+ EState
*estate,
+
ResultRelInfo *sourcePartInfo,
ResultRelInfo *destPartInfo,
ItemPointer
tupleid,
TupleTableSlot *oldslot,
-
TupleTableSlot *newslot,
-
ModifyTableState *mtstate,
- EState
*estate)
+
TupleTableSlot *newslot)
{
ListCell *lc;
ResultRelInfo *rootRelInfo = sourcePartInfo->ri_RootResultRelInfo;
@@ -1699,10 +1701,9 @@ ExecCrossPartitionUpdateForeignKey(ResultRelInfo
*sourcePartInfo,
}
/* Perform the root table's triggers. */
- ExecARUpdateTriggers(estate, mtstate, rootRelInfo,
- sourcePartInfo, destPartInfo,
- tupleid, NULL,
- newslot, NIL, NULL);
+ ExecARUpdateTriggers(estate,
+ rootRelInfo, sourcePartInfo,
destPartInfo,
+ tupleid, NULL, newslot, NIL,
NULL, true);
}
/* ----------------------------------------------------------------
@@ -1920,11 +1921,11 @@ lreplace:;
if (insert_destrel &&
resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_update_after_row)
-
ExecCrossPartitionUpdateForeignKey(resultRelInfo,
+ ExecCrossPartitionUpdateForeignKey(mtstate,
estate,
+
resultRelInfo,
insert_destrel,
tupleid, oldslot,
-
inserted_tuple,
-
mtstate, estate);
+
inserted_tuple);
return returning_slot;
}
@@ -2105,14 +2106,15 @@ lreplace:;
(estate->es_processed)++;
/* AFTER ROW UPDATE Triggers */
- ExecARUpdateTriggers(estate, mtstate, resultRelInfo,
+ ExecARUpdateTriggers(estate, resultRelInfo,
NULL, NULL,
tupleid, oldtuple,
slot,
recheckIndexes,
mtstate->operation ==
CMD_INSERT ?
mtstate->mt_oc_transition_capture :
-
mtstate->mt_transition_capture);
+ mtstate->mt_transition_capture,
+ false);
list_free(recheckIndexes);
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 1ba3a54499..66bf6c16e3 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -211,11 +211,11 @@ extern bool ExecBRDeleteTriggers(EState *estate,
HeapTuple
fdw_trigtuple,
TupleTableSlot
**epqslot);
extern void ExecARDeleteTriggers(EState *estate,
-
ModifyTableState *mtstate,
ResultRelInfo
*relinfo,
ItemPointer
tupleid,
HeapTuple
fdw_trigtuple,
-
TransitionCaptureState *transition_capture);
+
TransitionCaptureState *transition_capture,
+ bool
is_crosspart_update);
extern bool ExecIRDeleteTriggers(EState *estate,
ResultRelInfo
*relinfo,
HeapTuple
trigtuple);
@@ -231,7 +231,6 @@ extern bool ExecBRUpdateTriggers(EState *estate,
HeapTuple
fdw_trigtuple,
TupleTableSlot
*slot);
extern void ExecARUpdateTriggers(EState *estate,
-
ModifyTableState *mtstate,
ResultRelInfo
*relinfo,
ResultRelInfo
*src_partinfo,
ResultRelInfo
*dst_partinfo,
@@ -239,7 +238,8 @@ extern void ExecARUpdateTriggers(EState *estate,
HeapTuple
fdw_trigtuple,
TupleTableSlot
*slot,
List
*recheckIndexes,
-
TransitionCaptureState *transition_capture);
+
TransitionCaptureState *transition_capture,
+ bool
is_crosspart_update);
extern bool ExecIRUpdateTriggers(EState *estate,
ResultRelInfo
*relinfo,
HeapTuple
trigtuple,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 6c7eef1e54..37ad2b7663 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -530,7 +530,10 @@ typedef struct ResultRelInfo
/* for use by copyfrom.c when performing multi-inserts */
struct CopyMultiInsertBuffer *ri_CopyMultiInsertBuffer;
- /* Used during cross-partition updates on partitioned tables. */
+ /*
+ * Used when a leaf partition is involved in a cross-partition update of
+ * one of its ancestors; see ExecCrossPartitionUpdateForeignKey().
+ */
List *ri_ancestorResultRels;
} ResultRelInfo;
v14-0001-Enforce-foreign-key-correctly-during-cross-parti.patch
Description: Binary data
