From 95eee81cff36bcbbfed75205d93f16dc1c53e1c5 Mon Sep 17 00:00:00 2001 From: amit Date: Tue, 30 Jul 2019 10:51:35 +0900 Subject: [PATCH v3 4/4] Refactor transition tuple capture code a bit In the case of inherited update and partitioned table inserts, a child tuple needs to be converted back into the root table format. The tuple conversion map needed to do that was previously stored in ModifyTableState and adjusted every time the child relation changed, an arrangement which is a bit cumbersome to maintain. Instead save the map in the child result relation's ResultRelInfo. --- src/backend/commands/copy.c | 31 ++----- src/backend/commands/trigger.c | 21 +++-- src/backend/executor/execPartition.c | 23 +++-- src/backend/executor/nodeModifyTable.c | 156 +++++++++------------------------ src/include/commands/trigger.h | 10 +-- src/include/nodes/execnodes.h | 6 ++ 6 files changed, 84 insertions(+), 163 deletions(-) diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 2f682de785..5d02c67389 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -3114,32 +3114,15 @@ CopyFrom(CopyState cstate) } /* - * If we're capturing transition tuples, we might need to convert - * from the partition rowtype to root rowtype. + * If we're capturing transition tuples and there are no BEFORE + * triggers on the partition, we can just use the original + * unconverted tuple instead of converting the tuple in partition + * format back to root format. We must do the conversion if such + * triggers exist because they may change the tuple. */ if (cstate->transition_capture != NULL) - { - if (has_before_insert_row_trig) - { - /* - * If there are any BEFORE triggers on the partition, - * we'll have to be ready to convert their result back to - * tuplestore format. - */ - cstate->transition_capture->tcs_original_insert_tuple = NULL; - cstate->transition_capture->tcs_map = - resultRelInfo->ri_PartitionInfo->pi_PartitionToRootMap; - } - else - { - /* - * Otherwise, just remember the original unconverted - * tuple, to avoid a needless round trip conversion. - */ - cstate->transition_capture->tcs_original_insert_tuple = myslot; - cstate->transition_capture->tcs_map = NULL; - } - } + cstate->transition_capture->tcs_original_insert_tuple = + !has_before_insert_row_trig ? myslot : NULL; /* * We might need to convert from the root rowtype to the partition diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 2d9a8e9d54..43f796172c 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -35,6 +35,7 @@ #include "commands/defrem.h" #include "commands/trigger.h" #include "executor/executor.h" +#include "executor/execPartition.h" #include "miscadmin.h" #include "nodes/bitmapset.h" #include "nodes/makefuncs.h" @@ -4644,9 +4645,7 @@ GetAfterTriggersTableData(Oid relid, CmdType cmdType) * If there are no triggers in 'trigdesc' that request relevant transition * tables, then return NULL. * - * The resulting object can be passed to the ExecAR* functions. The caller - * should set tcs_map or tcs_original_insert_tuple as appropriate when dealing - * with child tables. + * The resulting object can be passed to the ExecAR* functions. * * Note that we copy the flags from a parent table into this struct (rather * than subsequently using the relation's TriggerDesc directly) so that we can @@ -5750,14 +5749,26 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo, */ if (row_trigger && transition_capture != NULL) { - TupleTableSlot *original_insert_tuple = transition_capture->tcs_original_insert_tuple; - TupleConversionMap *map = transition_capture->tcs_map; + TupleTableSlot *original_insert_tuple; + PartitionRoutingInfo *pinfo = relinfo->ri_PartitionInfo; + TupleConversionMap *map = pinfo ? + pinfo->pi_PartitionToRootMap : + relinfo->ri_ChildToRootMap; bool delete_old_table = transition_capture->tcs_delete_old_table; bool update_old_table = transition_capture->tcs_update_old_table; bool update_new_table = transition_capture->tcs_update_new_table; bool insert_new_table = transition_capture->tcs_insert_new_table; /* + * Get the originally inserted tuple from the global variable and set + * the latter to NULL because any given tuple must be read only once. + * Note that the TransitionCaptureState is shared across many calls + * to this function. + */ + original_insert_tuple = transition_capture->tcs_original_insert_tuple; + transition_capture->tcs_original_insert_tuple = NULL; + + /* * For INSERT events NEW should be non-NULL, for DELETE events OLD * should be non-NULL, whereas for UPDATE events normally both OLD and * NEW are non-NULL. But for UPDATE events fired for capturing diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 729dc396a9..62b93f39d4 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -167,7 +167,8 @@ static void ExecInitRoutingInfo(ModifyTableState *mtstate, PartitionTupleRouting *proute, PartitionDispatch dispatch, ResultRelInfo *partRelInfo, - int partidx); + int partidx, + bool is_update_result_rel); static PartitionDispatch ExecInitPartitionDispatchInfo(EState *estate, PartitionTupleRouting *proute, Oid partoid, PartitionDispatch parent_pd, int partidx); @@ -389,7 +390,7 @@ ExecFindPartition(ModifyTableState *mtstate, /* Set up the PartitionRoutingInfo for it */ ExecInitRoutingInfo(mtstate, estate, proute, dispatch, - rri, partidx); + rri, partidx, true); } } @@ -676,7 +677,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, /* Set up information needed for routing tuples to the partition. */ ExecInitRoutingInfo(mtstate, estate, proute, dispatch, - leaf_part_rri, partidx); + leaf_part_rri, partidx, false); /* * If there is an ON CONFLICT clause, initialize state for it. @@ -888,7 +889,8 @@ ExecInitRoutingInfo(ModifyTableState *mtstate, PartitionTupleRouting *proute, PartitionDispatch dispatch, ResultRelInfo *partRelInfo, - int partidx) + int partidx, + bool is_update_result_rel) { MemoryContext oldcxt; PartitionRoutingInfo *partrouteinfo; @@ -935,10 +937,15 @@ ExecInitRoutingInfo(ModifyTableState *mtstate, if (mtstate && (mtstate->mt_transition_capture || mtstate->mt_oc_transition_capture)) { - partrouteinfo->pi_PartitionToRootMap = - convert_tuples_by_name(RelationGetDescr(partRelInfo->ri_RelationDesc), - RelationGetDescr(partRelInfo->ri_PartitionRoot), - gettext_noop("could not convert row type")); + /* If partition is an update target, then we already got the map. */ + if (is_update_result_rel) + partrouteinfo->pi_PartitionToRootMap = + partRelInfo->ri_ChildToRootMap; + else + partrouteinfo->pi_PartitionToRootMap = + convert_tuples_by_name(RelationGetDescr(partRelInfo->ri_RelationDesc), + RelationGetDescr(partRelInfo->ri_PartitionRoot), + gettext_noop("could not convert row type")); } else partrouteinfo->pi_PartitionToRootMap = NULL; diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index fad8b928bb..4868e6f3a6 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -74,8 +74,6 @@ static TupleTableSlot *ExecPrepareTupleRouting(ModifyTableState *mtstate, ResultRelInfo **partRelInfo); static ResultRelInfo *getTargetResultRelInfo(ModifyTableState *node); static void ExecSetupChildParentMapForSubplan(ModifyTableState *mtstate); -static TupleConversionMap *tupconv_map_for_subplan(ModifyTableState *node, - int whichplan); /* * Verify that the tuples to be produced by INSERT or UPDATE match the @@ -339,10 +337,6 @@ ExecComputeStoredGenerated(ResultRelInfo *resultRelInfo, * relations. * * Returns RETURNING result if any, otherwise NULL. - * - * This may change the currently active tuple conversion map in - * mtstate->mt_transition_capture, so the callers must take care to - * save the previous value to avoid losing track of it. * ---------------------------------------------------------------- */ static TupleTableSlot * @@ -1053,9 +1047,7 @@ ExecCrossPartitionUpdate(ModifyTableState *mtstate, { EState *estate = mtstate->ps.state; PartitionTupleRouting *proute = mtstate->mt_partition_tuple_routing; - int map_index; - TupleConversionMap *tupconv_map; - TupleConversionMap *saved_tcs_map = NULL; + TupleConversionMap *tupconv_map = resultRelInfo->ri_ChildToRootMap; bool tuple_deleted; TupleTableSlot *epqslot = NULL; @@ -1131,41 +1123,16 @@ ExecCrossPartitionUpdate(ModifyTableState *mtstate, } } - /* - * resultRelInfo is one of the per-subplan resultRelInfos. So we - * should convert the tuple into root's tuple descriptor, since - * ExecInsert() starts the search from root. The tuple conversion - * map list is in the order of mtstate->resultRelInfo[], so to - * retrieve the one for this resultRel, we need to know the - * position of the resultRel in mtstate->resultRelInfo[]. - */ - map_index = resultRelInfo - mtstate->resultRelInfo; - Assert(map_index >= 0 && map_index < mtstate->mt_nplans); - tupconv_map = tupconv_map_for_subplan(mtstate, map_index); if (tupconv_map != NULL) slot = execute_attr_map_slot(tupconv_map->attrMap, slot, mtstate->mt_root_tuple_slot); - /* - * ExecInsert() may scribble on mtstate->mt_transition_capture, - * so save the currently active map. - */ - if (mtstate->mt_transition_capture) - saved_tcs_map = mtstate->mt_transition_capture->tcs_map; - /* Tuple routing starts from the root table. */ Assert(mtstate->rootResultRelInfo != NULL); *inserted_tuple = ExecInsert(mtstate, mtstate->rootResultRelInfo, slot, planSlot, estate, canSetTag); - /* Clear the INSERT's tuple and restore the saved map. */ - if (mtstate->mt_transition_capture) - { - mtstate->mt_transition_capture->tcs_original_insert_tuple = NULL; - mtstate->mt_transition_capture->tcs_map = saved_tcs_map; - } - /* We're done moving. */ return true; } @@ -1872,28 +1839,6 @@ ExecSetupTransitionCaptureState(ModifyTableState *mtstate, EState *estate) MakeTransitionCaptureState(targetRelInfo->ri_TrigDesc, RelationGetRelid(targetRelInfo->ri_RelationDesc), CMD_UPDATE); - - /* - * If we found that we need to collect transition tuples then we may also - * need tuple conversion maps for any children that have TupleDescs that - * aren't compatible with the tuplestores. (We can share these maps - * between the regular and ON CONFLICT cases.) - */ - if (mtstate->mt_transition_capture != NULL || - mtstate->mt_oc_transition_capture != NULL) - { - ExecSetupChildParentMapForSubplan(mtstate); - - /* - * Install the conversion map for the first plan for UPDATE and DELETE - * operations. It will be advanced each time we switch to the next - * plan. (INSERT operations set it every time, so we need not update - * mtstate->mt_oc_transition_capture here.) - */ - if (mtstate->mt_transition_capture && mtstate->operation != CMD_INSERT) - mtstate->mt_transition_capture->tcs_map = - tupconv_map_for_subplan(mtstate, 0); - } } /* @@ -1917,6 +1862,7 @@ ExecPrepareTupleRouting(ModifyTableState *mtstate, ResultRelInfo *partrel; PartitionRoutingInfo *partrouteinfo; TupleConversionMap *map; + bool has_before_insert_row_trig; /* * Look up the target partition's ResultRelInfo. If ExecFindPartition @@ -1931,37 +1877,17 @@ ExecPrepareTupleRouting(ModifyTableState *mtstate, Assert(partrouteinfo != NULL); /* - * If we're capturing transition tuples, we might need to convert from the - * partition rowtype to root partitioned table's rowtype. + * If we're capturing transition tuples and there are no BEFORE + * triggers on the partition, we can just use the original + * unconverted tuple instead of converting the tuple in partition + * format back to root format. We must do the conversion if such + * triggers exist because they may change the tuple. */ + has_before_insert_row_trig = (partrel->ri_TrigDesc && + partrel->ri_TrigDesc->trig_insert_before_row); if (mtstate->mt_transition_capture != NULL) - { - if (partrel->ri_TrigDesc && - partrel->ri_TrigDesc->trig_insert_before_row) - { - /* - * If there are any BEFORE triggers on the partition, we'll have - * to be ready to convert their result back to tuplestore format. - */ - mtstate->mt_transition_capture->tcs_original_insert_tuple = NULL; - mtstate->mt_transition_capture->tcs_map = - partrouteinfo->pi_PartitionToRootMap; - } - else - { - /* - * Otherwise, just remember the original unconverted tuple, to - * avoid a needless round trip conversion. - */ - mtstate->mt_transition_capture->tcs_original_insert_tuple = slot; - mtstate->mt_transition_capture->tcs_map = NULL; - } - } - if (mtstate->mt_oc_transition_capture != NULL) - { - mtstate->mt_oc_transition_capture->tcs_map = - partrouteinfo->pi_PartitionToRootMap; - } + mtstate->mt_transition_capture->tcs_original_insert_tuple = + !has_before_insert_row_trig ? slot : NULL; /* * Convert the tuple, if necessary. @@ -2016,20 +1942,6 @@ ExecSetupChildParentMapForSubplan(ModifyTableState *mtstate) } } -/* - * For a given subplan index, get the tuple conversion map. - */ -static TupleConversionMap * -tupconv_map_for_subplan(ModifyTableState *mtstate, int whichplan) -{ - /* If nobody else set the per-subplan array of maps, do so ourselves. */ - if (mtstate->mt_per_subplan_tupconv_maps == NULL) - ExecSetupChildParentMapForSubplan(mtstate); - - Assert(whichplan >= 0 && whichplan < mtstate->mt_nplans); - return mtstate->mt_per_subplan_tupconv_maps[whichplan]; -} - /* ---------------------------------------------------------------- * ExecModifyTable * @@ -2125,17 +2037,6 @@ ExecModifyTable(PlanState *pstate) junkfilter = resultRelInfo->ri_junkFilter; EvalPlanQualSetPlan(&node->mt_epqstate, subplanstate->plan, node->mt_arowmarks[node->mt_whichplan]); - /* Prepare to convert transition tuples from this child. */ - if (node->mt_transition_capture != NULL) - { - node->mt_transition_capture->tcs_map = - tupconv_map_for_subplan(node, node->mt_whichplan); - } - if (node->mt_oc_transition_capture != NULL) - { - node->mt_oc_transition_capture->tcs_map = - tupconv_map_for_subplan(node, node->mt_whichplan); - } continue; } else @@ -2304,6 +2205,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) int i; Relation rel; bool update_tuple_routing_needed = node->partColsUpdated; + ResultRelInfo *rootResultRel; /* check for unsupported flags */ Assert(!(eflags & (EXEC_FLAG_BACKWARD | EXEC_FLAG_MARK))); @@ -2326,8 +2228,13 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) /* If modifying a partitioned table, initialize the root table info */ if (node->rootResultRelIndex >= 0) + { mtstate->rootResultRelInfo = estate->es_root_result_relations + node->rootResultRelIndex; + rootResultRel = mtstate->rootResultRelInfo; + } + else + rootResultRel = mtstate->resultRelInfo; mtstate->mt_arowmarks = (List **) palloc0(sizeof(List *) * nplans); mtstate->mt_nplans = nplans; @@ -2337,6 +2244,13 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) mtstate->fireBSTriggers = true; /* + * Build state for collecting transition tuples. This requires having a + * valid trigger query context, so skip it in explain-only mode. + */ + if (!(eflags & EXEC_FLAG_EXPLAIN_ONLY)) + ExecSetupTransitionCaptureState(mtstate, estate); + + /* * call ExecInitNode on each of the plans to be executed and save the * results into the array "mt_plans". This is also a convenient place to * verify that the proposed target relations are valid and open their @@ -2411,6 +2325,21 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) eflags); } + /* + * If needed, initialize a map to convert tuples in the child format + * to the format of the table mentioned in the query (root relation). + * It's needed for update tuple routing, because the routing starts + * from the root relation. It's also needed for capturing transition + * tuples, because the transition tuple store can only store tuples + * in the root table format. + */ + if (update_tuple_routing_needed || + (mtstate->mt_transition_capture && + mtstate->operation != CMD_INSERT)) + resultRelInfo->ri_ChildToRootMap = + convert_tuples_by_name(RelationGetDescr(resultRelInfo->ri_RelationDesc), + RelationGetDescr(rootResultRel->ri_RelationDesc), + gettext_noop("could not convert row type")); resultRelInfo++; i++; } @@ -2435,13 +2364,6 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) ExecSetupPartitionTupleRouting(estate, mtstate, rel); /* - * Build state for collecting transition tuples. This requires having a - * valid trigger query context, so skip it in explain-only mode. - */ - if (!(eflags & EXEC_FLAG_EXPLAIN_ONLY)) - ExecSetupTransitionCaptureState(mtstate, estate); - - /* * Construct mapping from each of the per-subplan partition attnos to the * root attno. This is required when during update row movement the tuple * descriptor of a source partition does not match the root partitioned diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index a46feeedb0..bb080980c0 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -45,7 +45,7 @@ typedef struct TriggerData * The state for capturing old and new tuples into transition tables for a * single ModifyTable node (or other operation source, e.g. copy.c). * - * This is per-caller to avoid conflicts in setting tcs_map or + * This is per-caller to avoid conflicts in setting * tcs_original_insert_tuple. Note, however, that the pointed-to * private data may be shared across multiple callers. */ @@ -65,14 +65,6 @@ typedef struct TransitionCaptureState bool tcs_insert_new_table; /* - * For UPDATE and DELETE, AfterTriggerSaveEvent may need to convert the - * new and old tuples from a child table's format to the format of the - * relation named in a query so that it is compatible with the transition - * tuplestores. The caller must store the conversion map here if so. - */ - TupleConversionMap *tcs_map; - - /* * For INSERT and COPY, it would be wasteful to convert tuples from child * format to parent format after they have already been converted in the * opposite direction during routing. In that case we bypass conversion diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index b527cd93ed..0e70a62b26 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -485,6 +485,12 @@ typedef struct ResultRelInfo /* For use by copy.c when performing multi-inserts */ struct CopyMultiInsertBuffer *ri_CopyMultiInsertBuffer; + + /* + * Map to convert child sublan tuples to root parent format, set only if + * either update row movement or transition tuple capture is active. + */ + TupleConversionMap *ri_ChildToRootMap; } ResultRelInfo; /* ---------------- -- 2.11.0