On 2015/02/10 14:49, Etsuro Fujita wrote:
> On 2015/02/07 1:09, Tom Lane wrote:
>> IIRC, this code was written at a time when we didn't have NO INHERIT check
>> constraints and so it was impossible for the parent table to get optimized
>> away while leaving children. So the comment in ExplainModifyTarget was
>> good at the time. But it no longer is.
>>
>> I think your basic idea of preserving the original parent table's relid
>> is correct; but instead of doing it like this patch does, I'd be inclined
>> to make ModifyTable inherit from Scan not Plan, and use the scan.scanrelid
>> field to carry the parent RTI. Then you would probably end up with a net
>> savings of code rather than net addition; certainly ExplainModifyTarget
>> would go away entirely since you'd just treat ModifyTable like any other
>> Scan in this part of EXPLAIN.
>
> Will follow your revision.
Done. Attached is an updated version of the patch.
Best regards,
Etsuro Fujita
*** a/src/backend/commands/explain.c
--- b/src/backend/commands/explain.c
***************
*** 95,101 **** static const char *explain_get_index_name(Oid indexId);
static void ExplainIndexScanDetails(Oid indexid, ScanDirection indexorderdir,
ExplainState *es);
static void ExplainScanTarget(Scan *plan, ExplainState *es);
- static void ExplainModifyTarget(ModifyTable *plan, ExplainState *es);
static void ExplainTargetRel(Plan *plan, Index rti, ExplainState *es);
static void show_modifytable_info(ModifyTableState *mtstate, ExplainState *es);
static void ExplainMemberNodes(List *plans, PlanState **planstates,
--- 95,100 ----
***************
*** 724,736 **** ExplainPreScanNode(PlanState *planstate, Bitmapset **rels_used)
case T_WorkTableScan:
case T_ForeignScan:
case T_CustomScan:
- *rels_used = bms_add_member(*rels_used,
- ((Scan *) plan)->scanrelid);
- break;
case T_ModifyTable:
- /* cf ExplainModifyTarget */
*rels_used = bms_add_member(*rels_used,
! linitial_int(((ModifyTable *) plan)->resultRelations));
break;
default:
break;
--- 723,731 ----
case T_WorkTableScan:
case T_ForeignScan:
case T_CustomScan:
case T_ModifyTable:
*rels_used = bms_add_member(*rels_used,
! ((Scan *) plan)->scanrelid);
break;
default:
break;
***************
*** 1067,1072 **** ExplainNode(PlanState *planstate, List *ancestors,
--- 1062,1068 ----
case T_WorkTableScan:
case T_ForeignScan:
case T_CustomScan:
+ case T_ModifyTable:
ExplainScanTarget((Scan *) plan, es);
break;
case T_IndexScan:
***************
*** 1101,1109 **** ExplainNode(PlanState *planstate, List *ancestors,
ExplainPropertyText("Index Name", indexname, es);
}
break;
- case T_ModifyTable:
- ExplainModifyTarget((ModifyTable *) plan, es);
- break;
case T_NestLoop:
case T_MergeJoin:
case T_HashJoin:
--- 1097,1102 ----
***************
*** 2104,2127 **** ExplainScanTarget(Scan *plan, ExplainState *es)
}
/*
- * Show the target of a ModifyTable node
- */
- static void
- ExplainModifyTarget(ModifyTable *plan, ExplainState *es)
- {
- Index rti;
-
- /*
- * We show the name of the first target relation. In multi-target-table
- * cases this should always be the parent of the inheritance tree.
- */
- Assert(plan->resultRelations != NIL);
- rti = linitial_int(plan->resultRelations);
-
- ExplainTargetRel((Plan *) plan, rti, es);
- }
-
- /*
* Show the target relation of a scan or modify node
*/
static void
--- 2097,2102 ----
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 120,125 **** CopyPlanFields(const Plan *from, Plan *newnode)
--- 120,140 ----
}
/*
+ * CopyScanFields
+ *
+ * This function copies the fields of the Scan node. It is used by
+ * all the copy functions for classes which inherit from Scan.
+ */
+ static void
+ CopyScanFields(const Scan *from, Scan *newnode)
+ {
+ CopyPlanFields((const Plan *) from, (Plan *) newnode);
+
+ COPY_SCALAR_FIELD(scanrelid);
+ }
+
+
+ /*
* _copyPlan
*/
static Plan *
***************
*** 168,174 **** _copyModifyTable(const ModifyTable *from)
/*
* copy node superclass fields
*/
! CopyPlanFields((const Plan *) from, (Plan *) newnode);
/*
* copy remainder of node
--- 183,189 ----
/*
* copy node superclass fields
*/
! CopyScanFields((const Scan *) from, (Scan *) newnode);
/*
* copy remainder of node
***************
*** 306,325 **** _copyBitmapOr(const BitmapOr *from)
/*
- * CopyScanFields
- *
- * This function copies the fields of the Scan node. It is used by
- * all the copy functions for classes which inherit from Scan.
- */
- static void
- CopyScanFields(const Scan *from, Scan *newnode)
- {
- CopyPlanFields((const Plan *) from, (Plan *) newnode);
-
- COPY_SCALAR_FIELD(scanrelid);
- }
-
- /*
* _copyScan
*/
static Scan *
--- 321,326 ----
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 323,329 **** _outModifyTable(StringInfo str, const ModifyTable *node)
{
WRITE_NODE_TYPE("MODIFYTABLE");
! _outPlanInfo(str, (const Plan *) node);
WRITE_ENUM_FIELD(operation, CmdType);
WRITE_BOOL_FIELD(canSetTag);
--- 323,329 ----
{
WRITE_NODE_TYPE("MODIFYTABLE");
! _outScanInfo(str, (const Scan *) node);
WRITE_ENUM_FIELD(operation, CmdType);
WRITE_BOOL_FIELD(canSetTag);
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***************
*** 4809,4820 **** make_result(PlannerInfo *root,
ModifyTable *
make_modifytable(PlannerInfo *root,
CmdType operation, bool canSetTag,
List *resultRelations, List *subplans,
List *withCheckOptionLists, List *returningLists,
List *rowMarks, int epqParam)
{
ModifyTable *node = makeNode(ModifyTable);
! Plan *plan = &node->plan;
double total_size;
List *fdw_private_list;
ListCell *subnode;
--- 4809,4821 ----
ModifyTable *
make_modifytable(PlannerInfo *root,
CmdType operation, bool canSetTag,
+ Index resultRelation,
List *resultRelations, List *subplans,
List *withCheckOptionLists, List *returningLists,
List *rowMarks, int epqParam)
{
ModifyTable *node = makeNode(ModifyTable);
! Plan *plan = &node->scan.plan;
double total_size;
List *fdw_private_list;
ListCell *subnode;
***************
*** 4849,4860 **** make_modifytable(PlannerInfo *root,
else
plan->plan_width = 0;
! node->plan.lefttree = NULL;
! node->plan.righttree = NULL;
! node->plan.qual = NIL;
/* setrefs.c will fill in the targetlist, if needed */
! node->plan.targetlist = NIL;
node->operation = operation;
node->canSetTag = canSetTag;
node->resultRelations = resultRelations;
--- 4850,4862 ----
else
plan->plan_width = 0;
! plan->lefttree = NULL;
! plan->righttree = NULL;
! plan->qual = NIL;
/* setrefs.c will fill in the targetlist, if needed */
! plan->targetlist = NIL;
+ node->scan.scanrelid = resultRelation;
node->operation = operation;
node->canSetTag = canSetTag;
node->resultRelations = resultRelations;
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
***************
*** 607,612 **** subquery_planner(PlannerGlobal *glob, Query *parse,
--- 607,613 ----
plan = (Plan *) make_modifytable(root,
parse->commandType,
parse->canSetTag,
+ parse->resultRelation,
list_make1_int(parse->resultRelation),
list_make1(plan),
withCheckOptionLists,
***************
*** 790,795 **** inheritance_planner(PlannerInfo *root)
--- 791,797 ----
{
Query *parse = root->parse;
int parentRTindex = parse->resultRelation;
+ int pseudoParentRTindex = -1;
List *final_rtable = NIL;
int save_rel_array_size = 0;
RelOptInfo **save_rel_array = NULL;
***************
*** 925,930 **** inheritance_planner(PlannerInfo *root)
--- 927,940 ----
appinfo->child_relid = subroot.parse->resultRelation;
/*
+ * If this child rel was the first target relation, it should always be
+ * the parent rel in its role as a simple member of the inheritance set.
+ * Get it for use of EXPLAIN.
+ */
+ if (pseudoParentRTindex == -1)
+ pseudoParentRTindex = appinfo->child_relid;
+
+ /*
* If this child rel was excluded by constraint exclusion, exclude it
* from the result plan.
*/
***************
*** 1051,1056 **** inheritance_planner(PlannerInfo *root)
--- 1061,1067 ----
return (Plan *) make_modifytable(root,
parse->commandType,
parse->canSetTag,
+ pseudoParentRTindex,
resultRelations,
subplans,
withCheckOptionLists,
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
***************
*** 707,714 **** set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
{
ModifyTable *splan = (ModifyTable *) plan;
! Assert(splan->plan.targetlist == NIL);
! Assert(splan->plan.qual == NIL);
splan->withCheckOptionLists =
fix_scan_list(root, splan->withCheckOptionLists, rtoffset);
--- 707,715 ----
{
ModifyTable *splan = (ModifyTable *) plan;
! splan->scan.scanrelid += rtoffset;
! Assert(splan->scan.plan.targetlist == NIL);
! Assert(splan->scan.plan.qual == NIL);
splan->withCheckOptionLists =
fix_scan_list(root, splan->withCheckOptionLists, rtoffset);
***************
*** 751,757 **** set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
* we don't have to do set_returning_clause_references()
* twice on identical targetlists.
*/
! splan->plan.targetlist = copyObject(linitial(newRL));
}
foreach(l, splan->resultRelations)
--- 752,758 ----
* we don't have to do set_returning_clause_references()
* twice on identical targetlists.
*/
! splan->scan.plan.targetlist = copyObject(linitial(newRL));
}
foreach(l, splan->resultRelations)
*** a/src/include/nodes/plannodes.h
--- b/src/include/nodes/plannodes.h
***************
*** 144,149 **** typedef struct Plan
--- 144,160 ----
/* ----------------
+ * Scan node
+ * ----------------
+ */
+ typedef struct Scan
+ {
+ Plan plan;
+ Index scanrelid; /* relid is index into the range table */
+ } Scan;
+
+
+ /* ----------------
* Result node -
* If no outer plan, evaluate a variable-free targetlist.
* If outer plan, return tuples from outer plan (after a level of
***************
*** 171,177 **** typedef struct Result
*/
typedef struct ModifyTable
{
! Plan plan;
CmdType operation; /* INSERT, UPDATE, or DELETE */
bool canSetTag; /* do we set the command tag/es_processed? */
List *resultRelations; /* integer list of RT indexes */
--- 182,188 ----
*/
typedef struct ModifyTable
{
! Scan scan;
CmdType operation; /* INSERT, UPDATE, or DELETE */
bool canSetTag; /* do we set the command tag/es_processed? */
List *resultRelations; /* integer list of RT indexes */
***************
*** 265,275 **** typedef struct BitmapOr
* Scan nodes
* ==========
*/
- typedef struct Scan
- {
- Plan plan;
- Index scanrelid; /* relid is index into the range table */
- } Scan;
/* ----------------
* sequential scan node
--- 276,281 ----
*** a/src/include/optimizer/planmain.h
--- b/src/include/optimizer/planmain.h
***************
*** 82,87 **** extern Result *make_result(PlannerInfo *root, List *tlist,
--- 82,88 ----
Node *resconstantqual, Plan *subplan);
extern ModifyTable *make_modifytable(PlannerInfo *root,
CmdType operation, bool canSetTag,
+ Index resultRelation,
List *resultRelations, List *subplans,
List *withCheckOptionLists, List *returningLists,
List *rowMarks, int epqParam);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
