On Mon, 3 Oct 2022 at 09:59, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > For the master version, I think it's safe just to get rid of
> > PlannerInfo.num_groupby_pathkeys now. I only added that so I could
> > strip off the ORDER BY / DISTINCT aggregate PathKeys from the group by
> > pathkeys before passing to the functions that rearranged the GROUP BY
> > clause.
>
> I was kind of unhappy with that data structure too, but from the
> other direction: I didn't like that you were folding aggregate-derived
> pathkeys into root->group_pathkeys in the first place. That seems like
> a kluge that might work all right for the moment but will cause problems
> down the road. (Despite the issues with the patch at hand, I don't
> think it's unreasonable to suppose that somebody will have a more
> successful go at optimizing GROUP BY sorting later.) If we keep the
> data structure like this, I think we absolutely need num_groupby_pathkeys,
> or some other way of recording which pathkeys came from what source.
Ok, I don't feel too strongly about removing num_groupby_pathkeys. I'm
fine to leave it there. However, I'll reserve slight concerns that
we'll likely receive sporadic submissions of cleanup patches that
remove the unused field over the course of the next few years and that
dealing with those might take up more time than just removing it now
and putting it back when we need it. We have been receiving quite a
few patches along those lines lately.
As for the slight misuse of group_pathkeys, I guess since there are no
users that require just the plain pathkeys belonging to the GROUP BY,
then likely the best thing would be just to rename that field to
something like groupagg_pathkeys. Maintaining two separate fields and
concatenating them every time we want group_pathkeys does not seem
that appealing to me. Seems like a waste of memory and effort. I don't
want to hi-jack this thread to discuss that, but if you have a
preferred course of action, then I'm happy to kick off a discussion on
a new thread.
David
