Date: Sat, 28 Mar 2020 22:30:50 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: James Coleman <jtc331@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
	Michael Paquier <michael@paquier.xyz>,
	Rafia Sabih <rafia.pghackers@gmail.com>,
	Peter Geoghegan <pg@bowt.ie>, Simon Riggs <simon@2ndquadrant.com>,
	Shaun Thomas <shaun.thomas@2ndquadrant.com>,
	Dmitry Dolgov <9erthalion6@gmail.com>,
	Alexander Korotkov <a.korotkov@postgrespro.ru>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>,
	Andreas Karlsson <andreas@proxel.se>
Subject: Re: [PATCH] Incremental sort (was: PoC: Partial sort)
Message-ID: <20200328213050.byyeuozylnxv2bhf@development>
References: 
 <CAAaqYe_J=kVVb9J3EegU39DSFdMDo_Yk5ptGwbX0VJX6wb1-rw@mail.gmail.com>
 <20200324230831.5zvjpgayh35bxedp@development>
 <CAAaqYe9+ap2SbU_E2WaC4F9ZMF4oa=pJZ1NBwaKDMP6GFUA77g@mail.gmail.com>
 <CAAaqYe-sOp2o=L7nvGZDJ6GsL9=b_ztrGE1rhyi+F82p3my2bQ@mail.gmail.com>
 <CAAaqYe-mmeb9gSL7HMYX_YpH0jCv60rEeBzw3C8G95n0z+0Lxw@mail.gmail.com>
 <CAAaqYe9hWS174Q4RQOm3ydM3rZG+0fp5QA34JJ9egz5r0DZdVA@mail.gmail.com>
 <20200328011913.btjuwwkhieav3aod@development>
 <CAAaqYe_FSUOY7jA3VGQh-TOXJUOqM-3On-oJHaJ4htQwQRox4g@mail.gmail.com>
 <20200328025830.6v6ogkseohakc32q@development>
 <CAAaqYe-Jhp-QEC2-pwg61aF0vbGR=gEW657cnXxcqUJvdvBaQA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAAaqYe-Jhp-QEC2-pwg61aF0vbGR=gEW657cnXxcqUJvdvBaQA@mail.gmail.com>
Precedence: bulk

On Sat, Mar 28, 2020 at 10:19:04AM -0400, James Coleman wrote:
>On Fri, Mar 27, 2020 at 10:58 PM Tomas Vondra
><tomas.vondra@2ndquadrant.com> wrote:
>>
>> ...
>>
>> The more I look at pathkeys_useful_for_ordering() the more I think the
>> get_useful_pathkeys_for_relation() function should look more like it
>> than the postgres_fdw one ...
>
>If we go down that path (and indeed this is actually implied by
>removing the volatile check too), doesn't that really just mean (at
>least for now) that get_useful_pathkeys_for_relation goes away
>entirely and we effectively use root->query_pathkeys directly? The
>only thing your lose them is the check that each eclass has a member
>in the rel. But that check probably wasn't quite right anyway: at
>least for incremental sort (maybe not regular sort), I think we
>probably don't necessarily care that the entire list has members in
>the rel, but rather that some prefix does, right? The idea there would
>be that we could create a gather merge here that supplies a partial
>ordering (that prefix of query_pathkeys) and then above it the planner
>might place another incremental sort (say above a join), or perhaps
>even a join above that would actually be sufficient (since many joins
>are capable of providing an ordering anyway).
>
>I've attached (added prefix .txt to avoid the cfbot assuming this is a
>full series) an idea in that direction to see if we're thinking along
>the same route. You'll want to apply and view with `git diff -w`
>probably.
>

Hmmm, I'm not sure the patch is quite correct.

Firstly, I suggest we don't remove get_useful_pathkeys_for_relation
entirely, because that allows us to add more useful pathkeys in the
future (even if we don't consider pathkeys useful for merging now).
We could also do the EC optimization in the function, return NIL and
not loop over partial_pathlist at all. That's cosmetic issue, though.

More importantly, I'm not sure this makes sense:

   /* consider incremental sort for interesting orderings */
   useful_pathkeys = truncate_useless_pathkeys(root, rel, subpath->pathkeys);

   ...

   is_sorted = pathkeys_common_contained_in(useful_pathkeys,
                                            subpath->pathkeys,
                                            &presorted_keys);

I mean, useful_pathkeys is bound to be a sublist of subpath->pathkeys,
right? So how could this ever return is_sorted=false?

The whole point is to end up with query_pathkeys (or whatever pathkeys
we deem useful), but this does not do that.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services