Re: ExplainModifyTarget doesn't work as expected

Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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