Fix cost_incremental_sort for expressions with varno 0

When estimating the number of pre-sorted groups in cost_incremental_sort
we must not pass Vars with varno 0 to estimate_num_groups, which would
cause failues in find_base_rel. This may happen when sorting output of
set operations, thanks to generate_append_tlist.

Unlike recurse_set_operations we can't easily access the original target
list, so if we find any Vars with varno 0, we fall back to the default
estimate DEFAULT_NUM_DISTINCT.

Reported-by: Justin Pryzby
Discussion: https://postgr.es/m/20200411214639.GK2228%40telsasoft.com

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 0eef5d7707e37c603c14a01ac2e979c454621823..f5f9bae1a209102b0a9a3c917a651aa4d1b0d365 100644 (file)
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -1804,6 +1804,7 @@ cost_incremental_sort(Path *path,
    List       *presortedExprs = NIL;
    ListCell   *l;
    int         i = 0;
+   bool        unknown_varno = false;
 
    Assert(presorted_keys != 0);
 
@@ -1814,13 +1815,58 @@ cost_incremental_sort(Path *path,
    if (input_tuples < 2.0)
        input_tuples = 2.0;
 
-   /* Extract presorted keys as list of expressions */
+   /* Default estimate of number of groups, capped to one group per row. */
+   input_groups = Min(input_tuples, DEFAULT_NUM_DISTINCT);
+
+   /*
+    * Extract presorted keys as list of expressions.
+    *
+    * We need to be careful about Vars containing "varno 0" which might
+    * have been introduced by generate_append_tlist, which would confuse
+    * estimate_num_groups (in fact it'd fail for such expressions). See
+    * recurse_set_operations which has to deal with the same issue.
+    *
+    * Unlike recurse_set_operations we can't access the original target
+    * list here, and even if we could it's not very clear how useful would
+    * that be for a set operation combining multiple tables. So we simply
+    * detect if there are any expressions with "varno 0" and use the
+    * default DEFAULT_NUM_DISTINCT in that case.
+    *
+    * We might also use either 1.0 (a single group) or input_tuples (each
+    * row being a separate group), pretty much the worst and best case for
+    * incremental sort. But those are extreme cases and using something in
+    * between seems reasonable. Furthermore, generate_append_tlist is used
+    * for set operations, which are likely to produce mostly unique output
+    * anyway - from that standpoint the DEFAULT_NUM_DISTINCT is defensive
+    * while maintaining lower startup cost.
+    */
    foreach(l, pathkeys)
    {
+       Node       *expr;
+       Relids      varnos;
+
        PathKey    *key = (PathKey *) lfirst(l);
        EquivalenceMember *member = (EquivalenceMember *)
        linitial(key->pk_eclass->ec_members);
 
+       /*
+        * Check if the expression contains Var with "varno 0" so that we
+        * don't call estimate_num_groups in that case.
+        */
+       expr = (Node *) member->em_expr;
+
+       if (IsA(expr, RelabelType))
+           expr = (Node *) ((RelabelType *) expr)->arg;
+
+       varnos = pull_varnos(expr);
+
+       if (bms_is_member(0, varnos))
+       {
+           unknown_varno = true;
+           break;
+       }
+
+       /* expression not containing any Vars with "varno 0" */
        presortedExprs = lappend(presortedExprs, member->em_expr);
 
        i++;
@@ -1828,8 +1874,10 @@ cost_incremental_sort(Path *path,
            break;
    }
 
-   /* Estimate number of groups with equal presorted keys */
-   input_groups = estimate_num_groups(root, presortedExprs, input_tuples, NULL);
+   /* Estimate number of groups with equal presorted keys. */
+   if (!unknown_varno)
+       input_groups = estimate_num_groups(root, presortedExprs, input_tuples, NULL);
+
    group_tuples = input_tuples / input_groups;
    group_input_run_cost = input_run_cost / input_groups;
 

diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out
index 238d89a206ecb2cba50a36d094d04c0d5f12df93..21d2d8b5fd80bada09dd2843d7e11d66aad3fa3c 100644 (file)
--- a/src/test/regress/expected/incremental_sort.out
+++ b/src/test/regress/expected/incremental_sort.out
@@ -1447,4 +1447,24 @@ explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1
                            ->  Parallel Index Scan using t_a_idx on t
 (12 rows)
 
+-- Incremental sort vs. set operations with varno 0
+set enable_hashagg to off;
+explain (costs off) select * from t union select * from t order by 1,3;
+                        QUERY PLAN                        
+----------------------------------------------------------
+ Incremental Sort
+   Sort Key: t.a, t.c
+   Presorted Key: t.a
+   ->  Unique
+         ->  Sort
+               Sort Key: t.a, t.b, t.c
+               ->  Append
+                     ->  Gather
+                           Workers Planned: 2
+                           ->  Parallel Seq Scan on t
+                     ->  Gather
+                           Workers Planned: 2
+                           ->  Parallel Seq Scan on t t_1
+(13 rows)
+
 drop table t;

diff --git a/src/test/regress/sql/incremental_sort.sql b/src/test/regress/sql/incremental_sort.sql
index 2241cc9c025c9c32027fe46598cec3ba145ed6aa..fa2e320e9ae50d4cc818418227c6e8d8f2cafc87 100644 (file)
--- a/src/test/regress/sql/incremental_sort.sql
+++ b/src/test/regress/sql/incremental_sort.sql
@@ -216,4 +216,8 @@ explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1
 set enable_incrementalsort = on;
 explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1;
 
+-- Incremental sort vs. set operations with varno 0
+set enable_hashagg to off;
+explain (costs off) select * from t union select * from t order by 1,3;
+
 drop table t;