Re: [HACKERS] [PATCH] Incremental sort

On Mon, Jan 8, 2018 at 2:29 PM, Antonin Houska <ah(at)cybertec(dot)at> wrote:

> Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
> > Antonin Houska <ah(at)cybertec(dot)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.
>

Agree, these comments are not clear enough. I've rewritten comments: they
became much
more wordy, but now they look clearer for me. Also I've swapped the
queries order, for me
it seems to easier for understanding.

> 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.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company