On 12/02/2016 12:01 am, "Tom Lane" <t...@sss.pgh.pa.us> wrote: > > David Rowley <david.row...@2ndquadrant.com> writes: > > [ prune_group_by_clause_ab4f491_2016-01-23.patch ] > > [ check_functional_grouping_refactor.patch ] > > I've committed this with mostly-cosmetic revisions (principally, rewriting > a lot of the comments, which seemed on the sloppy side).
Many thanks for committing this and improving the comments. > >> * Both of the loops iterating over the groupClause neglect to check > >> varlevelsup, thus leading to assert failures or worse if an outer-level > >> Var is present in the GROUP BY list. (I'm pretty sure outer Vars can > >> still be present at this point, though I might be wrong.) > > > Fixed in the first loop, and the way I've rewritten the code to use > > bms_difference, there's no need to check again in the 2nd loop. > > Um, AFAICS, you *do* need to check again in the second loop, else you'll > be accessing a surplusvars[] entry that might not exist at all, and in > any case might falsely tell you that you can exclude the outer var from > the new GROUP BY list. > I can't quite understand what you're seeing here. As far as I can tell from reading the code again (on my phone ) the varlevelsup check in the 2nd loop is not required. Any var with varlevelsup above zero won't make it into the surplus bitmap for the release as the bms_difference call just removes the pk vars from the varlevelsup =0 vars. So the bms_ismember should be fine. I can't see what I'm missing. In either case it test does no harm. > That was the only actual bug I found, though I changed some other stuff.
