Subject: Re: pg16: XX000: could not find pathkey item to sort

From: Richard Guo <[email-masked]>
Date: Tue, 19 Sep 2023 18:36:11 +0800

On Mon, Sep 18, 2023 at 10:02 PM Justin Pryzby <[email-masked]> wrote:

> This fails since 1349d2790b
>
> commit 1349d2790bf48a4de072931c722f39337e72055e
> Author: David Rowley <[email-masked]>
> Date:   Tue Aug 2 23:11:45 2022 +1200
>
>     Improve performance of ORDER BY / DISTINCT aggregates
>
> ts=# CREATE TABLE t (a int, b text) PARTITION BY RANGE (a);
> ts=# CREATE TABLE td PARTITION OF t DEFAULT;
> ts=# INSERT INTO t SELECT 1 AS a, '' AS b;
> ts=# SET enable_partitionwise_aggregate=on;
> ts=# explain SELECT a, COUNT(DISTINCT b) FROM t GROUP BY a;
> ERROR:  XX000: could not find pathkey item to sort
> LOCATION:  prepare_sort_from_pathkeys, createplan.c:6235

Thanks for the report!  I've looked at it a little bit.  In function
adjust_group_pathkeys_for_groupagg we add the pathkeys in ordered
aggregates to root->group_pathkeys.  But if the new added pathkeys do
not have EC members that match the targetlist or can be computed from
the targetlist, prepare_sort_from_pathkeys would have problem computing
sort column info for the new added pathkeys.  In the given example, the
pathkey representing 'b' can not match or be computed from the current
targetlist, so prepare_sort_from_pathkeys emits the error.

My first thought about the fix is that we artificially add resjunk
target entries to parse->targetList for the ordered aggregates'
arguments that are ORDER BY expressions, as attached.  While this can
fix the given query, it would cause Assert failure for the query in
sql/triggers.sql.

-- inserts only
insert into my_table values (1, 'AAA'), (2, 'BBB')
  on conflict (a) do
  update set b = my_table.b || ':' || excluded.b;

I haven't looked into how that happens.

Any thoughts?

Thanks
Richard

---

From: David Rowley <[email-masked]>
Date: Tue, 3 Oct 2023 09:11:43 +1300

> Any thoughts?

Unfortunately, we can't do that as it'll lead to target entries
existing in the GroupAggregate's target list that have been
aggregated.

postgres=# explain verbose SELECT a, COUNT(DISTINCT b) FROM rp GROUP BY a;
                                      QUERY PLAN
--------------------------------------------------------------------------------------
 Append  (cost=88.17..201.39 rows=400 width=44)
   ->  GroupAggregate  (cost=88.17..99.70 rows=200 width=44)
         Output: rp.a, count(DISTINCT rp.b), rp.b

Your patch adds rp.b as an output column of the GroupAggregate.
Logically, that column cannot exist there as there is no correct
single value of rp.b after aggregation.

I think the fix needs to go into create_agg_path().  The problem is
that for AGG_SORTED we do:

if (aggstrategy == AGG_SORTED)
    pathnode->path.pathkeys = subpath->pathkeys; /* preserves order */

which assumes that all of the columns before the aggregate will be
available after the aggregate.  That likely used to work ok before
1349d2790 as the planner wouldn't have requested any Pathkeys for
columns that were not available below the Agg node.

We can no longer take the subpath pathkey's verbatim. We need to strip
off pathkeys for columns that are not in pathnode's targetlist.

I've attached a patch which adds a new function to pathkeys.c to strip
off any PathKeys in a list that don't have a corresponding item in the
given PathTarget and just return a prefix of the input pathkey list up
until the first expr that can't be found.

I'm concerned that this patch will be too much overhead when creating
paths when a PathKey's EquivalenceClass has a large number of members
from partitioned tables.  I wondered if we should instead just check
if the subpath's pathkeys match root->group_pathkeys and if they do
set the AggPath's pathkeys to list_copy_head(subpath->pathkeys,
root->num_groupby_pathkeys),  that'll be much cheaper, but it just
feels a bit too much like a special case.

David