Re: [PATCH] Incremental sort (was: PoC: Partial sort)

On Fri, Mar 20, 2020 at 8:56 PM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> Hi,
>
> I've looked at v38 but it seems it's a bit broken by some recent explain
> changes (mostly missing type in declarations). Attached is v39 fixing
> those issues, and including a bunch of fixes based on a review - most of
> the changes is in comments, so I've instead kept them in separate "fix"
> patches after each part.
>
> In general I'm mostly happy with the current shape of the patch, and
> unless there are some objections I'd like to get some of it committed
> sometime next week.
>
> I've done a fair amount of testing with various queries, and the plan
> changes seem pretty sensible. I'm still not entirely sure whether to be
> a bit conservative and only tweak the first patch adding incremental
> sort to extra places, or commit both.
>
> The main thing I still have on my plate is assessment of how much more
> expensive can the planning due to increased number of paths we
> generate/keep (due to considering extra pathkeys). I haven't seen any
> significant slowdowns, but I plan to look at some extreme cases (many
> similar and applicable indexes etc.).

I'm currently incorporating all of the fixes you proposed into the
main patch series, as well as doing a thorough read-through of the
current state of the patch. I'm hoping to reply tomorrow with:

- Fix patches of my own to clean up and add additional comments.
- Catalog all of the current open questions (XXX, etc.) in the patch
to more easily discuss them in the mailing list.

One question I have while I work on that: I've noticed some confusion
in the patch as to whether we should refer to the node below the
incremental sort node in the plan tree (i.e., the node we get tuples
from) as the inner node or the outer node. Intuitively I'd expect to
call it the inner node, but the original patch referred to it
frequently as the outer node. The outerPlanState/innerPlanState macro
comments don't offer a lot of clarification though they're "to avoid
confusion" about right/left inner/outer. I suppose if the
outerPlanState macro is working here the correct term should be outer?

Thanks,
James