Обсуждение: 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

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

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

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

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

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

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

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

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