Re: BUG #17564: Planner bug in combination of generate_series(), unnest() and ORDER BY

On Tue, Aug 2, 2022 at 4:50 PM Martijn van Oosterhout <kleptog@gmail.com> wrote:

Now it's morning I've thought of a way to reproduce it more easily, see the attached script. The tricky part is getting the tuples in a position that triggers the planner in the right way. So the script includes a list of (ctid, primary key) and creates a table using that with quite a large amount of bloat. It then creates some constraints, vacuums and runs the offending query. On my system it reproduces with 100% reliability (so far anyway).

Thanks for the report! I can reproduce it on HEAD. The part of the plan
that causes problem looks like:

->  Gather Merge
      Output: results.group_id
      Workers Planned: 1
      ->  Sort
            Output: results.group_id, ((unnest('{}'::jsonb[]) ->> 'timestamp'::text))
            Sort Key: ((unnest('{}'::jsonb[]) ->> 'timestamp'::text))
            ->  Parallel Seq Scan on public.results
                  Output: results.group_id, (unnest('{}'::jsonb[]) ->> 'timestamp'::text)
                  Filter: (results.search_id = 3336)

Please note that the expression below appears in the targetlist of
seqscan:

    unnest('{}'::jsonb[]) ->> 'timestamp'::text

The func for operator '->>' itself is not proretset, but one of its args
(the unnest func) is proretset. And that causes problem when we set up
projection info for the seqscan node.

So why does this expression appear in the targetlist of seqscan node? I
did some debug on that. Since this expression is the desired pathkey of
the query, relation_can_be_sorted_early() checks on it and finds that it
can be computed from the reltarget of rel 'results', which is true since
this expression can be computed all by itself. So it is considered as a
useful ordering for rel 'results' and generate_useful_gather_paths()
creates the Sort and then Gather Merge nodes for 'results' based on this
pathkey.

Still need more time to investigate for the fix.

In relation_can_be_sorted_early(), we try to find an EC member that
matches some reltarget or can be computed from the reltarget. If we find
out such an EC member, we check whether it involves set-returning
functions (and reject it if so) with IS_SRF_CALL, which only tests
funcretset/opretset flag of the expression, without checking further
into its args. I'm wondering if this is enough.

I'm considering a fix as checking the EC member expression recursively
with expression_returns_set(), something like below:

diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 60c0e3f108..7991295548 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -986,7 +986,7 @@ relation_can_be_sorted_early(PlannerInfo *root, RelOptInfo *rel,
                 * one are effectively checking properties of targetexpr, so there's
                 * no point in asking whether some other EC member would be better.)
                 */
-               if (IS_SRF_CALL((Node *) em->em_expr))
+               if (expression_returns_set((Node *) em->em_expr))
                        continue;

                /*
@@ -1014,7 +1014,7 @@ relation_can_be_sorted_early(PlannerInfo *root, RelOptInfo *rel,
         * member in this case; since SRFs can't appear in WHERE, they cannot
         * belong to multi-member ECs.)
         */
-       if (IS_SRF_CALL((Node *) em->em_expr))
+       if (expression_returns_set((Node *) em->em_expr))
                return false;

        return true;

Does this make any sense?

Thanks