On Sun, 8 Oct 2023 at 23:52, Richard Guo <guofenglinux@gmail.com> wrote:
> If the pathkeys that were added by adjust_group_pathkeys_for_groupagg()
> are computable from the targetlist, it seems that we do not need to trim
> them off, because prepare_sort_from_pathkeys() will add resjunk target
> entries for them. But it's also no harm if we trim them off. So I
> think the patch is a pretty safe fix. +1 to it.
hmm, I think one of us does not understand what is going on here. I
tried to explain in [1] why we *need* to strip off the pathkeys added
by adjust_group_pathkeys_for_groupagg().
Sorry I didn't make myself clear. I understand why we need to trim off
the pathkeys added by adjust_group_pathkeys_for_groupagg(). What I
meant was that if the new added pathkeys are *computable* from the
existing target entries, then prepare_sort_from_pathkeys() will add
resjunk target entries for them, so there seems to be no problem even if
we do not trim them off. For example
explain (verbose, costs off)
select a, count(distinct a+1) from prt1 group by a order by a;
QUERY PLAN
------------------------------------------------------------------------------------
Result
Output: prt1.a, (count(DISTINCT ((prt1.a + 1))))
-> Merge Append
Sort Key: prt1.a, ((prt1.a + 1))
-> GroupAggregate
Output: prt1.a, count(DISTINCT ((prt1.a + 1))), ((prt1.a + 1))
Group Key: prt1.a
-> Sort
Output: prt1.a, ((prt1.a + 1))
Sort Key: prt1.a, ((prt1.a + 1))
-> Seq Scan on public.prt1_p1 prt1
Output: prt1.a, (prt1.a + 1)
...
Expression 'a+1' is *computable* from the existing entry 'a', so we just
add a new resjunk target entry for 'a+1', and there is no error planning
this query. But if we change 'a+1' to something that is not computable,
then we would have problems (without your fix), and the reason has been
well explained by your messages.
explain (verbose, costs off)
select a, count(distinct b) from prt1 group by a order by a;
ERROR: could not find pathkey item to sort
Having said that, I think it's the right thing to do to trim off the new
added pathkeys, even if they are *computable*. In the plan above, the
'(prt1.a + 1)' in GroupAggregate's targetlist and MergeAppend's
pathkeys are actually redundant. It's good to remove it.
Can you explain why you think we can put a resjunk "b" in the target
list of the GroupAggregate in the above case?
Hmm, I don't think we can do that, because 'b' is not *computable* from
the existing target entries, as I explained above.
Thanks
Richard