ExplainModifyTarget doesn't work as expected
Hi,
I think ExplainModifyTarget should show the parent of the inheritance
tree in multi-target-table cases, as described there, but noticed that
it doesn't always work like that. Here is an example.
postgres=# create table parent (a int check (a < 0) no inherit);
CREATE TABLE
postgres=# create table child (a int check (a >= 0));
CREATE TABLE
postgres=# alter table child inherit parent;
ALTER TABLE
postgres=# explain update parent set a = a * 2 where a >= 0;
QUERY PLAN
---------------------------------------------------------------
Update on child (cost=0.00..42.00 rows=800 width=10)
-> Seq Scan on child (cost=0.00..42.00 rows=800 width=10)
Filter: (a >= 0)
(3 rows)
IIUC, I think this is because ExplainModifyTarget doesn't take into
account that the parent *can* be excluded by constraint exclusion. So,
I added a field to ModifyTable to record the parent, apart from
resultRelations. (More precisely, the parent in its role as a simple
member of the inheritance tree is recorded so that appending digits to
refname in select_rtable_names_for_explain works as before.) Attached
is a proposed patch for that.
Thanks,
Best regards,
Etsuro Fujita
Attachments:
explain-inherited-updates.patchtext/x-patch; name=explain-inherited-updates.patchDownload
*** a/src/backend/commands/explain.c
--- b/src/backend/commands/explain.c
***************
*** 728,736 **** ExplainPreScanNode(PlanState *planstate, Bitmapset **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;
--- 728,735 ----
((Scan *) plan)->scanrelid);
break;
case T_ModifyTable:
*rels_used = bms_add_member(*rels_used,
! ((ModifyTable *) plan)->resultRelation);
break;
default:
break;
***************
*** 2109,2122 **** ExplainScanTarget(Scan *plan, ExplainState *es)
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);
}
--- 2108,2114 ----
static void
ExplainModifyTarget(ModifyTable *plan, ExplainState *es)
{
! Index rti = plan->resultRelation;
ExplainTargetRel((Plan *) plan, rti, es);
}
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 175,180 **** _copyModifyTable(const ModifyTable *from)
--- 175,181 ----
*/
COPY_SCALAR_FIELD(operation);
COPY_SCALAR_FIELD(canSetTag);
+ COPY_SCALAR_FIELD(resultRelation);
COPY_NODE_FIELD(resultRelations);
COPY_SCALAR_FIELD(resultRelIndex);
COPY_NODE_FIELD(plans);
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 327,332 **** _outModifyTable(StringInfo str, const ModifyTable *node)
--- 327,333 ----
WRITE_ENUM_FIELD(operation, CmdType);
WRITE_BOOL_FIELD(canSetTag);
+ WRITE_UINT_FIELD(resultRelation);
WRITE_NODE_FIELD(resultRelations);
WRITE_INT_FIELD(resultRelIndex);
WRITE_NODE_FIELD(plans);
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***************
*** 4809,4814 **** make_result(PlannerInfo *root,
--- 4809,4815 ----
ModifyTable *
make_modifytable(PlannerInfo *root,
CmdType operation, bool canSetTag,
+ Index resultRelation,
List *resultRelations, List *subplans,
List *withCheckOptionLists, List *returningLists,
List *rowMarks, int epqParam)
***************
*** 4857,4862 **** make_modifytable(PlannerInfo *root,
--- 4858,4864 ----
node->operation = operation;
node->canSetTag = canSetTag;
+ node->resultRelation = resultRelation;
node->resultRelations = resultRelations;
node->resultRelIndex = -1; /* will be set correctly in setrefs.c */
node->plans = subplans;
*** 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
***************
*** 754,759 **** set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
--- 754,761 ----
splan->plan.targetlist = copyObject(linitial(newRL));
}
+ splan->resultRelation += rtoffset;
+
foreach(l, splan->resultRelations)
{
lfirst_int(l) += rtoffset;
*** a/src/include/nodes/plannodes.h
--- b/src/include/nodes/plannodes.h
***************
*** 174,179 **** typedef struct ModifyTable
--- 174,180 ----
Plan plan;
CmdType operation; /* INSERT, UPDATE, or DELETE */
bool canSetTag; /* do we set the command tag/es_processed? */
+ Index resultRelation; /* Parent RT index for use of EXPLAIN */
List *resultRelations; /* integer list of RT indexes */
int resultRelIndex; /* index of first resultRel in plan's list */
List *plans; /* plan(s) producing source data */
*** 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);
On 12/22/14 12:50 AM, Etsuro Fujita wrote:
I think ExplainModifyTarget should show the parent of the inheritance
tree in multi-target-table cases, as described there, but noticed that
it doesn't always work like that. Here is an example.
Anything ever happen with this?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015/01/27 9:15, Jim Nasby wrote:
On 12/22/14 12:50 AM, Etsuro Fujita wrote:
I think ExplainModifyTarget should show the parent of the inheritance
tree in multi-target-table cases, as described there, but noticed that
it doesn't always work like that. Here is an example.Anything ever happen with this?
Nothing. I'll add this to the next CF.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi Fujita-san,
I agree that it's a problem, and it looks more severe when there are
multiple children
postgres=# create table parent (a int check (a < 0) no inherit);
CREATE TABLE
postgres=# create table child1 (a int check (a >= 0));
CREATE TABLE
postgres=# create table child2 (a int check (a >= 0));
CREATE TABLE
postgres=# create table child3 (a int check (a >= 0));
CREATE TABLE
postgres=# alter table child1 inherit parent;
ALTER TABLE
postgres=# alter table child2 inherit parent;
ALTER TABLE
postgres=# alter table child3 inherit parent;
ALTER TABLE
postgres=# explain update parent set a = a * 2 where a >= 0;
QUERY PLAN
----------------------------------------------------------------
Update on child1 (cost=0.00..126.00 rows=2400 width=10)
-> Seq Scan on child1 (cost=0.00..42.00 rows=800 width=10)
Filter: (a >= 0)
-> Seq Scan on child2 (cost=0.00..42.00 rows=800 width=10)
Filter: (a >= 0)
-> Seq Scan on child3 (cost=0.00..42.00 rows=800 width=10)
Filter: (a >= 0)
(7 rows)
It's certainly confusing why would an update on child1 cause scan on child*.
But I also think that showing parent's name with Upate would be misleading
esp. when user expects it to get filtered because of constraint exclusion.
Instead, can we show all the relations that are being modified e.g Update
on child1, child2, child3. That will disambiguate everything.
On Mon, Dec 22, 2014 at 12:20 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp
wrote:
Hi,
I think ExplainModifyTarget should show the parent of the inheritance
tree in multi-target-table cases, as described there, but noticed that
it doesn't always work like that. Here is an example.postgres=# create table parent (a int check (a < 0) no inherit);
CREATE TABLE
postgres=# create table child (a int check (a >= 0));
CREATE TABLE
postgres=# alter table child inherit parent;
ALTER TABLE
postgres=# explain update parent set a = a * 2 where a >= 0;
QUERY PLAN
---------------------------------------------------------------
Update on child (cost=0.00..42.00 rows=800 width=10)
-> Seq Scan on child (cost=0.00..42.00 rows=800 width=10)
Filter: (a >= 0)
(3 rows)IIUC, I think this is because ExplainModifyTarget doesn't take into
account that the parent *can* be excluded by constraint exclusion. So,
I added a field to ModifyTable to record the parent, apart from
resultRelations. (More precisely, the parent in its role as a simple
member of the inheritance tree is recorded so that appending digits to
refname in select_rtable_names_for_explain works as before.) Attached
is a proposed patch for that.Thanks,
Best regards,
Etsuro Fujita--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Hi Ashutosh,
Thank you for the review!
On 2015/02/03 15:32, Ashutosh Bapat wrote:
I agree that it's a problem, and it looks more severe when there are
multiple children
postgres=# create table parent (a int check (a < 0) no inherit);
CREATE TABLE
postgres=# create table child1 (a int check (a >= 0));
CREATE TABLE
postgres=# create table child2 (a int check (a >= 0));
CREATE TABLE
postgres=# create table child3 (a int check (a >= 0));
CREATE TABLE
postgres=# alter table child1 inherit parent;
ALTER TABLE
postgres=# alter table child2 inherit parent;
ALTER TABLE
postgres=# alter table child3 inherit parent;
ALTER TABLE
postgres=# explain update parent set a = a * 2 where a >= 0;
QUERY PLAN
----------------------------------------------------------------
Update on child1 (cost=0.00..126.00 rows=2400 width=10)
-> Seq Scan on child1 (cost=0.00..42.00 rows=800 width=10)
Filter: (a >= 0)
-> Seq Scan on child2 (cost=0.00..42.00 rows=800 width=10)
Filter: (a >= 0)
-> Seq Scan on child3 (cost=0.00..42.00 rows=800 width=10)
Filter: (a >= 0)
(7 rows)It's certainly confusing why would an update on child1 cause scan on child*.
Yeah, I think so too.
But I also think that showing parent's name with Upate would be
misleading esp. when user expects it to get filtered because of
constraint exclusion.Instead, can we show all the relations that are being modified e.g
Update on child1, child2, child3. That will disambiguate everything.
That's an idea, but my concern about that is the cases where there are a
large number of child tables as the EXPLAIN would be difficult to read
in such cases.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Well let's see what others think. Also, we might want to separate that
information on result relations heading probably.
On Fri, Feb 6, 2015 at 1:35 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
wrote:
Hi Ashutosh,
Thank you for the review!
On 2015/02/03 15:32, Ashutosh Bapat wrote:
I agree that it's a problem, and it looks more severe when there are
multiple children
postgres=# create table parent (a int check (a < 0) no inherit);
CREATE TABLE
postgres=# create table child1 (a int check (a >= 0));
CREATE TABLE
postgres=# create table child2 (a int check (a >= 0));
CREATE TABLE
postgres=# create table child3 (a int check (a >= 0));
CREATE TABLE
postgres=# alter table child1 inherit parent;
ALTER TABLE
postgres=# alter table child2 inherit parent;
ALTER TABLE
postgres=# alter table child3 inherit parent;
ALTER TABLE
postgres=# explain update parent set a = a * 2 where a >= 0;
QUERY PLAN
----------------------------------------------------------------
Update on child1 (cost=0.00..126.00 rows=2400 width=10)
-> Seq Scan on child1 (cost=0.00..42.00 rows=800 width=10)
Filter: (a >= 0)
-> Seq Scan on child2 (cost=0.00..42.00 rows=800 width=10)
Filter: (a >= 0)
-> Seq Scan on child3 (cost=0.00..42.00 rows=800 width=10)
Filter: (a >= 0)
(7 rows)It's certainly confusing why would an update on child1 cause scan on
child*.Yeah, I think so too.
But I also think that showing parent's name with Upate would be
misleading esp. when user expects it to get filtered because of
constraint exclusion.Instead, can we show all the relations that are being modified e.g
Update on child1, child2, child3. That will disambiguate everything.That's an idea, but my concern about that is the cases where there are a
large number of child tables as the EXPLAIN would be difficult to read in
such cases.Best regards,
Etsuro Fujita
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
On 2015/02/03 15:32, Ashutosh Bapat wrote:
Instead, can we show all the relations that are being modified e.g
Update on child1, child2, child3. That will disambiguate everything.
That's an idea, but my concern about that is the cases where there are a
large number of child tables as the EXPLAIN would be difficult to read
in such cases.
I concur, that would *not* be an improvement in readability. Moreover,
I don't think it really fixes the issue: what we want to show is a table
name in Modify that matches what the user wrote in the query. Given that
context, the user should be able to understand why some tables are listed
below that and others are not.
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.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
Thanks!
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
explain-inherited-updates-v2.patchtext/x-patch; name=explain-inherited-updates-v2.patchDownload
*** 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);
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
On 2015/02/10 14:49, Etsuro Fujita wrote:
On 2015/02/07 1:09, Tom Lane wrote:
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.
I looked at this and was not really pleased with the results, in
particular the way that you'd moved a bare minimum number of the code
stanzas for struct Scan so that things still compiled. The new placement
of those stanzas didn't make any sense in context, and it was a
significant violation of our layout rule that files dealing with various
types of Nodes should whenever possible handle those Nodes in the same
order that they're listed in nodes.h, so that it's clear where new bits of
code ought to be placed. (I'm aware that there are historical violations
of this policy in a few places, but that doesn't make it a bad policy to
follow.)
I experimented with relocating ModifyTable down into the group of Scan
nodes in this global ordering, but soon decided that that would involve
far more code churn than the idea was worth.
So I went back to your v1 patch and have now committed that with some
cosmetic modifications. Sorry for making you put time into a dead end.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015/02/18 8:13, Tom Lane wrote:
So I went back to your v1 patch and have now committed that with some
cosmetic modifications. Sorry for making you put time into a dead end.
I don't mind it. Thanks for committing the patch!
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers