From a67a4a8a5cc3609dd57a496be5614adee2000b5f Mon Sep 17 00:00:00 2001 From: amitlan Date: Tue, 31 Mar 2020 15:36:27 +0900 Subject: [PATCH v1] Build root tuple in ExecBuildSlotValueDescription Having the logic to do so be spread across various callers is starting to seem cumbersome to maintain. Use mt_root_tuple_slot to store the tuple to be shown in the description. For it to be available in all cases that require it, always set it in ExecInitModifyTable instead if only when UPDATE tuple routing is active as is the case currently. --- src/backend/executor/execMain.c | 204 ++++++++------------------------- src/backend/executor/execPartition.c | 6 + src/backend/executor/nodeModifyTable.c | 21 ++-- src/include/nodes/execnodes.h | 6 + src/test/regress/expected/inherit.out | 2 +- src/test/regress/expected/update.out | 6 +- 6 files changed, 80 insertions(+), 165 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 4fdffad..0b295c0 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -93,11 +93,10 @@ static bool ExecCheckRTEPermsModified(Oid relOid, Oid userid, Bitmapset *modifiedCols, AclMode requiredPerms); static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt); -static char *ExecBuildSlotValueDescription(Oid reloid, - TupleTableSlot *slot, - TupleDesc tupdesc, - Bitmapset *modifiedCols, - int maxfieldlen); +static char *ExecBuildSlotValueDescription(EState *estate, + ResultRelInfo *resultRelInfo, + TupleTableSlot *slot, + int maxfieldlen); static void EvalPlanQualStart(EPQState *epqstate, Plan *planTree); /* @@ -1829,51 +1828,10 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate) { - Oid root_relid; - TupleDesc tupdesc; char *val_desc; - Bitmapset *modifiedCols; - /* - * If the tuple has been routed, it's been converted to the partition's - * rowtype, which might differ from the root table's. We must convert it - * back to the root table's rowtype so that val_desc in the error message - * matches the input tuple. - */ - if (resultRelInfo->ri_PartitionRoot) - { - TupleDesc old_tupdesc; - AttrMap *map; - - root_relid = RelationGetRelid(resultRelInfo->ri_PartitionRoot); - tupdesc = RelationGetDescr(resultRelInfo->ri_PartitionRoot); - - old_tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc); - /* a reverse map */ - map = build_attrmap_by_name_if_req(old_tupdesc, tupdesc); - - /* - * Partition-specific slot's tupdesc can't be changed, so allocate a - * new one. - */ - if (map != NULL) - slot = execute_attr_map_slot(map, slot, - MakeTupleTableSlot(tupdesc, &TTSOpsVirtual)); - } - else - { - root_relid = RelationGetRelid(resultRelInfo->ri_RelationDesc); - tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc); - } - - modifiedCols = bms_union(GetInsertedColumns(resultRelInfo, estate), - GetUpdatedColumns(resultRelInfo, estate)); - - val_desc = ExecBuildSlotValueDescription(root_relid, - slot, - tupdesc, - modifiedCols, - 64); + val_desc = ExecBuildSlotValueDescription(estate, resultRelInfo, + slot, 64); ereport(ERROR, (errcode(ERRCODE_CHECK_VIOLATION), errmsg("new row for relation \"%s\" violates partition constraint", @@ -1900,9 +1858,6 @@ ExecConstraints(ResultRelInfo *resultRelInfo, Relation rel = resultRelInfo->ri_RelationDesc; TupleDesc tupdesc = RelationGetDescr(rel); TupleConstr *constr = tupdesc->constr; - Bitmapset *modifiedCols; - Bitmapset *insertedCols; - Bitmapset *updatedCols; Assert(constr || resultRelInfo->ri_PartitionCheck); @@ -1918,51 +1873,19 @@ ExecConstraints(ResultRelInfo *resultRelInfo, if (att->attnotnull && slot_attisnull(slot, attrChk)) { char *val_desc; - Relation orig_rel = rel; - TupleDesc orig_tupdesc = RelationGetDescr(rel); - - /* - * If the tuple has been routed, it's been converted to the - * partition's rowtype, which might differ from the root - * table's. We must convert it back to the root table's - * rowtype so that val_desc shown error message matches the - * input tuple. - */ - if (resultRelInfo->ri_PartitionRoot) - { - AttrMap *map; - - rel = resultRelInfo->ri_PartitionRoot; - tupdesc = RelationGetDescr(rel); - /* a reverse map */ - map = build_attrmap_by_name_if_req(orig_tupdesc, - tupdesc); - /* - * Partition-specific slot's tupdesc can't be changed, so - * allocate a new one. - */ - if (map != NULL) - slot = execute_attr_map_slot(map, slot, - MakeTupleTableSlot(tupdesc, &TTSOpsVirtual)); - } - - insertedCols = GetInsertedColumns(resultRelInfo, estate); - updatedCols = GetUpdatedColumns(resultRelInfo, estate); - modifiedCols = bms_union(insertedCols, updatedCols); - val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel), + val_desc = ExecBuildSlotValueDescription(estate, + resultRelInfo, slot, - tupdesc, - modifiedCols, 64); ereport(ERROR, (errcode(ERRCODE_NOT_NULL_VIOLATION), errmsg("null value in column \"%s\" of relation \"%s\" violates not-null constraint", NameStr(att->attname), - RelationGetRelationName(orig_rel)), + RelationGetRelationName(rel)), val_desc ? errdetail("Failing row contains %s.", val_desc) : 0, - errtablecol(orig_rel, attrChk))); + errtablecol(rel, attrChk))); } } } @@ -1974,43 +1897,17 @@ ExecConstraints(ResultRelInfo *resultRelInfo, if ((failed = ExecRelCheck(resultRelInfo, slot, estate)) != NULL) { char *val_desc; - Relation orig_rel = rel; - /* See the comment above. */ - if (resultRelInfo->ri_PartitionRoot) - { - TupleDesc old_tupdesc = RelationGetDescr(rel); - AttrMap *map; - - rel = resultRelInfo->ri_PartitionRoot; - tupdesc = RelationGetDescr(rel); - /* a reverse map */ - map = build_attrmap_by_name_if_req(old_tupdesc, - tupdesc); - - /* - * Partition-specific slot's tupdesc can't be changed, so - * allocate a new one. - */ - if (map != NULL) - slot = execute_attr_map_slot(map, slot, - MakeTupleTableSlot(tupdesc, &TTSOpsVirtual)); - } - - insertedCols = GetInsertedColumns(resultRelInfo, estate); - updatedCols = GetUpdatedColumns(resultRelInfo, estate); - modifiedCols = bms_union(insertedCols, updatedCols); - val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel), + val_desc = ExecBuildSlotValueDescription(estate, + resultRelInfo, slot, - tupdesc, - modifiedCols, 64); ereport(ERROR, (errcode(ERRCODE_CHECK_VIOLATION), errmsg("new row for relation \"%s\" violates check constraint \"%s\"", - RelationGetRelationName(orig_rel), failed), + RelationGetRelationName(rel), failed), val_desc ? errdetail("Failing row contains %s.", val_desc) : 0, - errtableconstraint(orig_rel, failed))); + errtableconstraint(rel, failed))); } } } @@ -2028,8 +1925,6 @@ void ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate) { - Relation rel = resultRelInfo->ri_RelationDesc; - TupleDesc tupdesc = RelationGetDescr(rel); ExprContext *econtext; ListCell *l1, *l2; @@ -2067,9 +1962,6 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo, if (!ExecQual(wcoExpr, econtext)) { char *val_desc; - Bitmapset *modifiedCols; - Bitmapset *insertedCols; - Bitmapset *updatedCols; switch (wco->kind) { @@ -2083,34 +1975,9 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo, * USING policy. */ case WCO_VIEW_CHECK: - /* See the comment in ExecConstraints(). */ - if (resultRelInfo->ri_PartitionRoot) - { - TupleDesc old_tupdesc = RelationGetDescr(rel); - AttrMap *map; - - rel = resultRelInfo->ri_PartitionRoot; - tupdesc = RelationGetDescr(rel); - /* a reverse map */ - map = build_attrmap_by_name_if_req(old_tupdesc, - tupdesc); - - /* - * Partition-specific slot's tupdesc can't be changed, - * so allocate a new one. - */ - if (map != NULL) - slot = execute_attr_map_slot(map, slot, - MakeTupleTableSlot(tupdesc, &TTSOpsVirtual)); - } - - insertedCols = GetInsertedColumns(resultRelInfo, estate); - updatedCols = GetUpdatedColumns(resultRelInfo, estate); - modifiedCols = bms_union(insertedCols, updatedCols); - val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel), + val_desc = ExecBuildSlotValueDescription(estate, + resultRelInfo, slot, - tupdesc, - modifiedCols, 64); ereport(ERROR, @@ -2163,8 +2030,7 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo, * * Also, unlike the case with index entries, we need to be prepared to ignore * dropped columns. We used to use the slot's tuple descriptor to decode the - * data, but the slot's descriptor doesn't identify dropped columns, so we - * now need to be passed the relation's descriptor. + * data, but the slot's descriptor doesn't identify dropped columns. * * Note that, like BuildIndexValueDescription, if the user does not have * permission to view any of the columns involved, a NULL is returned. Unlike @@ -2173,10 +2039,9 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo, * columns they are. */ static char * -ExecBuildSlotValueDescription(Oid reloid, +ExecBuildSlotValueDescription(EState *estate, + ResultRelInfo *resultRelInfo, TupleTableSlot *slot, - TupleDesc tupdesc, - Bitmapset *modifiedCols, int maxfieldlen) { StringInfoData buf; @@ -2187,6 +2052,9 @@ ExecBuildSlotValueDescription(Oid reloid, AclResult aclresult; bool table_perm = false; bool any_perm = false; + Oid reloid = RelationGetRelid(resultRelInfo->ri_RelationDesc); + TupleDesc tupdesc; + Bitmapset *modifiedCols; /* * Check if RLS is enabled and should be active for the relation; if so, @@ -2196,6 +2064,34 @@ ExecBuildSlotValueDescription(Oid reloid, if (check_enable_rls(reloid, InvalidOid, true) == RLS_ENABLED) return NULL; + /* + * If the tuple has been routed, it's been converted to the partition's + * rowtype, which might differ from the root table's. We must convert it + * back to the root table's rowtype so that val_desc matches the input + * tuple. + */ + if (resultRelInfo->ri_PartitionRootSlot) + { + TupleDesc old_tupdesc; + AttrMap *map; + + tupdesc = resultRelInfo->ri_PartitionRootSlot->tts_tupleDescriptor; + + old_tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc); + /* a reverse map */ + map = build_attrmap_by_name_if_req(old_tupdesc, tupdesc); + + /* Note that this uses a new slot for the converted tuple. */ + if (map != NULL) + slot = execute_attr_map_slot(map, slot, + resultRelInfo->ri_PartitionRootSlot); + } + else + tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc); + + modifiedCols = bms_union(GetInsertedColumns(resultRelInfo, estate), + GetUpdatedColumns(resultRelInfo, estate)); + initStringInfo(&buf); appendStringInfoChar(&buf, '('); diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index fb6ce49..9069c5b 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -527,6 +527,12 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, estate->es_instrument); /* + * ri_PartitionRootSlot will be used when converting partition's tuples to + * root format. + */ + leaf_part_rri->ri_PartitionRootSlot = mtstate->mt_root_tuple_slot; + + /* * Verify result relation is a valid target for an INSERT. An UPDATE of a * partition-key becomes a DELETE+INSERT operation, so this check is still * required when the operation is CMD_UPDATE. diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index d71c0a4..9369ee7 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -2320,8 +2320,14 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) /* If modifying a partitioned table, initialize the root table info */ if (node->rootResultRelIndex >= 0) + { + Relation rootrel; + mtstate->rootResultRelInfo = estate->es_root_result_relations + node->rootResultRelIndex; + rootrel = mtstate->rootResultRelInfo->ri_RelationDesc; + mtstate->mt_root_tuple_slot = table_slot_create(rootrel, NULL); + } mtstate->mt_arowmarks = (List **) palloc0(sizeof(List *) * nplans); mtstate->mt_nplans = nplans; @@ -2403,6 +2409,12 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) eflags); } + /* + * ri_PartitionRootSlot will be used when converting partition's + * tuples to root format. + */ + resultRelInfo->ri_PartitionRootSlot = mtstate->mt_root_tuple_slot; + resultRelInfo++; i++; } @@ -2445,10 +2457,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) * We can skip this setup if it's not a partition key update. */ if (update_tuple_routing_needed) - { ExecSetupChildParentMapForSubplan(mtstate); - mtstate->mt_root_tuple_slot = table_slot_create(rel, NULL); - } /* * Initialize any WITH CHECK OPTION constraints if needed. @@ -2774,12 +2783,10 @@ ExecEndModifyTable(ModifyTableState *node) * and release the slot used for tuple routing, if set. */ if (node->mt_partition_tuple_routing) - { ExecCleanupTupleRouting(node, node->mt_partition_tuple_routing); - if (node->mt_root_tuple_slot) - ExecDropSingleTupleTableSlot(node->mt_root_tuple_slot); - } + if (node->mt_root_tuple_slot) + ExecDropSingleTupleTableSlot(node->mt_root_tuple_slot); /* * Free the exprcontext diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 0fb5d61..fbcff6a 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -486,6 +486,12 @@ typedef struct ResultRelInfo /* relation descriptor for root partitioned table */ Relation ri_PartitionRoot; + /* + * Slot initialized with root table descriptor; may be set even if + * ri_PartitionRoot is not. + */ + TupleTableSlot *ri_PartitionRootSlot; + /* Additional information specific to partition tuple routing */ struct PartitionRoutingInfo *ri_PartitionInfo; diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index 2b68aef..fbfff7b 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -2494,7 +2494,7 @@ ERROR: new row for relation "errtst_child_plaindef" violates check constraint " DETAIL: Failing row contains (10, 1, 15). UPDATE errtst_parent SET data = data + 10 WHERE partid = 20; ERROR: new row for relation "errtst_child_reorder" violates check constraint "errtst_child_reorder_data_check" -DETAIL: Failing row contains (15, 1, 20). +DETAIL: Failing row contains (20, 1, 15). -- direct leaf partition update, without partition id violation BEGIN; UPDATE errtst_child_fastdef SET partid = 1 WHERE partid = 0; diff --git a/src/test/regress/expected/update.out b/src/test/regress/expected/update.out index b7f90de..91a8280 100644 --- a/src/test/regress/expected/update.out +++ b/src/test/regress/expected/update.out @@ -344,7 +344,7 @@ DETAIL: Failing row contains (105, 85, null, b, 15). -- but "a = 'a'" violates partition constraint enforced by root partition) UPDATE part_b_10_b_20 set a = 'a'; ERROR: new row for relation "part_c_1_100" violates partition constraint -DETAIL: Failing row contains (null, 1, 96, 12, a). +DETAIL: Failing row contains (null, 96, a, 12, 1). -- ok, partition key update, no constraint violation UPDATE range_parted set d = d - 10 WHERE d > 10; -- ok, no partition key update, no constraint violation @@ -375,7 +375,7 @@ UPDATE part_b_10_b_20 set c = c + 20 returning c, b, a; -- fail, row movement happens only within the partition subtree. UPDATE part_b_10_b_20 set b = b - 6 WHERE c > 116 returning *; ERROR: new row for relation "part_d_1_15" violates partition constraint -DETAIL: Failing row contains (2, 117, 2, b, 7). +DETAIL: Failing row contains (2, 117, b, 7, 2). -- ok, row movement, with subset of rows moved into different partition. UPDATE range_parted set b = b - 6 WHERE c > 116 returning a, b + c; a | ?column? @@ -817,7 +817,7 @@ INSERT into sub_parted VALUES (1,2,10); -- constraint is inherited from upper root. UPDATE sub_parted set a = 2 WHERE c = 10; ERROR: new row for relation "sub_part2" violates partition constraint -DETAIL: Failing row contains (2, 10, 2). +DETAIL: Failing row contains (2, 2, 10). -- Test update-partition-key, where the unpruned partitions do not have their -- partition keys updated. SELECT tableoid::regclass::text, * FROM list_parted WHERE a = 2 ORDER BY 1; -- 1.8.3.1