public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 4/5] WIP rework tracking of expressions
8+ messages / 4 participants
[nested] [flat]
* [PATCH 4/5] WIP rework tracking of expressions
@ 2021-02-17 00:02 Tomas Vondra <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Tomas Vondra @ 2021-02-17 00:02 UTC (permalink / raw)
---
src/backend/statistics/dependencies.c | 136 ++++++++++++++----
src/backend/statistics/extended_stats.c | 73 ++++------
src/backend/statistics/mcv.c | 26 +++-
src/backend/statistics/mvdistinct.c | 120 +++++++++-------
src/backend/utils/adt/selfuncs.c | 29 +++-
.../statistics/extended_stats_internal.h | 11 +-
src/include/statistics/statistics.h | 3 +-
7 files changed, 255 insertions(+), 143 deletions(-)
diff --git a/src/backend/statistics/dependencies.c b/src/backend/statistics/dependencies.c
index 6bf3127bcc..602301b724 100644
--- a/src/backend/statistics/dependencies.c
+++ b/src/backend/statistics/dependencies.c
@@ -252,7 +252,7 @@ dependency_degree(int numrows, HeapTuple *rows, ExprInfo *exprs, int k,
* member easier, and then construct a filtered version with only attnums
* referenced by the dependency we validate.
*/
- attnums = build_attnums_array(attrs, &numattrs);
+ attnums = build_attnums_array(attrs, exprs->nexprs, &numattrs);
attnums_dep = (AttrNumber *) palloc(k * sizeof(AttrNumber));
for (i = 0; i < k; i++)
@@ -372,19 +372,46 @@ statext_dependencies_build(int numrows, HeapTuple *rows,
k;
int numattrs;
AttrNumber *attnums;
+ int nattnums;
/* result */
MVDependencies *dependencies = NULL;
- /* treat expressions as special attributes with high attnums */
- attrs = add_expressions_to_attributes(attrs, exprs->nexprs);
+ /*
+ * Transform the bms into an array of attnums, to make accessing i-th
+ * member easier. We add the expressions first, represented by negative
+ * attnums (this is OK, we don't allow statistics on system attributes),
+ * and then regular attributes.
+ */
+ nattnums = bms_num_members(attrs) + exprs->nexprs;
+ attnums = (AttrNumber *) palloc(sizeof(AttrNumber) * nattnums);
+
+ numattrs = 0;
+
+ /* treat expressions as attributes with negative attnums */
+ for (i = 0; i < exprs->nexprs; i++)
+ attnums[numattrs++] = -(i+1);
/*
- * Transform the bms into an array, to make accessing i-th member easier.
+ * and then regular attributes
+ *
+ * XXX Maybe add this in the opposite order, just like in MCV? first
+ * regular attnums, then exressions.
*/
- attnums = build_attnums_array(attrs, &numattrs);
+ k = -1;
+ while ((k = bms_next_member(attrs, k)) >= 0)
+ attnums[numattrs++] = k;
Assert(numattrs >= 2);
+ Assert(numattrs == nattnums);
+
+ /*
+ * Build a new bitmapset of attnums, offset by number of expressions (this
+ * is needed, because bitmaps can store only non-negative values).
+ */
+ attrs = NULL;
+ for (i = 0; i < numattrs; i++)
+ attrs = bms_add_member(attrs, attnums[i] + exprs->nexprs);
/*
* We'll try build functional dependencies starting from the smallest ones
@@ -1374,8 +1401,10 @@ dependencies_clauselist_selectivity(PlannerInfo *root,
* We also skip clauses that we already estimated using different types of
* statistics (we treat them as incompatible).
*
- * For expressions, we generate attnums higher than MaxHeapAttributeNumber
- * so that we can work with attnums only.
+ * To handle expressions, we assign them negative attnums, as if it was a
+ * system attribute (this is fine, as we only allow extended stats on user
+ * attributes). And then we offset everything by the number of expressions,
+ * so that we can store the values in a bitmapset.
*/
listidx = 0;
foreach(l, clauses)
@@ -1391,13 +1420,12 @@ dependencies_clauselist_selectivity(PlannerInfo *root,
{
/*
* If it's a simple column refrence, just extract the attnum. If
- * it's an expression, make sure it's not a duplicate and assign
- * a special attnum to it (higher than any regular value).
+ * it's an expression, assign a negative attnum as if it was a
+ * system attribute.
*/
if (dependency_is_compatible_clause(clause, rel->relid, &attnum))
{
list_attnums[listidx] = attnum;
- clauses_attnums = bms_add_member(clauses_attnums, attnum);
}
else if (dependency_is_compatible_expression(clause, rel->relid,
rel->statlist,
@@ -1413,7 +1441,8 @@ dependencies_clauselist_selectivity(PlannerInfo *root,
{
if (equal(unique_exprs[i], expr))
{
- attnum = EXPRESSION_ATTNUM(i);
+ /* negative attribute number to expression */
+ attnum = -(i + 1);
break;
}
}
@@ -1421,14 +1450,10 @@ dependencies_clauselist_selectivity(PlannerInfo *root,
/* not found in the list, so add it */
if (attnum == InvalidAttrNumber)
{
- attnum = EXPRESSION_ATTNUM(unique_exprs_cnt);
unique_exprs[unique_exprs_cnt++] = expr;
- /* shouldn't have seen this attnum yet */
- Assert(!bms_is_member(attnum, clauses_attnums));
-
- /* we may add the attnum repeatedly to clauses_attnums */
- clauses_attnums = bms_add_member(clauses_attnums, attnum);
+ /* after incrementing the value, to get -1, -2, ... */
+ attnum = -unique_exprs_cnt;
}
/* remember which attnum was assigned to this clause */
@@ -1439,6 +1464,37 @@ dependencies_clauselist_selectivity(PlannerInfo *root,
listidx++;
}
+ Assert(listidx == list_length(clauses));
+
+ /*
+ * Now that we know how many expressions there are, we can offset the
+ * values just enough to build the bitmapset.
+ */
+ for (i = 0; i < list_length(clauses); i++)
+ {
+ AttrNumber attnum;
+
+ /* ignore incompatible or already estimated clauses */
+ if (list_attnums[i] == InvalidAttrNumber)
+ continue;
+
+ /* make sure the attnum is in the expected range */
+ Assert(list_attnums[i] >= (-unique_exprs_cnt));
+ Assert(list_attnums[i] <= MaxHeapAttributeNumber);
+
+ /* make sure the attnum is not negative */
+ attnum = list_attnums[i] + unique_exprs_cnt;
+
+ /*
+ * Expressions are unique, and so we must not have seen this attnum
+ * before.
+ */
+ Assert(AttrNumberIsForUserDefinedAttr(list_attnums[i]) ||
+ !bms_is_member(attnum, clauses_attnums));
+
+ clauses_attnums = bms_add_member(clauses_attnums, attnum);
+ }
+
/*
* If there's not at least two distinct attnums and expressions, then
* reject the whole list of clauses. We must return 1.0 so the calling
@@ -1470,19 +1526,37 @@ dependencies_clauselist_selectivity(PlannerInfo *root,
foreach(l, rel->statlist)
{
StatisticExtInfo *stat = (StatisticExtInfo *) lfirst(l);
- Bitmapset *matched;
int nmatched;
int nexprs;
+ int k;
MVDependencies *deps;
/* skip statistics that are not of the correct type */
if (stat->kind != STATS_EXT_DEPENDENCIES)
continue;
- /* count matching simple clauses */
- matched = bms_intersect(clauses_attnums, stat->keys);
- nmatched = bms_num_members(matched);
- bms_free(matched);
+ /*
+ * Count matching attributes - we have to undo two attnum offsets.
+ * First, the dependency is offset using the number of expressions
+ * for that statistics, and then (if it's a plain attribute) we
+ * need to apply the same offset as above, by unique_exprs_cnt.
+ */
+ nmatched = 0;
+ k = -1;
+ while ((k = bms_next_member(stat->keys, k)) >= 0)
+ {
+ AttrNumber attnum = (AttrNumber) k;
+
+ /* skip expressions */
+ if (!AttrNumberIsForUserDefinedAttr(attnum))
+ continue;
+
+ /* apply the same offset as above */
+ attnum += unique_exprs_cnt;
+
+ if (bms_is_member(attnum, clauses_attnums))
+ nmatched++;
+ }
/* count matching expressions */
nexprs = 0;
@@ -1537,13 +1611,23 @@ dependencies_clauselist_selectivity(PlannerInfo *root,
Node *expr;
int k;
AttrNumber unique_attnum = InvalidAttrNumber;
+ AttrNumber attnum;
- /* regular attribute, no need to remap */
- if (dep->attributes[j] <= MaxHeapAttributeNumber)
+ /* undo the per-statistics offset */
+ attnum = dep->attributes[j];
+
+ /* regular attribute, simply offset by number of expressions */
+ if (AttrNumberIsForUserDefinedAttr(attnum))
+ {
+ dep->attributes[j] = attnum + unique_exprs_cnt;
continue;
+ }
+
+ /* the attnum should be a valid system attnum (-1, -2, ...) */
+ Assert(AttributeNumberIsValid(attnum));
/* index of the expression */
- idx = EXPRESSION_INDEX(dep->attributes[j]);
+ idx = (1 - attnum);
/* make sure the expression index is valid */
Assert((idx >= 0) && (idx < list_length(stat->exprs)));
@@ -1559,7 +1643,7 @@ dependencies_clauselist_selectivity(PlannerInfo *root,
*/
if (equal(unique_exprs[k], expr))
{
- unique_attnum = EXPRESSION_ATTNUM(k);
+ unique_attnum = -(k + 1) + unique_exprs_cnt;
break;
}
}
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index 6ed938d6ab..95b2cc683e 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -892,7 +892,7 @@ bsearch_arg(const void *key, const void *base, size_t nmemb, size_t size,
* is not necessary here (and when querying the bitmap).
*/
AttrNumber *
-build_attnums_array(Bitmapset *attrs, int *numattrs)
+build_attnums_array(Bitmapset *attrs, int nexprs, int *numattrs)
{
int i,
j;
@@ -908,16 +908,19 @@ build_attnums_array(Bitmapset *attrs, int *numattrs)
j = -1;
while ((j = bms_next_member(attrs, j)) >= 0)
{
+ AttrNumber attnum = (j - nexprs);
+
/*
* Make sure the bitmap contains only user-defined attributes. As
* bitmaps can't contain negative values, this can be violated in two
* ways. Firstly, the bitmap might contain 0 as a member, and secondly
* the integer value might be larger than MaxAttrNumber.
*/
- Assert(AttrNumberIsForUserDefinedAttr(j));
- Assert(j <= MaxAttrNumber);
+ Assert(AttributeNumberIsValid(attnum));
+ Assert(attnum <= MaxAttrNumber);
+ Assert(attnum >= (-nexprs));
- attnums[i++] = (AttrNumber) j;
+ attnums[i++] = (AttrNumber) attnum;
/* protect against overflows */
Assert(i <= num);
@@ -984,15 +987,16 @@ build_sorted_items(int numrows, int *nitems, HeapTuple *rows, ExprInfo *exprs,
Datum value;
bool isnull;
int attlen;
+ AttrNumber attnum = attnums[j];
- if (attnums[j] <= MaxHeapAttributeNumber)
+ if (AttrNumberIsForUserDefinedAttr(attnum))
{
- value = heap_getattr(rows[i], attnums[j], tdesc, &isnull);
- attlen = TupleDescAttr(tdesc, attnums[j] - 1)->attlen;
+ value = heap_getattr(rows[i], attnum, tdesc, &isnull);
+ attlen = TupleDescAttr(tdesc, attnum - 1)->attlen;
}
else
{
- int idx = EXPRESSION_INDEX(attnums[j]);
+ int idx = -(attnums[j] + 1);
Assert((idx >= 0) && (idx < exprs->nexprs));
@@ -1097,6 +1101,21 @@ stat_find_expression(StatisticExtInfo *stat, Node *expr)
return -1;
}
+static bool
+stat_covers_attributes(StatisticExtInfo *stat, Bitmapset *attnums)
+{
+ int k;
+
+ k = -1;
+ while ((k = bms_next_member(attnums, k)) >= 0)
+ {
+ if (!bms_is_member(k, stat->keys))
+ return false;
+ }
+
+ return true;
+}
+
/*
* stat_covers_expressions
* Test whether a statistics object covers all expressions in a list.
@@ -1181,7 +1200,7 @@ choose_best_statistics(List *stats, char requiredkind,
continue;
/* ignore clauses that are not covered by this object */
- if (!bms_is_subset(clause_attnums[i], info->keys) ||
+ if (!stat_covers_attributes(info, clause_attnums[i]) ||
!stat_covers_expressions(info, clause_exprs[i], &expr_idxs))
continue;
@@ -1685,7 +1704,7 @@ statext_mcv_clauselist_selectivity(PlannerInfo *root, List *clauses, int varReli
* estimate.
*/
if (!bms_is_member(listidx, *estimatedclauses) &&
- bms_is_subset(list_attnums[listidx], stat->keys) &&
+ stat_covers_attributes(stat, list_attnums[listidx]) &&
stat_covers_expressions(stat, list_exprs[listidx], NULL))
{
/* record simple clauses (single column or expression) */
@@ -2555,37 +2574,3 @@ evaluate_expressions(Relation rel, List *exprs, int numrows, HeapTuple *rows)
return result;
}
-
-/*
- * add_expressions_to_attributes
- * add expressions as attributes with high attnums
- *
- * Treat the expressions as attributes with attnums above the regular
- * attnum range. This will allow us to handle everything in the same
- * way, and identify expressions in the dependencies.
- *
- * XXX This always creates a copy of the bitmap. We might optimize this
- * by only creating the copy with (nexprs > 0) but then we'd have to track
- * this in order to free it (if we want to). Does not seem worth it.
- */
-Bitmapset *
-add_expressions_to_attributes(Bitmapset *attrs, int nexprs)
-{
- int i;
-
- /*
- * Copy the bitmapset and add fake attnums representing expressions,
- * starting above MaxHeapAttributeNumber.
- */
- attrs = bms_copy(attrs);
-
- /* start with (MaxHeapAttributeNumber + 1) */
- for (i = 0; i < nexprs; i++)
- {
- Assert(EXPRESSION_ATTNUM(i) > MaxHeapAttributeNumber);
-
- attrs = bms_add_member(attrs, EXPRESSION_ATTNUM(i));
- }
-
- return attrs;
-}
diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index 3bb6fa733d..323d476814 100644
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -187,6 +187,7 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs,
double totalrows, int stattarget)
{
int i,
+ k,
numattrs,
ngroups,
nitems;
@@ -206,10 +207,26 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs,
* XXX We do this after build_mss, because that expects the bitmapset
* to only contain simple attributes (with a matching VacAttrStats)
*/
- attrs = add_expressions_to_attributes(attrs, exprs->nexprs);
- /* now build the array, with the special expression attnums */
- attnums = build_attnums_array(attrs, &numattrs);
+ /*
+ * Transform the bms into an array, to make accessing i-th member easier.
+ */
+ attnums = (AttrNumber *) palloc(sizeof(AttrNumber) * (bms_num_members(attrs) + exprs->nexprs));
+
+ numattrs = 0;
+
+ /* regular attributes */
+ k = -1;
+ while ((k = bms_next_member(attrs, k)) >= 0)
+ attnums[numattrs++] = k;
+
+ /* treat expressions as attributes with negative attnums */
+ for (i = 0; i < exprs->nexprs; i++)
+ attnums[numattrs++] = -(i+1);
+
+ Assert(numattrs >= 2);
+ Assert(numattrs == (bms_num_members(attrs) + exprs->nexprs));
+
/* sort the rows */
items = build_sorted_items(numrows, &nitems, rows, exprs,
@@ -349,7 +366,6 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs,
pfree(items);
pfree(groups);
- pfree(attrs);
return mcvlist;
}
@@ -1692,6 +1708,8 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
bool match = true;
MCVItem *item = &mcvlist->items[i];
+ Assert(idx >= 0);
+
/*
* When the MCV item or the Const value is NULL we can
* treat this as a mismatch. We must not call the operator
diff --git a/src/backend/statistics/mvdistinct.c b/src/backend/statistics/mvdistinct.c
index 55d3fa0e1f..5e796e7123 100644
--- a/src/backend/statistics/mvdistinct.c
+++ b/src/backend/statistics/mvdistinct.c
@@ -83,9 +83,9 @@ static void generate_combinations(CombinationGenerator *state);
* This computes the ndistinct estimate using the same estimator used
* in analyze.c and then computes the coefficient.
*
- * To handle expressions easily, we treat them as special attributes with
- * attnums above MaxHeapAttributeNumber, and we assume the expressions are
- * placed after all simple attributes.
+ * To handle expressions easily, we treat them as system attributes with
+ * negative attnums, and offset everything by number of expressions to
+ * allow using Bitmapsets.
*/
MVNDistinct *
statext_ndistinct_build(double totalrows, int numrows, HeapTuple *rows,
@@ -93,10 +93,12 @@ statext_ndistinct_build(double totalrows, int numrows, HeapTuple *rows,
VacAttrStats **stats)
{
MVNDistinct *result;
+ int i;
int k;
int itemcnt;
int numattrs = bms_num_members(attrs);
int numcombs = num_combinations(numattrs + exprs->nexprs);
+ Bitmapset *tmp = NULL;
result = palloc(offsetof(MVNDistinct, items) +
numcombs * sizeof(MVNDistinctItem));
@@ -104,8 +106,26 @@ statext_ndistinct_build(double totalrows, int numrows, HeapTuple *rows,
result->type = STATS_NDISTINCT_TYPE_BASIC;
result->nitems = numcombs;
- /* treat expressions as special attributes with high attnums */
- attrs = add_expressions_to_attributes(attrs, exprs->nexprs);
+ /*
+ * Treat expressions as system attributes with negative attnums,
+ * but offset everything by number of expressions.
+ */
+ for (i = 0; i < exprs->nexprs; i++)
+ {
+ AttrNumber attnum = -(i + 1);
+ tmp = bms_add_member(tmp, attnum + exprs->nexprs);
+ }
+
+ /* regular attributes */
+ k = -1;
+ while ((k = bms_next_member(attrs, k)) >= 0)
+ {
+ AttrNumber attnum = k;
+ tmp = bms_add_member(tmp, attnum + exprs->nexprs);
+ }
+
+ /* use the newly built bitmapset */
+ attrs = tmp;
/* make sure there were no clashes */
Assert(bms_num_members(attrs) == numattrs + exprs->nexprs);
@@ -124,29 +144,33 @@ statext_ndistinct_build(double totalrows, int numrows, HeapTuple *rows,
MVNDistinctItem *item = &result->items[itemcnt];
int j;
- item->attrs = NULL;
+ item->attributes = palloc(sizeof(AttrNumber) * k);
+ item->nattributes = k;
+
for (j = 0; j < k; j++)
{
AttrNumber attnum = InvalidAttrNumber;
/*
- * The simple attributes are before expressions, so have
- * indexes below numattrs.
- * */
- if (combination[j] < numattrs)
- attnum = stats[combination[j]]->attr->attnum;
+ * The expressions have negative attnums, so even with the
+ * offset are before regular attributes. So the first chunk
+ * of indexes are for expressions.
+ */
+ if (combination[j] >= exprs->nexprs)
+ attnum
+ = stats[combination[j] - exprs->nexprs]->attr->attnum;
else
{
/* make sure the expression index is valid */
- Assert((combination[j] - numattrs) >= 0);
- Assert((combination[j] - numattrs) < exprs->nexprs);
+ Assert(combination[j] >= 0);
+ Assert(combination[j] < exprs->nexprs);
- attnum = EXPRESSION_ATTNUM(combination[j] - numattrs);
+ attnum = -(combination[j] + 1);
}
Assert(attnum != InvalidAttrNumber);
- item->attrs = bms_add_member(item->attrs, attnum);
+ item->attributes[j] = attnum;
}
item->ndistinct =
@@ -223,7 +247,7 @@ statext_ndistinct_serialize(MVNDistinct *ndistinct)
{
int nmembers;
- nmembers = bms_num_members(ndistinct->items[i].attrs);
+ nmembers = ndistinct->items[i].nattributes;
Assert(nmembers >= 2);
len += SizeOfItem(nmembers);
@@ -248,22 +272,15 @@ statext_ndistinct_serialize(MVNDistinct *ndistinct)
for (i = 0; i < ndistinct->nitems; i++)
{
MVNDistinctItem item = ndistinct->items[i];
- int nmembers = bms_num_members(item.attrs);
- int x;
+ int nmembers = item.nattributes;
memcpy(tmp, &item.ndistinct, sizeof(double));
tmp += sizeof(double);
memcpy(tmp, &nmembers, sizeof(int));
tmp += sizeof(int);
- x = -1;
- while ((x = bms_next_member(item.attrs, x)) >= 0)
- {
- AttrNumber value = (AttrNumber) x;
-
- memcpy(tmp, &value, sizeof(AttrNumber));
- tmp += sizeof(AttrNumber);
- }
+ memcpy(tmp, item.attributes, sizeof(AttrNumber) * nmembers);
+ tmp += nmembers * sizeof(AttrNumber);
/* protect against overflows */
Assert(tmp <= ((char *) output + len));
@@ -335,27 +352,21 @@ statext_ndistinct_deserialize(bytea *data)
for (i = 0; i < ndistinct->nitems; i++)
{
MVNDistinctItem *item = &ndistinct->items[i];
- int nelems;
-
- item->attrs = NULL;
/* ndistinct value */
memcpy(&item->ndistinct, tmp, sizeof(double));
tmp += sizeof(double);
/* number of attributes */
- memcpy(&nelems, tmp, sizeof(int));
+ memcpy(&item->nattributes, tmp, sizeof(int));
tmp += sizeof(int);
- Assert((nelems >= 2) && (nelems <= STATS_MAX_DIMENSIONS));
+ Assert((item->nattributes >= 2) && (item->nattributes <= STATS_MAX_DIMENSIONS));
- while (nelems-- > 0)
- {
- AttrNumber attno;
+ item->attributes
+ = (AttrNumber *) palloc(item->nattributes * sizeof(AttrNumber));
- memcpy(&attno, tmp, sizeof(AttrNumber));
- tmp += sizeof(AttrNumber);
- item->attrs = bms_add_member(item->attrs, attno);
- }
+ memcpy(item->attributes, tmp, sizeof(AttrNumber) * item->nattributes);
+ tmp += sizeof(AttrNumber) * item->nattributes;
/* still within the bytea */
Assert(tmp <= ((char *) data + VARSIZE_ANY(data)));
@@ -403,17 +414,16 @@ pg_ndistinct_out(PG_FUNCTION_ARGS)
for (i = 0; i < ndist->nitems; i++)
{
- MVNDistinctItem item = ndist->items[i];
- int x = -1;
- bool first = true;
+ int j;
+ MVNDistinctItem item = ndist->items[i];
if (i > 0)
appendStringInfoString(&str, ", ");
- while ((x = bms_next_member(item.attrs, x)) >= 0)
+ for (j = 0; j < item.nattributes; j++)
{
- appendStringInfo(&str, "%s%d", first ? "\"" : ", ", x);
- first = false;
+ AttrNumber attnum = item.attributes[j];
+ appendStringInfo(&str, "%s%d", (j == 0) ? "\"" : ", ", attnum);
}
appendStringInfo(&str, "\": %d", (int) item.ndistinct);
}
@@ -508,9 +518,10 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows,
TupleDesc tdesc = NULL;
Oid collid = InvalidOid;
- if (combination[i] < nattrs)
+ /* first nexprs indexes are for expressions, then regular attributes */
+ if (combination[i] >= exprs->nexprs)
{
- VacAttrStats *colstat = stats[combination[i]];
+ VacAttrStats *colstat = stats[combination[i] - exprs->nexprs];
typid = colstat->attrtypid;
attnum = colstat->attr->attnum;
collid = colstat->attrcollid;
@@ -518,8 +529,8 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows,
}
else
{
- typid = exprs->types[combination[i] - nattrs];
- collid = exprs->collations[combination[i] - nattrs];
+ typid = exprs->types[combination[i]];
+ collid = exprs->collations[combination[i]];
}
type = lookup_type_cache(typid, TYPECACHE_LT_OPR);
@@ -534,10 +545,13 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows,
for (j = 0; j < numrows; j++)
{
/*
- * The first nattrs indexes identify simple attributes, higher
- * indexes are expressions.
+ * The first exprs indexes identify expressions, higher indexes
+ * are for plain attributes.
+ *
+ * XXX This seems a bit strange that we don't offset the (i)
+ * in any way?
*/
- if (combination[i] < nattrs)
+ if (combination[i] >= exprs->nexprs)
items[j].values[i] =
heap_getattr(rows[j],
attnum,
@@ -545,7 +559,9 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows,
&items[j].isnull[i]);
else
{
- int idx = (combination[i] - nattrs);
+ /* we know the first nexprs expressions are expressions,
+ * and the value is directly the expression index */
+ int idx = combination[i];
/* make sure the expression index is valid */
Assert((idx >= 0) && (idx < exprs->nexprs));
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index a7edcaeaff..26e1912940 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -4144,7 +4144,8 @@ estimate_multivariate_ndistinct(PlannerInfo *root, RelOptInfo *rel,
if (equal(exprinfo->expr, expr))
{
- matched = bms_add_member(matched, MaxHeapAttributeNumber + idx);
+ AttrNumber attnum = -(idx + 1);
+ matched = bms_add_member(matched, attnum + list_length(matched_info->exprs));
found = true;
break;
}
@@ -4165,10 +4166,10 @@ estimate_multivariate_ndistinct(PlannerInfo *root, RelOptInfo *rel,
if (!AttrNumberIsForUserDefinedAttr(attnum))
continue;
- if (!bms_is_member(attnum, matched_info->keys))
+ if (!bms_is_member(attnum + list_length(matched_info->exprs), matched_info->keys))
continue;
- matched = bms_add_member(matched, attnum);
+ matched = bms_add_member(matched, attnum + list_length(matched_info->exprs));
}
}
}
@@ -4176,13 +4177,29 @@ estimate_multivariate_ndistinct(PlannerInfo *root, RelOptInfo *rel,
/* Find the specific item that exactly matches the combination */
for (i = 0; i < stats->nitems; i++)
{
+ int j;
MVNDistinctItem *tmpitem = &stats->items[i];
- if (bms_subset_compare(tmpitem->attrs, matched) == BMS_EQUAL)
+ if (tmpitem->nattributes != bms_num_members(matched))
+ continue;
+
+ /* assume it's the right item */
+ item = tmpitem;
+
+ for (j = 0; j < tmpitem->nattributes; j++)
{
- item = tmpitem;
- break;
+ AttrNumber attnum = tmpitem->attributes[j];
+
+ if (!bms_is_member(attnum, matched))
+ {
+ /* nah, it's not this item */
+ item = NULL;
+ break;
+ }
}
+
+ if (item)
+ break;
}
/* make sure we found an item */
diff --git a/src/include/statistics/extended_stats_internal.h b/src/include/statistics/extended_stats_internal.h
index b2e59f9bc5..1f09799deb 100644
--- a/src/include/statistics/extended_stats_internal.h
+++ b/src/include/statistics/extended_stats_internal.h
@@ -106,7 +106,7 @@ extern void *bsearch_arg(const void *key, const void *base,
int (*compar) (const void *, const void *, void *),
void *arg);
-extern AttrNumber *build_attnums_array(Bitmapset *attrs, int *numattrs);
+extern AttrNumber *build_attnums_array(Bitmapset *attrs, int nexprs, int *numattrs);
extern SortItem *build_sorted_items(int numrows, int *nitems, HeapTuple *rows,
ExprInfo *exprs, TupleDesc tdesc,
@@ -141,13 +141,4 @@ extern Selectivity mcv_clause_selectivity_or(PlannerInfo *root,
Selectivity *overlap_basesel,
Selectivity *totalsel);
-extern Bitmapset *add_expressions_to_attributes(Bitmapset *attrs, int nexprs);
-
-/* translate 0-based expression index to attnum and back */
-#define EXPRESSION_ATTNUM(index) \
- (MaxHeapAttributeNumber + (index) + 1)
-
-#define EXPRESSION_INDEX(attnum) \
- ((attnum) - MaxHeapAttributeNumber - 1)
-
#endif /* EXTENDED_STATS_INTERNAL_H */
diff --git a/src/include/statistics/statistics.h b/src/include/statistics/statistics.h
index 006d578e0c..326cf26fea 100644
--- a/src/include/statistics/statistics.h
+++ b/src/include/statistics/statistics.h
@@ -26,7 +26,8 @@
typedef struct MVNDistinctItem
{
double ndistinct; /* ndistinct value for this combination */
- Bitmapset *attrs; /* attr numbers of items */
+ int nattributes; /* number of attributes */
+ AttrNumber *attributes; /* attribute numbers */
} MVNDistinctItem;
/* A MVNDistinct object, comprising all possible combinations of columns */
--
2.26.2
--------------614DDB87AFFED893713AC0E9
Content-Type: text/x-patch; charset=UTF-8;
name="0005-WIP-unify-handling-of-attributes-and-expres-20210304.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="0005-WIP-unify-handling-of-attributes-and-expres-20210304.pa";
filename*1="tch"
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: standby recovery fails (tablespace related) (tentative patch and discussion)
@ 2022-03-21 19:03 Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Alvaro Herrera @ 2022-03-21 19:03 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Michael Paquier <[email protected]>; Julien Rouhaud <[email protected]>; pgsql-hackers
On 2022-Mar-14, Robert Haas wrote:
> 2. Why not instead change the code so that the operation can succeed,
> by creating the prerequisite parent directories? Do we not have enough
> information for that? I'm not saying that we definitely should do it
> that way rather than this way, but I think we do take that approach in
> some cases.
It seems we can choose freely between these two implementations -- I
mean I don't see any upsides or downsides to either one.
The current one has the advantage that it never makes the datadir
"dirty", to use Kyotaro's term. It verifies that the creation/drop form
a pair. A possible downside is that if there's a bug, we could end up
with a spurious PANIC at the end of recovery, and no way to recover.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: standby recovery fails (tablespace related) (tentative patch and discussion)
@ 2022-03-28 16:17 Robert Haas <[email protected]>
parent: Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Robert Haas @ 2022-03-28 16:17 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Michael Paquier <[email protected]>; Julien Rouhaud <[email protected]>; pgsql-hackers
On Mon, Mar 21, 2022 at 3:02 PM Alvaro Herrera <[email protected]> wrote:
> > 2. Why not instead change the code so that the operation can succeed,
> > by creating the prerequisite parent directories? Do we not have enough
> > information for that? I'm not saying that we definitely should do it
> > that way rather than this way, but I think we do take that approach in
> > some cases.
>
> It seems we can choose freely between these two implementations -- I
> mean I don't see any upsides or downsides to either one.
What got committed here feels inconsistent to me. Suppose we have a
checkpoint, and then a series of operations that touch a tablespace,
and then a drop database and drop tablespace. If the first operation
happens to be CREATE DATABASE, then this patch is going to fix it by
skipping the operation. However, if the first operation happens to be
almost anything else, the way it's going to reference the dropped
tablespace is via a block reference in a WAL record of a wide variety
of types. That's going to result in a call to
XLogReadBufferForRedoExtended() which will call
XLogReadBufferExtended() which will do smgrcreate(smgr, forknum, true)
which will in turn call TablespaceCreateDbspace() to fill in all the
missing directories.
I don't think that's very good. It would be reasonable to decide that
we're never going to create the missing directories and instead just
remember that they were not found so we can do a cross check. It's
also reasonable to just create the directories on the fly. But doing a
mix of those systems doesn't really seem like the right idea -
particularly because it means that the cross-check system is probably
not very effective at finding actual problems in the code.
Am I missing something here?
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: standby recovery fails (tablespace related) (tentative patch and discussion)
@ 2022-03-29 04:55 Kyotaro Horiguchi <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-29 04:55 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers
At Mon, 28 Mar 2022 12:17:50 -0400, Robert Haas <[email protected]> wrote in
> On Mon, Mar 21, 2022 at 3:02 PM Alvaro Herrera <[email protected]> wrote:
> > > 2. Why not instead change the code so that the operation can succeed,
> > > by creating the prerequisite parent directories? Do we not have enough
> > > information for that? I'm not saying that we definitely should do it
> > > that way rather than this way, but I think we do take that approach in
> > > some cases.
> >
> > It seems we can choose freely between these two implementations -- I
> > mean I don't see any upsides or downsides to either one.
>
> What got committed here feels inconsistent to me. Suppose we have a
> checkpoint, and then a series of operations that touch a tablespace,
> and then a drop database and drop tablespace. If the first operation
> happens to be CREATE DATABASE, then this patch is going to fix it by
> skipping the operation. However, if the first operation happens to be
> almost anything else, the way it's going to reference the dropped
> tablespace is via a block reference in a WAL record of a wide variety
> of types. That's going to result in a call to
> XLogReadBufferForRedoExtended() which will call
> XLogReadBufferExtended() which will do smgrcreate(smgr, forknum, true)
> which will in turn call TablespaceCreateDbspace() to fill in all the
> missing directories.
Right. I thought that recovery avoids that but that's wrong. This
behavior creates a bare (non-linked) directly within pg_tblspc. The
directory would dissapear soon if recovery proceeds to the consistency
point, though.
> I don't think that's very good. It would be reasonable to decide that
> we're never going to create the missing directories and instead just
> remember that they were not found so we can do a cross check. It's
> also reasonable to just create the directories on the fly. But doing a
> mix of those systems doesn't really seem like the right idea -
> particularly because it means that the cross-check system is probably
> not very effective at finding actual problems in the code.
>
> Am I missing something here?
No. I agree that mixing them is not good. On the other hand we
already doing that by heapam. AFAICS sometimes it avoid creating a
new page but sometimes creates it. But I don't mean to use the fact
for justifying this patch to do that, or denying to do that.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: standby recovery fails (tablespace related) (tentative patch and discussion)
@ 2022-03-29 11:37 Alvaro Herrera <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Alvaro Herrera @ 2022-03-29 11:37 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers
On 2022-Mar-29, Kyotaro Horiguchi wrote:
> > That's going to result in a call to
> > XLogReadBufferForRedoExtended() which will call
> > XLogReadBufferExtended() which will do smgrcreate(smgr, forknum, true)
> > which will in turn call TablespaceCreateDbspace() to fill in all the
> > missing directories.
>
> Right. I thought that recovery avoids that but that's wrong. This
> behavior creates a bare (non-linked) directly within pg_tblspc. The
> directory would dissapear soon if recovery proceeds to the consistency
> point, though.
Hmm, this is not good.
> No. I agree that mixing them is not good. On the other hand we
> already doing that by heapam. AFAICS sometimes it avoid creating a
> new page but sometimes creates it. But I don't mean to use the fact
> for justifying this patch to do that, or denying to do that.
I think we should revert this patch and do it again using the other
approach: create a stub directory during recovery that can be deleted
later.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... ¿Quién es el machito que tendría carnet?" (Mafalda)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: standby recovery fails (tablespace related) (tentative patch and discussion)
@ 2022-03-29 12:45 Robert Haas <[email protected]>
parent: Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Robert Haas @ 2022-03-29 12:45 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Michael Paquier <[email protected]>; Julien Rouhaud <[email protected]>; pgsql-hackers
On Tue, Mar 29, 2022 at 7:37 AM Alvaro Herrera <[email protected]> wrote:
> I think we should revert this patch and do it again using the other
> approach: create a stub directory during recovery that can be deleted
> later.
I'm fine with that approach, but I'd like to ask that we proceed
expeditiously, because I have another patch that I want to commit that
touches this area. I can commit to helping with whatever we decide to
do here, but I don't want to keep that patch on ice while we figure it
out and then have it miss the release.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: standby recovery fails (tablespace related) (tentative patch and discussion)
@ 2022-03-29 13:28 Alvaro Herrera <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Alvaro Herrera @ 2022-03-29 13:28 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Michael Paquier <[email protected]>; Julien Rouhaud <[email protected]>; pgsql-hackers
On 2022-Mar-29, Robert Haas wrote:
> I'm fine with that approach, but I'd like to ask that we proceed
> expeditiously, because I have another patch that I want to commit that
> touches this area. I can commit to helping with whatever we decide to
> do here, but I don't want to keep that patch on ice while we figure it
> out and then have it miss the release.
OK, this is a bug that's been open for years. A fix can be committed
after the feature freeze anyway.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: standby recovery fails (tablespace related) (tentative patch and discussion)
@ 2022-03-29 13:31 Robert Haas <[email protected]>
parent: Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Robert Haas @ 2022-03-29 13:31 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Michael Paquier <[email protected]>; Julien Rouhaud <[email protected]>; pgsql-hackers
On Tue, Mar 29, 2022 at 9:28 AM Alvaro Herrera <[email protected]> wrote:
> OK, this is a bug that's been open for years. A fix can be committed
> after the feature freeze anyway.
+1
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2022-03-29 13:31 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-02-17 00:02 [PATCH 4/5] WIP rework tracking of expressions Tomas Vondra <[email protected]>
2022-03-21 19:03 Re: standby recovery fails (tablespace related) (tentative patch and discussion) Alvaro Herrera <[email protected]>
2022-03-28 16:17 ` Re: standby recovery fails (tablespace related) (tentative patch and discussion) Robert Haas <[email protected]>
2022-03-29 04:55 ` Re: standby recovery fails (tablespace related) (tentative patch and discussion) Kyotaro Horiguchi <[email protected]>
2022-03-29 11:37 ` Re: standby recovery fails (tablespace related) (tentative patch and discussion) Alvaro Herrera <[email protected]>
2022-03-29 12:45 ` Re: standby recovery fails (tablespace related) (tentative patch and discussion) Robert Haas <[email protected]>
2022-03-29 13:28 ` Re: standby recovery fails (tablespace related) (tentative patch and discussion) Alvaro Herrera <[email protected]>
2022-03-29 13:31 ` Re: standby recovery fails (tablespace related) (tentative patch and discussion) Robert Haas <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox