Skip to content

Commit 518442c

Browse files
tvondratvondra
committed
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
1 parent 7ab96cf commit 518442c

File tree

4 files changed

+100
-28
lines changed

4 files changed

+100
-28
lines changed

‎src/backend/statistics/extended_stats.c

+58-28
Original file line numberDiff line numberDiff line change
@@ -1255,6 +1255,10 @@ choose_best_statistics(List *stats, char requiredkind,
12551255
/*
12561256
* Collect attributes and expressions in remaining (unestimated)
12571257
* clauses fully covered by this statistic object.
1258+
*
1259+
* We know already estimated clauses have both clause_attnums and
1260+
* clause_exprs set to NULL. We leave the pointers NULL if already
1261+
* estimated, or we reset them to NULL after estimating the clause.
12581262
*/
12591263
for (i = 0; i < nclauses; i++)
12601264
{
@@ -1758,39 +1762,65 @@ statext_mcv_clauselist_selectivity(PlannerInfo *root, List *clauses, int varReli
17581762
/* record which clauses are simple (single column or expression) */
17591763
simple_clauses = NULL;
17601764

1761-
listidx = 0;
1765+
listidx = -1;
17621766
foreach(l, clauses)
17631767
{
1768+
/* Increment the index before we decide if to skip the clause. */
1769+
listidx++;
1770+
17641771
/*
1765-
* If the clause is not already estimated and is compatible with
1766-
* the selected statistics object (all attributes and expressions
1767-
* covered), mark it as estimated and add it to the list to
1768-
* estimate.
1772+
* Ignore clauses from which we did not extract any attnums or
1773+
* expressions (this needs to be consistent with what we do in
1774+
* choose_best_statistics).
1775+
*
1776+
* This also eliminates already estimated clauses - both those
1777+
* estimated before and during applying extended statistics.
1778+
*
1779+
* XXX This check is needed because both bms_is_subset and
1780+
* stat_covers_expressions return true for empty attnums and
1781+
* expressions.
17691782
*/
1770-
if (!bms_is_member(listidx, *estimatedclauses) &&
1771-
bms_is_subset(list_attnums[listidx], stat->keys) &&
1772-
stat_covers_expressions(stat, list_exprs[listidx], NULL))
1773-
{
1774-
/* record simple clauses (single column or expression) */
1775-
if ((list_attnums[listidx] == NULL &&
1776-
list_length(list_exprs[listidx]) == 1) ||
1777-
(list_exprs[listidx] == NIL &&
1778-
bms_membership(list_attnums[listidx]) == BMS_SINGLETON))
1779-
simple_clauses = bms_add_member(simple_clauses,
1780-
list_length(stat_clauses));
1781-
1782-
/* add clause to list and mark as estimated */
1783-
stat_clauses = lappend(stat_clauses, (Node *) lfirst(l));
1784-
*estimatedclauses = bms_add_member(*estimatedclauses, listidx);
1785-
1786-
bms_free(list_attnums[listidx]);
1787-
list_attnums[listidx] = NULL;
1788-
1789-
list_free(list_exprs[listidx]);
1790-
list_exprs[listidx] = NULL;
1791-
}
1783+
if (!list_attnums[listidx] && !list_exprs[listidx])
1784+
continue;
17921785

1793-
listidx++;
1786+
/*
1787+
* The clause was not estimated yet, and we've extracted either
1788+
* attnums of expressions from it. Ignore it if it's not fully
1789+
* covered by the chosen statistics.
1790+
*
1791+
* We need to check both attributes and expressions, and reject
1792+
* if either is not covered.
1793+
*/
1794+
if (!bms_is_subset(list_attnums[listidx], stat->keys) ||
1795+
!stat_covers_expressions(stat, list_exprs[listidx], NULL))
1796+
continue;
1797+
1798+
/*
1799+
* Now we know the clause is compatible (we have either atttnums
1800+
* or expressions extracted from it), and was not estimated yet.
1801+
*/
1802+
1803+
/* record simple clauses (single column or expression) */
1804+
if ((list_attnums[listidx] == NULL &&
1805+
list_length(list_exprs[listidx]) == 1) ||
1806+
(list_exprs[listidx] == NIL &&
1807+
bms_membership(list_attnums[listidx]) == BMS_SINGLETON))
1808+
simple_clauses = bms_add_member(simple_clauses,
1809+
list_length(stat_clauses));
1810+
1811+
/* add clause to list and mark it as estimated */
1812+
stat_clauses = lappend(stat_clauses, (Node *) lfirst(l));
1813+
*estimatedclauses = bms_add_member(*estimatedclauses, listidx);
1814+
1815+
/*
1816+
* Reset the pointers, so that choose_best_statistics knows this
1817+
* clause was estimated and does not consider it again.
1818+
*/
1819+
bms_free(list_attnums[listidx]);
1820+
list_attnums[listidx] = NULL;
1821+
1822+
list_free(list_exprs[listidx]);
1823+
list_exprs[listidx] = NULL;
17941824
}
17951825

17961826
if (is_or)

‎src/backend/statistics/mcv.c

+4
Original file line numberDiff line numberDiff line change
@@ -1575,6 +1575,8 @@ mcv_match_expression(Node *expr, Bitmapset *keys, List *exprs, Oid *collid)
15751575
(idx <= bms_num_members(keys) + list_length(exprs)));
15761576
}
15771577

1578+
Assert((idx >= 0) && (idx < bms_num_members(keys) + list_length(exprs)));
1579+
15781580
return idx;
15791581
}
15801582

