OK, attached is a sequence of WIP fixes for the issues discussed here.
1) using column-specific collations (instead of type/default ones)
The collations patch is pretty simple, but I'm not sure it actually does
the right thing, particularly during estimation where it uses collation
from the Var node (varcollid). But looking at 5e0928005, this should use
the same collation as when building the extended statistics (which we
get from the per-column stats, as stored in pg_statistic.stacoll#).
But we don't actually store collations for extended statistics, so we
can either modify pg_statistic_ext_data and store it there, or lookup
the per-column statistic info during estimation, and use that. I kinda
think the first option is the right one, but that'd mean yet another
catversion bump.
OTOH 5e0928005 actually did modify the extended statistics (mvdistinct
and dependencies) to use type->typcollation during building, so maybe we
want to use the default type collation for some reason?
2) proper extraction of Var/Const from opclauses
This is the primary issue discussed in this thread - I've renamed the
function to examine_opclause_expression() so that it kinda resembles
examine_variable() and I've moved it to the "internal" header file. We
still need it from two places so it can't be static, but hopefully this
naming is acceptable.
3) handling of NULL values (Const and MCV items)
Aside from the issue that Const may represent NULL, I've realized the
code might do the wrong thing for NULL in the MCV item itself. It did
treat it as mismatch and update the bitmap, but it might have invoke the
operator procedure anyway (depending on whether it's AND/OR clause,
what's the current value in the bitmap, etc.). This patch should fix
both issues by treating them as mismatch, and skipping the proc call.
4) refactoring of the bitmap updates
This is not a bug per se, but while working on (3) I've realized the
code updating the bitmap is quite repetitive and it does stuff like
if (is_or)
bitmap[i] = Max(bitmap[i], match)
else
bitmap[i] = Min(bitmap[i], match)
over and over on various places. This moves this into a separate static
function, which I think makes it way more readable. Also, it replaces
the Min/Max with a plain boolean operators (the patch originally used
three states, not true/false, hence the Min/Max).
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 1586963befe8fe7de473a28515bd7676fa2d0acd Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@2ndquadrant.com>
Date: Sun, 14 Jul 2019 14:19:01 +0200
Subject: [PATCH 1/5] Use proper collations in extended statistics
The extended statistics code was a bit confused about which collation
to use when building the statistics and when computing the estimates.
For building it used a default collation for each data type, while for
estimation it used DEFAULT_COLLATION_OID. That's clearly inconsistent.
Commit 5e0928005 changed how this works for per-column statistics, in
which case we now use collation specified for each column - both for
building the statistics and selectivity estimation. This commit adopts
the same approach for extended statistics.
Note: One issue is that for per-column statistics we store collation
in pg_statistic catalog, but we don't store this in pg_statistic_ext.
So we'd have to either add another column into the catalog (which is
probably the right thing to do) or rely on info from pg_statistic.
But we probably need to add this into pg_statistic_ext, to allow stats
on expressions, or extended statistics with different collations.
---
src/backend/statistics/mcv.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index 913a72ff67..2e375edcb4 100644
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -348,7 +348,7 @@ build_mss(VacAttrStats **stats, int numattrs)
elog(ERROR, "cache lookup failed for ordering operator
for type %u",
colstat->attrtypid);
- multi_sort_add_dimension(mss, i, type->lt_opr,
type->typcollation);
+ multi_sort_add_dimension(mss, i, type->lt_opr,
colstat->attrcollid);
}
return mss;
@@ -668,7 +668,7 @@ statext_mcv_serialize(MCVList *mcvlist, VacAttrStats
**stats)
/* sort and deduplicate the data */
ssup[dim].ssup_cxt = CurrentMemoryContext;
- ssup[dim].ssup_collation = DEFAULT_COLLATION_OID;
+ ssup[dim].ssup_collation = stats[dim]->attrcollid;
ssup[dim].ssup_nulls_first = false;
PrepareSortSupportFromOrderingOp(typentry->lt_opr, &ssup[dim]);
@@ -1577,8 +1577,6 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
if (ok)
{
- TypeCacheEntry *typecache;
- FmgrInfo gtproc;
Var *var;
Const *cst;
bool isgt;
@@ -1596,10 +1594,6 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
/* match the attribute to a dimension of the
statistic */
idx = bms_member_index(keys, var->varattno);
- /* get information about the >= procedure */
- typecache = lookup_type_cache(var->vartype,
TYPECACHE_GT_OPR);
- fmgr_info(get_opcode(typecache->gt_opr),
>proc);
-
/*
* Walk through the MCV items and evaluate the
current clause.
* We can skip items that were already ruled
out, and
@@ -1636,7 +1630,7 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
* or (const op var).
*/
mismatch =
!DatumGetBool(FunctionCall2Coll(&opproc,
-
DEFAULT_COLLATION_OID,
+
var->varcollid,
cst->constvalue,
item->values[idx]));
@@ -1654,12 +1648,12 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
*/
if (isgt)
mismatch =
!DatumGetBool(FunctionCall2Coll(&opproc,
-
DEFAULT_COLLATION_OID,
+
var->varcollid,
cst->constvalue,
item->values[idx]));
else
mismatch =
!DatumGetBool(FunctionCall2Coll(&opproc,
-
DEFAULT_COLLATION_OID,
+
var->varcollid,
item->values[idx],
cst->constvalue));
--
2.20.1
>From ea6acd6285bf697a3a838c6df163aa4a3466a769 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@2ndquadrant.com>
Date: Sat, 13 Jul 2019 00:12:16 +0200
Subject: [PATCH 2/5] Fix handling of opclauses in extended statistics
We expect opclauses to have exactly one Var and one Const, but the code
was checking the Const by calling is_pseudo_constant_clause() which is
incorrect - we need a proper constant.
Fixed by using plain IsA(x,Const) to check type of the node. We need to
do these checks in two places, so move it into a separate function that
can be called in both places.
Reported by Andreas Seltenreich, based on crash reported by sqlsmith.
Discussion: https://postgr.es/m/8736jdhbhc.fsf%40ansel.ydns.eu
---
src/backend/statistics/extended_stats.c | 74 ++++++++++++++++---
src/backend/statistics/mcv.c | 27 ++-----
.../statistics/extended_stats_internal.h | 2 +
3 files changed, 71 insertions(+), 32 deletions(-)
diff --git a/src/backend/statistics/extended_stats.c
b/src/backend/statistics/extended_stats.c
index c56ed48270..08032f1b28 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -796,21 +796,13 @@ statext_is_compatible_clause_internal(PlannerInfo *root,
Node *clause,
RangeTblEntry *rte = root->simple_rte_array[relid];
OpExpr *expr = (OpExpr *) clause;
Var *var;
- bool varonleft = true;
- bool ok;
/* Only expressions with two arguments are considered
compatible. */
if (list_length(expr->args) != 2)
return false;
- /* see if it actually has the right shape (one Var, one Const)
*/
- ok = (NumRelids((Node *) expr) == 1) &&
- (is_pseudo_constant_clause(lsecond(expr->args)) ||
- (varonleft = false,
- is_pseudo_constant_clause(linitial(expr->args))));
-
- /* unsupported structure (two variables or so) */
- if (!ok)
+ /* Check if the expression the right shape (one Var, one Const)
*/
+ if (!examine_opclause_expression(expr, &var, NULL, NULL))
return false;
/*
@@ -850,8 +842,6 @@ statext_is_compatible_clause_internal(PlannerInfo *root,
Node *clause,
!get_func_leakproof(get_opcode(expr->opno)))
return false;
- var = (varonleft) ? linitial(expr->args) : lsecond(expr->args);
-
return statext_is_compatible_clause_internal(root, (Node *) var,
relid, attnums);
}
@@ -1196,3 +1186,63 @@ statext_clauselist_selectivity(PlannerInfo *root, List
*clauses, int varRelid,
return sel;
}
+
+/*
+ * examine_operator_expression
+ * Split expression into Var and Const parts.
+ *
+ * Attempts to match the arguments to either (Var op Const) or (Const op Var),
+ * possibly with a RelabelType on top of the Var node. When successful, returns
+ * true, otherwise returns false.
+ *
+ * Optionally returns pointers to the extracted elements, when passed non-null
+ * pointers (varp, cstp and isgtp).
+ */
+bool
+examine_opclause_expression(OpExpr *expr, Var **varp, Const **cstp, bool
*isgtp)
+{
+ Var *var;
+ Const *cst;
+ bool isgt;
+ Node *leftop,
+ *rightop;
+
+ /* enforced by statext_is_compatible_clause_internal */
+ Assert(list_length(expr->args) == 2);
+
+ leftop = linitial(expr->args);
+ rightop = lsecond(expr->args);
+
+ /* strip RelabelType from either side of the expression */
+ if (IsA(leftop, RelabelType))
+ leftop = (Node *) ((RelabelType *) leftop)->arg;
+
+ if (IsA(rightop, RelabelType))
+ rightop = (Node *) ((RelabelType *) rightop)->arg;
+
+ if (IsA(leftop, Var) && IsA(rightop, Const))
+ {
+ var = (Var *) leftop;
+ cst = (Const *) rightop;
+ isgt = false;
+ }
+ else if (IsA(leftop, Const) && IsA(rightop, Var))
+ {
+ var = (Var *) rightop;
+ cst = (Const *) leftop;
+ isgt = true;
+ }
+ else
+ return false;
+
+ if (varp)
+ *varp = var;
+
+ if (cstp)
+ *cstp = cst;
+
+ if (isgtp)
+ *isgtp = isgt;
+
+ return true;
+}
diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index 2e375edcb4..865981bbb4 100644
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -1561,36 +1561,23 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
if (is_opclause(clause))
{
OpExpr *expr = (OpExpr *) clause;
- bool varonleft = true;
- bool ok;
FmgrInfo opproc;
/* get procedure computing operator selectivity */
RegProcedure oprrest = get_oprrest(expr->opno);
- fmgr_info(get_opcode(expr->opno), &opproc);
+ /* valid only after examine_opclause_expression returns
true */
+ Var *var;
+ Const *cst;
+ bool isgt;
- ok = (NumRelids(clause) == 1) &&
- (is_pseudo_constant_clause(lsecond(expr->args))
||
- (varonleft = false,
-
is_pseudo_constant_clause(linitial(expr->args))));
+ fmgr_info(get_opcode(expr->opno), &opproc);
- if (ok)
+ /* extract the var and const from the expression */
+ if (examine_opclause_expression(expr, &var, &cst,
&isgt))
{
- Var *var;
- Const *cst;
- bool isgt;
int idx;
- /* extract the var and const from the
expression */
- var = (varonleft) ? linitial(expr->args) :
lsecond(expr->args);
- cst = (varonleft) ? lsecond(expr->args) :
linitial(expr->args);
- isgt = (!varonleft);
-
- /* strip binary-compatible relabeling */
- if (IsA(var, RelabelType))
- var = (Var *) ((RelabelType *)
var)->arg;
-
/* match the attribute to a dimension of the
statistic */
idx = bms_member_index(keys, var->varattno);
diff --git a/src/include/statistics/extended_stats_internal.h
b/src/include/statistics/extended_stats_internal.h
index 8fc541993f..c7f01d4edc 100644
--- a/src/include/statistics/extended_stats_internal.h
+++ b/src/include/statistics/extended_stats_internal.h
@@ -97,6 +97,8 @@ extern SortItem *build_sorted_items(int numrows, int *nitems,
HeapTuple *rows,
TupleDesc tdesc, MultiSortSupport mss,
int
numattrs, AttrNumber *attnums);
+extern bool examine_opclause_expression(OpExpr *expr, Var **varp,
+
Const **cstp, bool *isgtp);
extern Selectivity mcv_clauselist_selectivity(PlannerInfo *root,
StatisticExtInfo *stat,
--
2.20.1
>From f05c0b05d3760ff5295518dcac049ddd810a611e Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@2ndquadrant.com>
Date: Mon, 15 Jul 2019 02:00:31 +0200
Subject: [PATCH 3/5] Fix handling of NULLs in MCV items and constants
There were two issues in how the extended statistics handled NULL values
in opclauses. Firstly, the code was oblivious to the possibility that
Const may be NULL (constisnull=true) in which case the constvalue is
undefined. We need to treat this as a mismatch, and not call the proc.
Secondly, the MCV item itself may contain NULL values too - the code
already did check that, and updated the match bitmap accordingly, but
failed to ensure we won't call the operator procedure anyway.
This fixes both issues by extending the existing check so that it looks
at constisnull too, and making sure it skips calling the procedure.
Discussion: https://postgr.es/m/8736jdhbhc.fsf%40ansel.ydns.eu
---
src/backend/statistics/mcv.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index 865981bbb4..77daa647e2 100644
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -1593,12 +1593,18 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
MCVItem *item = &mcvlist->items[i];
/*
- * For AND-lists, we can also mark NULL
items as 'no
- * match' (and then skip them). For
OR-lists this is not
- * possible.
+ * When the MCV item or the Const value
is NULL we can treat
+ * this as a mismatch. We must not call
the operator proc
+ * because of strictness.
*/
- if ((!is_or) && item->isnull[idx])
- matches[i] = false;
+ if (item->isnull[idx] ||
cst->constisnull)
+ {
+ /* we only care about AND,
because OR can't change */
+ if (!is_or)
+ matches[i] = false;
+
+ continue;
+ }
/* skip MCV items that were already
ruled out */
if ((!is_or) && (matches[i] == false))
--
2.20.1
>From 73619159fc4886dc35ecd933f1f54f695e4d61e5 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@2ndquadrant.com>
Date: Mon, 15 Jul 2019 02:30:50 +0200
Subject: [PATCH 4/5] Refactor / simplify evaluation of MCV bitmap
---
src/backend/statistics/mcv.c | 145 ++++++++++++++++-------------------
1 file changed, 65 insertions(+), 80 deletions(-)
diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index 77daa647e2..9697f3d101 100644
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -1504,6 +1504,37 @@ pg_mcv_list_send(PG_FUNCTION_ARGS)
return byteasend(fcinfo);
}
+/*
+ * mcv_result_merge
+ * Compute new value in result bitmap for AND/OR clauses.
+ *
+ * Compute new value for bitmap item, considering whether it's used for
+ * clauses connected by AND/OR.
+ */
+static bool
+mcv_result_merge(bool value, bool is_or, bool match)
+{
+ return (is_or) ? (value || match) : (value && match);
+}
+
+/*
+ * mcv_result_is_final
+ * Decide if the current bitmap value may change.
+ *
+ * When processing a list of clauses, the bitmap item may get set to a value
+ * such that additional clauses can't change it. For example, when processing
+ * a list of clauses connected to AND, as soon as the item gets set to 'false'
+ * then it'll remain like that. Similarly clauses connected by OR and 'true'.
+ *
+ * Returns true when the value in the bitmap can't change no matter how the
+ * remaining clauses are evaluated.
+ */
+static bool
+mcv_result_is_final(bool value, bool is_or)
+{
+ return (is_or) ? value : (!value);
+}
+
/*
* mcv_get_match_bitmap
* Evaluate clauses using the MCV list, and update the match bitmap.
@@ -1589,27 +1620,27 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
*/
for (i = 0; i < mcvlist->nitems; i++)
{
- bool mismatch = false;
+ bool match = true;
MCVItem *item = &mcvlist->items[i];
/*
- * When the MCV item or the Const value
is NULL we can treat
- * this as a mismatch. We must not call
the operator proc
- * because of strictness.
+ * Handle NULL constants and NULL
values in the MCV item by
+ * simply considering this to be a
mismatch.
+ *
+ * We must not call the opproc, because
it might be strict.
*/
- if (item->isnull[idx] ||
cst->constisnull)
+ if (cst->constisnull ||
item->isnull[idx])
{
- /* we only care about AND,
because OR can't change */
- if (!is_or)
- matches[i] = false;
-
+ matches[i] =
mcv_result_merge(matches[i], is_or, false);
continue;
}
- /* skip MCV items that were already
ruled out */
- if ((!is_or) && (matches[i] == false))
- continue;
- else if (is_or && (matches[i] == true))
+ /*
+ * Skip MCV items that can't change
result in the bitmap.
+ * Once the value gets false for
AND-lists, or true for
+ * OR-lists, we don't need to look at
more clauses.
+ */
+ if (mcv_result_is_final(matches[i],
is_or))
continue;
switch (oprrest)
@@ -1622,10 +1653,10 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
* it does not matter
whether it's (var op const)
* or (const op var).
*/
- mismatch =
!DatumGetBool(FunctionCall2Coll(&opproc,
-
var->varcollid,
-
cst->constvalue,
-
item->values[idx]));
+ match =
DatumGetBool(FunctionCall2Coll(&opproc,
+
var->varcollid,
+
cst->constvalue,
+
item->values[idx]));
break;
@@ -1640,39 +1671,21 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
* bucket, because
there's no overlap).
*/
if (isgt)
- mismatch =
!DatumGetBool(FunctionCall2Coll(&opproc,
-
var->varcollid,
-
cst->constvalue,
-
item->values[idx]));
+ match =
DatumGetBool(FunctionCall2Coll(&opproc,
+
var->varcollid,
+
cst->constvalue,
+
item->values[idx]));
else
- mismatch =
!DatumGetBool(FunctionCall2Coll(&opproc,
-
var->varcollid,
-
item->values[idx],
-
cst->constvalue));
+ match =
DatumGetBool(FunctionCall2Coll(&opproc,
+
var->varcollid,
+
item->values[idx],
+
cst->constvalue));
break;
}
- /*
- * XXX The conditions on matches[i] are
not needed, as we
- * skip MCV items that can't become
true/false, depending
- * on the current flag. See beginning
of the loop over MCV
- * items.
- */
-
- if ((is_or) && (!mismatch))
- {
- /* OR - was not a match before,
matches now */
- matches[i] = true;
- continue;
- }
- else if ((!is_or) && mismatch)
- {
- /* AND - was a match before,
does not match anymore */
- matches[i] = false;
- continue;
- }
-
+ /* update the match bitmap with the
result */
+ matches[i] =
mcv_result_merge(matches[i], is_or, match);
}
}
}
@@ -1707,10 +1720,7 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
}
/* now, update the match bitmap, depending on
OR/AND type */
- if (is_or)
- matches[i] = Max(matches[i], match);
- else
- matches[i] = Min(matches[i], match);
+ matches[i] = mcv_result_merge(matches[i],
is_or, match);
}
}
else if (is_orclause(clause) || is_andclause(clause))
@@ -1737,13 +1747,7 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
* condition when merging the results.
*/
for (i = 0; i < mcvlist->nitems; i++)
- {
- /* Is this OR or AND clause? */
- if (is_or)
- matches[i] = Max(matches[i],
bool_matches[i]);
- else
- matches[i] = Min(matches[i],
bool_matches[i]);
- }
+ matches[i] = mcv_result_merge(matches[i],
is_or, bool_matches[i]);
pfree(bool_matches);
}
@@ -1767,25 +1771,11 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
/*
* Merge the bitmap produced by mcv_get_match_bitmap
into the
- * current one.
+ * current one. We're handling a NOT clause, so invert
the result
+ * before merging it into the global bitmap.
*/
for (i = 0; i < mcvlist->nitems; i++)
- {
- /*
- * When handling a NOT clause, we need to
invert the result
- * before merging it into the global result.
- */
- if (not_matches[i] == false)
- not_matches[i] = true;
- else
- not_matches[i] = false;
-
- /* Is this OR or AND clause? */
- if (is_or)
- matches[i] = Max(matches[i],
not_matches[i]);
- else
- matches[i] = Min(matches[i],
not_matches[i]);
- }
+ matches[i] = mcv_result_merge(matches[i],
is_or, !not_matches[i]);
pfree(not_matches);
}
@@ -1814,17 +1804,12 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
if (!item->isnull[idx] &&
DatumGetBool(item->values[idx]))
match = true;
- /* now, update the match bitmap, depending on
OR/AND type */
- if (is_or)
- matches[i] = Max(matches[i], match);
- else
- matches[i] = Min(matches[i], match);
+ /* update the result bitmap */
+ matches[i] = mcv_result_merge(matches[i],
is_or, match);
}
}
else
- {
elog(ERROR, "unknown clause type: %d", clause->type);
- }
}
return matches;
--
2.20.1
