Re: Removing Functionally Dependent GROUP BY Columns

On 12/02/2016 12:01 am, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <david(dot)rowley(at)2ndquadrant(dot)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.