Fix handling of clauses incompatible with extended statistics

Handling of incompatible clauses while applying extended statistics was
a bit confused - while handling a mix of compatible and incompatible
clauses it sometimes incorrectly treated the incompatible clauses as
compatible, resulting in a crash.

Fixed by reworking the code applying the selected statistics object to
make it easier to understand, and adding a proper compatibility check.

Reported-by: David Rowley
Discussion: https://postgr.es/m/CAApHDvpYT10-nkSp8xXe-nbO3jmoaRyRFHbzh-RWMfAJynqgpQ%40mail.gmail.com

diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index dd3c84a91c0413fcfb4a1e008678cc959fa1a7f7..463d44a68a40f8138151d3169a2ebed23158749a 100644 (file)
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -1255,6 +1255,10 @@ choose_best_statistics(List *stats, char requiredkind,
                /*
                 * Collect attributes and expressions in remaining (unestimated)
                 * clauses fully covered by this statistic object.
+                *
+                * We know already estimated clauses have both clause_attnums and
+                * clause_exprs set to NULL. We leave the pointers NULL if already
+                * estimated, or we reset them to NULL after estimating the clause.
                 */
                for (i = 0; i < nclauses; i++)
                {
@@ -1758,39 +1762,65 @@ statext_mcv_clauselist_selectivity(PlannerInfo *root, List *clauses, int varReli
                /* record which clauses are simple (single column or expression) */
                simple_clauses = NULL;
 
-               listidx = 0;
+               listidx = -1;
                foreach(l, clauses)
                {
+                       /* Increment the index before we decide if to skip the clause. */
+                       listidx++;
+
                        /*
-                        * If the clause is not already estimated and is compatible with
-                        * the selected statistics object (all attributes and expressions
-                        * covered), mark it as estimated and add it to the list to
-                        * estimate.
+                        * Ignore clauses from which we did not extract any attnums or
+                        * expressions (this needs to be consistent with what we do in
+                        * choose_best_statistics).
+                        *
+                        * This also eliminates already estimated clauses - both those
+                        * estimated before and during applying extended statistics.
+                        *
+                        * XXX This check is needed because both bms_is_subset and
+                        * stat_covers_expressions return true for empty attnums and
+                        * expressions.
                         */
-                       if (!bms_is_member(listidx, *estimatedclauses) &&
-                               bms_is_subset(list_attnums[listidx], stat->keys) &&
-                               stat_covers_expressions(stat, list_exprs[listidx], NULL))
-                       {
-                               /* record simple clauses (single column or expression) */
-                               if ((list_attnums[listidx] == NULL &&
-                                        list_length(list_exprs[listidx]) == 1) ||
-                                       (list_exprs[listidx] == NIL &&
-                                        bms_membership(list_attnums[listidx]) == BMS_SINGLETON))
-                                       simple_clauses = bms_add_member(simple_clauses,
-                                                                                                       list_length(stat_clauses));
-
-                               /* add clause to list and mark as estimated */
-                               stat_clauses = lappend(stat_clauses, (Node *) lfirst(l));
-                               *estimatedclauses = bms_add_member(*estimatedclauses, listidx);
-
-                               bms_free(list_attnums[listidx]);
-                               list_attnums[listidx] = NULL;
-
-                               list_free(list_exprs[listidx]);
-                               list_exprs[listidx] = NULL;
-                       }
+                       if (!list_attnums[listidx] && !list_exprs[listidx])
+                               continue;
 
-                       listidx++;
+                       /*
+                        * The clause was not estimated yet, and we've extracted either
+                        * attnums of expressions from it. Ignore it if it's not fully
+                        * covered by the chosen statistics.
+                        *
+                        * We need to check both attributes and expressions, and reject
+                        * if either is not covered.
+                        */
+                       if (!bms_is_subset(list_attnums[listidx], stat->keys) ||
+                               !stat_covers_expressions(stat, list_exprs[listidx], NULL))
+                               continue;
+
+                       /*
+                        * Now we know the clause is compatible (we have either atttnums
+                        * or expressions extracted from it), and was not estimated yet.
+                        */
+
+                       /* record simple clauses (single column or expression) */
+                       if ((list_attnums[listidx] == NULL &&
+                                list_length(list_exprs[listidx]) == 1) ||
+                               (list_exprs[listidx] == NIL &&
+                                bms_membership(list_attnums[listidx]) == BMS_SINGLETON))
+                               simple_clauses = bms_add_member(simple_clauses,
+                                                                                               list_length(stat_clauses));
+
+                       /* add clause to list and mark it as estimated */
+                       stat_clauses = lappend(stat_clauses, (Node *) lfirst(l));
+                       *estimatedclauses = bms_add_member(*estimatedclauses, listidx);
+
+                       /*
+                        * Reset the pointers, so that choose_best_statistics knows this
+                        * clause was estimated and does not consider it again.
+                        */
+                       bms_free(list_attnums[listidx]);
+                       list_attnums[listidx] = NULL;
+
+                       list_free(list_exprs[listidx]);
+                       list_exprs[listidx] = NULL;
                }
 
                if (is_or)

diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index 2a00fb48483f477fb328b44c1f1b39fe34e6e11f..9ab3e81a91d7742cc83043e760d8bd6d3d8f6ab0 100644 (file)
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -1575,6 +1575,8 @@ mcv_match_expression(Node *expr, Bitmapset *keys, List *exprs, Oid *collid)
                           (idx <= bms_num_members(keys) + list_length(exprs)));
        }
 
+       Assert((idx >= 0) && (idx < bms_num_members(keys) + list_length(exprs)));
+
        return idx;
 }
 
