Re: Window function optimisation, allow pushdowns of items matching PARTITION BY clauses - Mailing list pgsql-hackers

Hello David,

sorry for the late response. I will try out your changes from the view 
of a user in mid-June. However, I can't do a trustworthy code review as 
I'm not an experienced postgre-hacker (yet).

Best Regards
Thomas


Am 14.04.2014 13:19, schrieb David Rowley:
> On 14 April 2014 03:31, Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.us>> wrote:
>
>     David Rowley <dgrowley@gmail.com <mailto:dgrowley@gmail.com>> writes:
>      > On this thread
>      >
>     http://www.postgresql.org/message-id/52C6F712.6040804@student.kit.edu there
>      > was some discussion around allowing push downs of quals that
>     happen to be
>      > in every window clause of the sub query. I've quickly put
>     together a patch
>      > which does this (see attached)
>
>     I think you should have check_output_expressions deal with this,
>     instead.
>     Compare the existing test there for non-DISTINCT output columns.
>
>
> I've moved the code there and it seems like a much better place for it.
> Thanks for the tip.
>
>      > Oh and I know that my function
>     var_exists_in_all_query_partition_by_clauses
>      > has no business in allpaths.c, I'll move it out as soon as I find
>     a better
>      > home for it.
>
>     I might be wrong, but I think you could just embed that search loop in
>     check_output_expressions, and it wouldn't be too ugly.
>
>
> I've changed the helper function to take the TargetEntry now the same
> as targetIsInSortList does for the hasDistinctOn test and I renamed the
> helper function to targetExistsInAllQueryPartitionByClauses. It seems
> much better, but there's probably a bit of room for improving on the
> names some more.
>
> I've included the updated patch with some regression tests.
> The final test I added tests that an unused window which would, if used,
> cause the pushdown not to take place still stops the pushdownf from
> happening... I know you both talked about this case in the other thread,
> but I've done nothing for it as Tom mentioned that this should likely be
> fixed elsewhere, if it's even worth worrying about at all. I'm not quite
> sure if I needed to bother including this test for it, but I did anyway,
> it can be removed if it is deemed unneeded.
>
> Per Thomas' comments, I added a couple of tests that ensure that the
> order of the columns in the partition by clause don't change the
> behaviour. Looking at the implementation of the actual code makes this
> test seems a bit unneeded as really but I thought that the test should
> not assume anything about the implementation so from that point of view
> the extra test seems like a good idea.
>
> For now I can't think of much else to do for the patch, but I'd really
> appreciate it Thomas if you could run it through the paces if you can
> find any time for it, or maybe just give my regression tests a once over
> to see if you can think of any other cases that should be covered.
>
> Regards
>
> David Rowley

-- 
======================================
Thomas Mayer
Durlacher Allee 61
D-76131 Karlsruhe
Telefon: +49-721-2081661
Fax:     +49-721-72380001
Mobil:   +49-174-2152332
E-Mail:  thomas.mayer@student.kit.edu
=======================================