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