On Mon, Jan 8, 2018 at 2:29 PM, Antonin Houska <a...@cybertec.at> wrote:
> Alexander Korotkov <a.korot...@postgrespro.ru> wrote: > > > Antonin Houska <a...@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. > 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
incremental-sort-15.patch
Description: Binary data
