On Wed, Jan 19, 2022 at 4:13 PM Amit Langote <amitlangot...@gmail.com> wrote:
> On Wed, Jan 19, 2022 at 7:29 AM Alvaro Herrera <alvhe...@alvh.no-ip.org>
> 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