@@ -1654,6 +1656,8 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
16541656
/* match the attribute/expression to a dimension of the statistic */
16551657
idx = mcv_match_expression(clause_expr, keys, exprs, &collid);
16561658

1659+
Assert((idx >= 0) && (idx < bms_num_members(keys) + list_length(exprs)));
1660+
16571661
/*
16581662
* Walk through the MCV items and evaluate the current clause. We
16591663
* can skip items that were already ruled out, and terminate if

‎src/test/regress/expected/stats_ext.out

+19
Original file line numberDiff line numberDiff line change
@@ -2938,6 +2938,25 @@ SELECT * FROM check_estimated_rows('SELECT * FROM expr_stats WHERE a = 0 AND (b
29382938
(1 row)
29392939

29402940
DROP TABLE expr_stats;
2941+
-- test handling of a mix of compatible and incompatible expressions
2942+
CREATE TABLE expr_stats_incompatible_test (
2943+
c0 double precision,
2944+
c1 boolean NOT NULL
2945+
);
2946+
CREATE STATISTICS expr_stat_comp_1 ON c0, c1 FROM expr_stats_incompatible_test;
2947+
INSERT INTO expr_stats_incompatible_test VALUES (1234,false), (5678,true);
2948+
ANALYZE expr_stats_incompatible_test;
2949+
SELECT c0 FROM ONLY expr_stats_incompatible_test WHERE
2950+
(
2951+
upper('x') LIKE ('x'||('[0,1]'::int4range))
2952+
AND
2953+
(c0 IN (0, 1) OR c1)
2954+
);
2955+
c0
2956+
----
2957+
(0 rows)
2958+
2959+
DROP TABLE expr_stats_incompatible_test;
29412960
-- Permission tests. Users should not be able to see specific data values in
29422961
-- the extended statistics, if they lack permission to see those values in
29432962
-- the underlying table.

‎src/test/regress/sql/stats_ext.sql

+19
Original file line numberDiff line numberDiff line change
@@ -1470,6 +1470,25 @@ SELECT * FROM check_estimated_rows('SELECT * FROM expr_stats WHERE a = 0 AND (b
14701470

14711471
DROP TABLE expr_stats;
14721472

1473+
-- test handling of a mix of compatible and incompatible expressions
1474+
CREATE TABLE expr_stats_incompatible_test (
1475+
c0 double precision,
1476+
c1 boolean NOT NULL
1477+
);
1478+
1479+
CREATE STATISTICS expr_stat_comp_1 ON c0, c1 FROM expr_stats_incompatible_test;
1480+
1481+
INSERT INTO expr_stats_incompatible_test VALUES (1234,false), (5678,true);
1482+
ANALYZE expr_stats_incompatible_test;
1483+
1484+
SELECT c0 FROM ONLY expr_stats_incompatible_test WHERE
1485+
(
1486+
upper('x') LIKE ('x'||('[0,1]'::int4range))
1487+
AND
1488+
(c0 IN (0, 1) OR c1)
1489+
);
1490+
1491+
DROP TABLE expr_stats_incompatible_test;
14731492

14741493
-- Permission tests. Users should not be able to see specific data values in
14751494
-- the extended statistics, if they lack permission to see those values in

0 commit comments

Comments
 (0)