Hello, I examined the your patch and it seemed reasonable, but I
have one question about this patch.

You made ec_relids differ to the union of all ec members'
em_relids. Is it right?

At Mon, 03 Mar 2014 14:05:10 -0500, Tom Lane wrote
> Noah Misch <n...@leadboat.com> writes:
> > If you are convinced that a separate flattening pass is best, that suffices
> > for me at this stage.  Please submit the patch you have in mind, 
> > incorporating
> > any improvements from the v7 patch that are relevant to both approaches.
> I went back and re-read the original message, and this time it struck me
> that really the issue here is that add_child_rel_equivalences() doesn't
> think it has to deal with the case of a "parent" rel being itself a child.

Yes, that is what I tried to solve in one of the first patch but
the brute way led to failure:(

> That's not inherent though; it's just that it didn't occur to me at the
> time that such a situation could arise.  It takes only very small changes
> to allow that to happen.  

I saw what you did. I marked not-full-fledged eq members as 'not
child'(its real meaning there was full-fledged in contradiction)
to reach the child rels. In contrast, you loosened the ec_relids
shortcut filter and it seems to work but.. Is it right to make
ec_relids different to union of all members' em_relids? This
seems to affect joins afterwards.

> If you do that, as in the attached changes to equivclass.c, you get
> "could not find pathkey item to sort" errors from createplan.c; but
> that's just because create_merge_append_plan() is likewise not expecting
> the mergeappend's parent rel to be itself a child.  Fix for that is
> a one-liner, ie, pass down relids.

This seems to just correcting the over simplification by the
assumption that the best_path there cannot have a parent.

> That gets you to a point where the code generates a valid plan, but it's
> got nested MergeAppends, which are unnecessary expense.  Kyotaro-san's
> original fix for that was overcomplicated.  It's sufficient to teach
> accumulate_append_subpath() to flatten nested MergeAppendPaths.

Ah, it was in typ1-collapse patch. As you might noticed seeing
there, I couldn't put the assumption that directly nested
MergeAppendPaths share the same pathkeys (really?). It's no
problem if the assumption goes on.

> In short, the attached patch seems to fix it, for a lot less added
> complication than anything else that's been discussed on this thread.
> I've only lightly tested it (it could use a new regression test case),
> but unless someone can find a flaw I think we should use this approach.

Mmm. That's motifying but you seems to be right :) Equipping this
with some regression tests become my work from now.


Kyotaro Horiguchi
NTT Open Source Software Center

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to