Re: [HACKERS] ExplainModifyTarget doesn't work as expected
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
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
Etsuro Fujita 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
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
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/bac
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
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
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
Etsuro Fujita 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
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
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 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
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
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
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
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 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
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
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
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
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
[HACKERS] 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
*** 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;
