Re: [HACKERS] [PATCH] Incremental sort

On Mon, Jan 8, 2018 at 2:29 PM, Antonin Houska <ah@cybertec.at> wrote:

Alexander Korotkov <a.korotkov@postgrespro.ru> wrote:

> Antonin Houska <ah@cybertec.at> wrote:

> >  Shouldn't the test contain *both* cases?

> Thank you for pointing that. Sure, both cases are better. I've added second case as well as comments. Patch is attached.

I'm fine with the tests now but have a minor comment on this comment:

-- CROSS JOIN, not pushed down, because we don't push down LIMIT and remote side
-- can't perform top-N sort like local side can.

I think the note on LIMIT push-down makes the comment less clear because
there's no difference in processing the LIMIT: EXPLAIN shows that both

SELECT t1.c1, t2.c1 FROM ft1 t1 CROSS JOIN ft2 t2 ORDER BY t1.c1, t2.c1 OFFSET
100 LIMIT 10;

and

SELECT t1.c3, t2.c3 FROM ft1 t1 CROSS JOIN ft2 t2 ORDER BY t1.c3, t2.c3 OFFSET
100 LIMIT 10;

evaluate the LIMIT clause only locally.

What I consider the important difference is that the 2nd case does not
generate the appropriate input for remote incremental sort (while incremental
sort tends to be very cheap). Therefore it's cheaper to do no remote sort at
all and perform the top-N sort locally than to do a regular (non-incremental)
remote sort.

I have no other questions about this patch. I expect the CFM to set the status
to "ready for committer" as soon as the other reviewers confirm they're happy
about the patch status.

Good, thank you.  Let's see what other reviewers will say.