@@ -1654,6 +1656,8 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
                        /* match the attribute/expression to a dimension of the statistic */
                        idx = mcv_match_expression(clause_expr, keys, exprs, &collid);
 
+                       Assert((idx >= 0) && (idx < bms_num_members(keys) + list_length(exprs)));
+
                        /*
                         * Walk through the MCV items and evaluate the current clause. We
                         * can skip items that were already ruled out, and terminate if

diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index 679fd2d64d483ddde3579940a2bbebfeebb4ac5a..8c214d8dfc557a3c134cdfe14d12e021b622048e 100644 (file)
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -2938,6 +2938,25 @@ SELECT * FROM check_estimated_rows('SELECT * FROM expr_stats WHERE a = 0 AND (b
 (1 row)
 
 DROP TABLE expr_stats;
+-- test handling of a mix of compatible and incompatible expressions
+CREATE TABLE expr_stats_incompatible_test (
+    c0 double precision,
+    c1 boolean NOT NULL
+);
+CREATE STATISTICS expr_stat_comp_1 ON c0, c1 FROM expr_stats_incompatible_test;
+INSERT INTO expr_stats_incompatible_test VALUES (1234,false), (5678,true);
+ANALYZE expr_stats_incompatible_test;
+SELECT c0 FROM ONLY expr_stats_incompatible_test WHERE
+(
+  upper('x') LIKE ('x'||('[0,1]'::int4range))
+  AND
+  (c0 IN (0, 1) OR c1)
+);
+ c0 
+----
+(0 rows)
+
+DROP TABLE expr_stats_incompatible_test;
 -- Permission tests. Users should not be able to see specific data values in
 -- the extended statistics, if they lack permission to see those values in
 -- the underlying table.

diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index 7e092571ca00bf7e2cf1d4935a10f6a0be0410da..e033080d4fb388c03019eb4e3c95b6093e11ef7b 100644 (file)
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -1470,6 +1470,25 @@ SELECT * FROM check_estimated_rows('SELECT * FROM expr_stats WHERE a = 0 AND (b
 
 DROP TABLE expr_stats;
 
+-- test handling of a mix of compatible and incompatible expressions
+CREATE TABLE expr_stats_incompatible_test (
+    c0 double precision,
+    c1 boolean NOT NULL
+);
+
+CREATE STATISTICS expr_stat_comp_1 ON c0, c1 FROM expr_stats_incompatible_test;
+
+INSERT INTO expr_stats_incompatible_test VALUES (1234,false), (5678,true);
+ANALYZE expr_stats_incompatible_test;
+
+SELECT c0 FROM ONLY expr_stats_incompatible_test WHERE
+(
+  upper('x') LIKE ('x'||('[0,1]'::int4range))
+  AND
+  (c0 IN (0, 1) OR c1)
+);
+
+DROP TABLE expr_stats_incompatible_test;
 
 -- Permission tests. Users should not be able to see specific data values in
 -- the extended statistics, if they lack permission to see those values in