On 4/20/22 16:15, Tom Lane wrote:
> Tomas Vondra <tomas.von...@enterprisedb.com> writes:
>> The whole idea is that instead of bailing out for non-RestrictInfo case,
>> it calculates the necessary information for the clause from scratch.
>> This means relids and pseudoconstant flag, which are checked to decide
>> if the clause is compatible with extended stats.
>
> Right.
>
>> But when inspecting how to calculate pseudoconstant, I realized that
>> maybe that's not really needed. Per distribute_qual_to_rels() we only
>> set it to 'true' when bms_is_empty(relids), and we already check that
>> relids is a singleton, so it can't be empty - which means pseudoconstant
>> can't be true either.
>
> Yeah, I would not bother with the pseudoconstant-related tests for a
> bare clause. Patch looks reasonably sane in a quick once-over otherwise,
> and the fact that it fixes the presented test case is promising.
Attached is a slightly more polished patch, adding a couple comments and
removing the unnecessary pseudoconstant checks.
> (If you set enable_indexscan = off, you can verify that the estimate
> for the number of index entries retrieved is now sane.)
I did that. Sorry for not mentioning that explicitly in my message.
> I did not look to see if there were any other RestrictInfo
> dependencies, though.
I checked the places messing with RestrictInfo in clausesel.c and
src/backend/statististics. Code in clausesel.c seems fine and mcv.c
seems fine to (it merely strips the RestrictInfo).
But dependencies.c might need a fix too, although the issue is somewhat
inverse to this one, because it looks like this:
if (IsA(clause, RestrictInfo))
{
... do some checks ...
}
so if there's no RestrictInfo on top, we just accept the clause. I guess
this should do the same thing with checking relids like the fix, but
I've been unable to construct an example demonstrating the issue (it'd
have to be either pseudoconstant or reference multiple rels, which seems
hard to get in btcostestimate).
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Companydiff --git a/src/backend/optimizer/path/clausesel.c b/src/backend/optimizer/path/clausesel.c
index 06f836308d..c3131e6c2c 100644
--- a/src/backend/optimizer/path/clausesel.c
+++ b/src/backend/optimizer/path/clausesel.c
@@ -520,12 +520,18 @@ addRangeClause(RangeQueryClause **rqlist, Node *clause,
* Examine each clause in 'clauses' and determine if all clauses
* reference only a single relation. If so return that relation,
* otherwise return NULL.
+ *
+ * The clauses may be either RestrictInfos or bare expression clauses, same as
+ * for clauselist_selectivity. The former is preferred since it allows caching
+ * of results, but some callers pass bare expressions.
+
*/
static RelOptInfo *
find_single_rel_for_clauses(PlannerInfo *root, List *clauses)
{
int lastrelid = 0;
ListCell *l;
+ Bitmapset *clause_relids;
foreach(l, clauses)
{
@@ -560,12 +566,20 @@ find_single_rel_for_clauses(PlannerInfo *root, List *clauses)
continue;
}
- if (!IsA(rinfo, RestrictInfo))
- return NULL;
+ /*
+ * For a RestrictInfo, we can use the cached relids. Otherwise
+ * inspect the clause to build the bitmap.
+ */
+ if (IsA(rinfo, RestrictInfo))
+ clause_relids = rinfo->clause_relids;
+ else
+ clause_relids = pull_varnos(root, (Node *) rinfo);
+
- if (bms_is_empty(rinfo->clause_relids))
- continue; /* we can ignore variable-free clauses */
- if (!bms_get_singleton_member(rinfo->clause_relids, &relid))
+ if (bms_is_empty(clause_relids))
+ continue; /* we can ignore variable-free clauses (this
+ * includes pseudoconstants) */
+ if (!bms_get_singleton_member(clause_relids, &relid))
return NULL; /* multiple relations in this clause */
if (lastrelid == 0)
lastrelid = relid; /* first clause referencing a relation */
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index ca48395d5c..907141dfee 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -1560,9 +1560,9 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
Bitmapset **attnums, List **exprs)
{
RangeTblEntry *rte = root->simple_rte_array[relid];
- RestrictInfo *rinfo = (RestrictInfo *) clause;
- int clause_relid;
+ Bitmapset *relids;
Oid userid;
+ int clause_relid;
/*
* Special-case handling for bare BoolExpr AND clauses, because the
@@ -1574,10 +1574,7 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
BoolExpr *expr = (BoolExpr *) clause;
ListCell *lc;
- /*
- * Check that each sub-clause is compatible. We expect these to be
- * RestrictInfos.
- */
+ /* Check that each sub-clause is compatible. */
foreach(lc, expr->args)
{
if (!statext_is_compatible_clause(root, (Node *) lfirst(lc),
@@ -1588,21 +1585,30 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
return true;
}
- /* Otherwise it must be a RestrictInfo. */
- if (!IsA(rinfo, RestrictInfo))
- return false;
+ /*
+ * If the clause has a RestrictInfo on top, use the cached results.
+ * Otherwise (if the clause is bare expression) calculate relids from
+ * scratch.
+ */
+ if (IsA(clause, RestrictInfo))
+ {
+ RestrictInfo *rinfo = (RestrictInfo *) clause;
- /* Pseudoconstants are not really interesting here. */
- if (rinfo->pseudoconstant)
- return false;
+ relids = rinfo->clause_relids;
+
+ /* extract the bare expression */
+ clause = (Node *) rinfo->clause;
+ }
+ else
+ relids = pull_varnos(root, clause);
/* Clauses referencing other varnos are incompatible. */
- if (!bms_get_singleton_member(rinfo->clause_relids, &clause_relid) ||
+ if (!bms_get_singleton_member(relids, &clause_relid) ||
clause_relid != relid)
return false;
/* Check the clause and determine what attributes it references. */
- if (!statext_is_compatible_clause_internal(root, (Node *) rinfo->clause,
+ if (!statext_is_compatible_clause_internal(root, clause,
relid, attnums, exprs))
return false;
