public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 4/5] WIP rework tracking of expressions 6+ messages / 6 participants [nested] [flat]
* [PATCH 4/5] WIP rework tracking of expressions @ 2021-02-17 00:02 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 6+ 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] 6+ messages in thread
* Is WAL_DEBUG related code still relevant today? @ 2023-12-02 14:06 Bharath Rupireddy <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Bharath Rupireddy @ 2023-12-02 14:06 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> Hi, I was recently looking at the code around the WAL_DEBUG macro and GUC. When enabled, the code does the following: 1. Creates a memory context that allows pallocs within critical sections. 2. Decodes (not logical decoding but DecodeXLogRecord()) every WAL record using the above memory context that's generated in the server and emits a LOG message. 3. Emits messages at DEBUG level in AdvanceXLInsertBuffer(), at LOG level in XLogFlush(), at LOG level in XLogBackgroundFlush(). 4. Emits messages at LOG level for every record that the server replays/applies in the main redo loop. I enabled this code by compiling with the WAL_DEBUG macro and setting wal_debug GUC to on. Firstly, the compilation on Windows failed because XL_ROUTINE was passed inappropriately for XLogReaderAllocate() used. After fixing the compilation issue [1], the TAP tests started to fail [2] which I'm sure we can fix. I started to think if this code is needed at all in production. How about we do either of the following? a) Remove the WAL_DEBUG macro and move all the code under the wal_debug GUC? Since the GUC is already marked as DEVELOPER_OPTION, the users will know the consequences of enabling it in production. b) Remove both the WAL_DEBUG macro and the wal_debug GUC. I don't think (2) is needed to be in core especially when tools like pg_walinspect and pg_waldump can do the same job. And, the messages in (3) and (4) can be turned to some DEBUGX level without being under the WAL_DEBUG macro. I have no idea if anyone uses WAL_DEBUG macro and wal_debug GUCs in production, if we have somebody using it, I think we need to fix the compilation and test failure issues, and start testing this code (perhaps I can think of setting up a buildfarm member to help here). I'm in favour of option (b), but I'd like to hear more thoughts on this. [1] diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index ca7100d4db..52633793d4 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -1023,8 +1023,12 @@ XLogInsertRecord(XLogRecData *rdata, palloc(DecodeXLogRecordRequiredSpace(record->xl_tot_len)); if (!debug_reader) - debug_reader = XLogReaderAllocate(wal_segment_size, NULL, - XL_ROUTINE(), NULL); + debug_reader = XLogReaderAllocate(wal_segment_size, + NULL, + XL_ROUTINE(.page_read = NULL, + .segment_open = NULL, + .segment_close = NULL), + NULL); [2] src/test/subscription/t/029_on_error.pl because the test gets LSN from an error context message emitted to server logs which the new WAL_DEBUG LOG messages flood the server logs with. src/bin/initdb/t/001_initdb.pl because the WAL_DEBUG LOG messages are emitted to the console while initdb. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Is WAL_DEBUG related code still relevant today? @ 2023-12-06 11:27 Peter Eisentraut <[email protected]> parent: Bharath Rupireddy <[email protected]> 0 siblings, 2 replies; 6+ messages in thread From: Peter Eisentraut @ 2023-12-06 11:27 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> On 02.12.23 15:06, Bharath Rupireddy wrote: > I enabled this code by compiling with the WAL_DEBUG macro and setting > wal_debug GUC to on. Firstly, the compilation on Windows failed > because XL_ROUTINE was passed inappropriately for XLogReaderAllocate() > used. This kind of thing could be mostly avoided if we didn't hide all the WAL_DEBUG behind #ifdefs. For example, in the attached patch, I instead changed it so that if (XLOG_DEBUG) resolves to if (false) in the normal case. That way, we don't need to wrap that in #ifdef WAL_DEBUG, and the compiler can see the disabled code and make sure it continues to build. From 93c74462c3e253f3ce9fd1beba1f09290988de8c Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Wed, 6 Dec 2023 12:22:38 +0100 Subject: [PATCH] Make WAL_DEBUG code harder to break accidentally --- src/backend/access/transam/generic_xlog.c | 2 -- src/backend/access/transam/xlog.c | 10 ---------- src/backend/access/transam/xlogrecovery.c | 7 ------- src/include/access/xlog.h | 2 ++ 4 files changed, 2 insertions(+), 19 deletions(-) diff --git a/src/backend/access/transam/generic_xlog.c b/src/backend/access/transam/generic_xlog.c index abd9e1c749..5b910f1024 100644 --- a/src/backend/access/transam/generic_xlog.c +++ b/src/backend/access/transam/generic_xlog.c @@ -248,7 +248,6 @@ computeDelta(PageData *pageData, Page curpage, Page targetpage) * If xlog debug is enabled, then check produced delta. Result of delta * application to curpage should be equivalent to targetpage. */ -#ifdef WAL_DEBUG if (XLOG_DEBUG) { PGAlignedBlock tmp; @@ -260,7 +259,6 @@ computeDelta(PageData *pageData, Page curpage, Page targetpage) BLCKSZ - targetUpper) != 0) elog(ERROR, "result of generic xlog apply does not match"); } -#endif } /* diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 2d603d8dee..3a7a38a100 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -644,9 +644,7 @@ static bool updateMinRecoveryPoint = true; static int MyLockNo = 0; static bool holdingAllLocks = false; -#ifdef WAL_DEBUG static MemoryContext walDebugCxt = NULL; -#endif static void CleanupAfterArchiveRecovery(TimeLineID EndOfLogTLI, XLogRecPtr EndOfLog, @@ -997,7 +995,6 @@ XLogInsertRecord(XLogRecData *rdata, } } -#ifdef WAL_DEBUG if (XLOG_DEBUG) { static XLogReaderState *debug_reader = NULL; @@ -1061,7 +1058,6 @@ XLogInsertRecord(XLogRecData *rdata, pfree(recordBuf.data); MemoryContextSwitchTo(oldCxt); } -#endif /* * Update our global variables @@ -1997,13 +1993,11 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic) } LWLockRelease(WALBufMappingLock); -#ifdef WAL_DEBUG if (XLOG_DEBUG && npages > 0) { elog(DEBUG1, "initialized %d pages, up to %X/%X", npages, LSN_FORMAT_ARGS(NewPageEndPtr)); } -#endif } /* @@ -2641,13 +2635,11 @@ XLogFlush(XLogRecPtr record) if (record <= LogwrtResult.Flush) return; -#ifdef WAL_DEBUG if (XLOG_DEBUG) elog(LOG, "xlog flush request %X/%X; write %X/%X; flush %X/%X", LSN_FORMAT_ARGS(record), LSN_FORMAT_ARGS(LogwrtResult.Write), LSN_FORMAT_ARGS(LogwrtResult.Flush)); -#endif START_CRIT_SECTION(); @@ -2903,14 +2895,12 @@ XLogBackgroundFlush(void) WriteRqst.Flush = 0; } -#ifdef WAL_DEBUG if (XLOG_DEBUG) elog(LOG, "xlog bg flush request write %X/%X; flush: %X/%X, current is write %X/%X; flush %X/%X", LSN_FORMAT_ARGS(WriteRqst.Write), LSN_FORMAT_ARGS(WriteRqst.Flush), LSN_FORMAT_ARGS(LogwrtResult.Write), LSN_FORMAT_ARGS(LogwrtResult.Flush)); -#endif START_CRIT_SECTION(); diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index c61566666a..26ba990b71 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -396,9 +396,7 @@ static bool read_tablespace_map(List **tablespaces); static void xlogrecovery_redo(XLogReaderState *record, TimeLineID replayTLI); static void CheckRecoveryConsistency(void); static void rm_redo_error_callback(void *arg); -#ifdef WAL_DEBUG static void xlog_outrec(StringInfo buf, XLogReaderState *record); -#endif static void xlog_block_info(StringInfo buf, XLogReaderState *record); static void checkTimeLineSwitch(XLogRecPtr lsn, TimeLineID newTLI, TimeLineID prevTLI, TimeLineID replayTLI); @@ -1703,7 +1701,6 @@ PerformWalRecovery(void) ereport_startup_progress("redo in progress, elapsed time: %ld.%02d s, current LSN: %X/%X", LSN_FORMAT_ARGS(xlogreader->ReadRecPtr)); -#ifdef WAL_DEBUG if (XLOG_DEBUG || (record->xl_rmid == RM_XACT_ID && trace_recovery_messages <= DEBUG2) || (record->xl_rmid != RM_XACT_ID && trace_recovery_messages <= DEBUG3)) @@ -1720,7 +1717,6 @@ PerformWalRecovery(void) elog(LOG, "%s", buf.data); pfree(buf.data); } -#endif /* Handle interrupt signals of startup process */ HandleStartupProcInterrupts(); @@ -2256,8 +2252,6 @@ xlog_outdesc(StringInfo buf, XLogReaderState *record) rmgr.rm_desc(buf, record); } -#ifdef WAL_DEBUG - static void xlog_outrec(StringInfo buf, XLogReaderState *record) { @@ -2270,7 +2264,6 @@ xlog_outrec(StringInfo buf, XLogReaderState *record) xlog_block_info(buf, record); } -#endif /* WAL_DEBUG */ /* * Returns a string giving information about all the blocks in an diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index a14126d164..698dd79c2d 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -125,6 +125,8 @@ extern PGDLLIMPORT int wal_level; #ifdef WAL_DEBUG extern PGDLLIMPORT bool XLOG_DEBUG; +#else +#define XLOG_DEBUG false #endif /* -- 2.43.0 Attachments: [text/plain] 0001-Make-WAL_DEBUG-code-harder-to-break-accidentally.patch (5.1K, ../../[email protected]/2-0001-Make-WAL_DEBUG-code-harder-to-break-accidentally.patch) download | inline diff: From 93c74462c3e253f3ce9fd1beba1f09290988de8c Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Wed, 6 Dec 2023 12:22:38 +0100 Subject: [PATCH] Make WAL_DEBUG code harder to break accidentally --- src/backend/access/transam/generic_xlog.c | 2 -- src/backend/access/transam/xlog.c | 10 ---------- src/backend/access/transam/xlogrecovery.c | 7 ------- src/include/access/xlog.h | 2 ++ 4 files changed, 2 insertions(+), 19 deletions(-) diff --git a/src/backend/access/transam/generic_xlog.c b/src/backend/access/transam/generic_xlog.c index abd9e1c749..5b910f1024 100644 --- a/src/backend/access/transam/generic_xlog.c +++ b/src/backend/access/transam/generic_xlog.c @@ -248,7 +248,6 @@ computeDelta(PageData *pageData, Page curpage, Page targetpage) * If xlog debug is enabled, then check produced delta. Result of delta * application to curpage should be equivalent to targetpage. */ -#ifdef WAL_DEBUG if (XLOG_DEBUG) { PGAlignedBlock tmp; @@ -260,7 +259,6 @@ computeDelta(PageData *pageData, Page curpage, Page targetpage) BLCKSZ - targetUpper) != 0) elog(ERROR, "result of generic xlog apply does not match"); } -#endif } /* diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 2d603d8dee..3a7a38a100 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -644,9 +644,7 @@ static bool updateMinRecoveryPoint = true; static int MyLockNo = 0; static bool holdingAllLocks = false; -#ifdef WAL_DEBUG static MemoryContext walDebugCxt = NULL; -#endif static void CleanupAfterArchiveRecovery(TimeLineID EndOfLogTLI, XLogRecPtr EndOfLog, @@ -997,7 +995,6 @@ XLogInsertRecord(XLogRecData *rdata, } } -#ifdef WAL_DEBUG if (XLOG_DEBUG) { static XLogReaderState *debug_reader = NULL; @@ -1061,7 +1058,6 @@ XLogInsertRecord(XLogRecData *rdata, pfree(recordBuf.data); MemoryContextSwitchTo(oldCxt); } -#endif /* * Update our global variables @@ -1997,13 +1993,11 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic) } LWLockRelease(WALBufMappingLock); -#ifdef WAL_DEBUG if (XLOG_DEBUG && npages > 0) { elog(DEBUG1, "initialized %d pages, up to %X/%X", npages, LSN_FORMAT_ARGS(NewPageEndPtr)); } -#endif } /* @@ -2641,13 +2635,11 @@ XLogFlush(XLogRecPtr record) if (record <= LogwrtResult.Flush) return; -#ifdef WAL_DEBUG if (XLOG_DEBUG) elog(LOG, "xlog flush request %X/%X; write %X/%X; flush %X/%X", LSN_FORMAT_ARGS(record), LSN_FORMAT_ARGS(LogwrtResult.Write), LSN_FORMAT_ARGS(LogwrtResult.Flush)); -#endif START_CRIT_SECTION(); @@ -2903,14 +2895,12 @@ XLogBackgroundFlush(void) WriteRqst.Flush = 0; } -#ifdef WAL_DEBUG if (XLOG_DEBUG) elog(LOG, "xlog bg flush request write %X/%X; flush: %X/%X, current is write %X/%X; flush %X/%X", LSN_FORMAT_ARGS(WriteRqst.Write), LSN_FORMAT_ARGS(WriteRqst.Flush), LSN_FORMAT_ARGS(LogwrtResult.Write), LSN_FORMAT_ARGS(LogwrtResult.Flush)); -#endif START_CRIT_SECTION(); diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index c61566666a..26ba990b71 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -396,9 +396,7 @@ static bool read_tablespace_map(List **tablespaces); static void xlogrecovery_redo(XLogReaderState *record, TimeLineID replayTLI); static void CheckRecoveryConsistency(void); static void rm_redo_error_callback(void *arg); -#ifdef WAL_DEBUG static void xlog_outrec(StringInfo buf, XLogReaderState *record); -#endif static void xlog_block_info(StringInfo buf, XLogReaderState *record); static void checkTimeLineSwitch(XLogRecPtr lsn, TimeLineID newTLI, TimeLineID prevTLI, TimeLineID replayTLI); @@ -1703,7 +1701,6 @@ PerformWalRecovery(void) ereport_startup_progress("redo in progress, elapsed time: %ld.%02d s, current LSN: %X/%X", LSN_FORMAT_ARGS(xlogreader->ReadRecPtr)); -#ifdef WAL_DEBUG if (XLOG_DEBUG || (record->xl_rmid == RM_XACT_ID && trace_recovery_messages <= DEBUG2) || (record->xl_rmid != RM_XACT_ID && trace_recovery_messages <= DEBUG3)) @@ -1720,7 +1717,6 @@ PerformWalRecovery(void) elog(LOG, "%s", buf.data); pfree(buf.data); } -#endif /* Handle interrupt signals of startup process */ HandleStartupProcInterrupts(); @@ -2256,8 +2252,6 @@ xlog_outdesc(StringInfo buf, XLogReaderState *record) rmgr.rm_desc(buf, record); } -#ifdef WAL_DEBUG - static void xlog_outrec(StringInfo buf, XLogReaderState *record) { @@ -2270,7 +2264,6 @@ xlog_outrec(StringInfo buf, XLogReaderState *record) xlog_block_info(buf, record); } -#endif /* WAL_DEBUG */ /* * Returns a string giving information about all the blocks in an diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index a14126d164..698dd79c2d 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -125,6 +125,8 @@ extern PGDLLIMPORT int wal_level; #ifdef WAL_DEBUG extern PGDLLIMPORT bool XLOG_DEBUG; +#else +#define XLOG_DEBUG false #endif /* -- 2.43.0 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Is WAL_DEBUG related code still relevant today? @ 2023-12-06 12:46 Euler Taveira <[email protected]> parent: Peter Eisentraut <[email protected]> 1 sibling, 1 reply; 6+ messages in thread From: Euler Taveira @ 2023-12-06 12:46 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Dec 6, 2023, at 8:27 AM, Peter Eisentraut wrote: > On 02.12.23 15:06, Bharath Rupireddy wrote: > > I enabled this code by compiling with the WAL_DEBUG macro and setting > > wal_debug GUC to on. Firstly, the compilation on Windows failed > > because XL_ROUTINE was passed inappropriately for XLogReaderAllocate() > > used. > > This kind of thing could be mostly avoided if we didn't hide all the > WAL_DEBUG behind #ifdefs. AFAICS LOCK_DEBUG also hides its GUCs behind #ifdefs. The fact that XLOG_DEBUG is a variable but seems like a constant surprises me. I would rename it to XLogDebug or xlog_debug. > in the normal case. That way, we don't need to wrap that in #ifdef > WAL_DEBUG, and the compiler can see the disabled code and make sure it > continues to build. I didn't check the LOCK_DEBUG code path to make sure it fits in the same category as WAL_DEBUG. If it does, maybe it is worth to apply the same logic there. -- Euler Taveira EDB https://www.enterprisedb.com/ ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Is WAL_DEBUG related code still relevant today? @ 2023-12-06 15:06 Tom Lane <[email protected]> parent: Peter Eisentraut <[email protected]> 1 sibling, 0 replies; 6+ messages in thread From: Tom Lane @ 2023-12-06 15:06 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> Peter Eisentraut <[email protected]> writes: > This kind of thing could be mostly avoided if we didn't hide all the > WAL_DEBUG behind #ifdefs. For example, in the attached patch, I instead > changed it so that > if (XLOG_DEBUG) > resolves to > if (false) > in the normal case. That way, we don't need to wrap that in #ifdef > WAL_DEBUG, and the compiler can see the disabled code and make sure it > continues to build. Hmm, maybe, but I'm not sure this would be an unalloyed good. The main concern I have is compilers and static analyzers starting to bleat about unreachable code (warnings like "variable set but never used", or the like, seem plausible). The dead code would also decrease our code coverage statistics, not that those are wonderful now. regards, tom lane ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Is WAL_DEBUG related code still relevant today? @ 2023-12-07 00:51 Michael Paquier <[email protected]> parent: Euler Taveira <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Michael Paquier @ 2023-12-07 00:51 UTC (permalink / raw) To: Euler Taveira <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Dec 06, 2023 at 09:46:09AM -0300, Euler Taveira wrote: > On Wed, Dec 6, 2023, at 8:27 AM, Peter Eisentraut wrote: >> This kind of thing could be mostly avoided if we didn't hide all the >> WAL_DEBUG behind #ifdefs. > > AFAICS LOCK_DEBUG also hides its GUCs behind #ifdefs. The fact that XLOG_DEBUG > is a variable but seems like a constant surprises me. I would rename it to > XLogDebug or xlog_debug. +1. Or just wal_debug for greppability. >> in the normal case. That way, we don't need to wrap that in #ifdef >> WAL_DEBUG, and the compiler can see the disabled code and make sure it >> continues to build. > > I didn't check the LOCK_DEBUG code path to make sure it fits in the same > category as WAL_DEBUG. If it does, maybe it is worth to apply the same logic > there. PerformWalRecovery() with its log for RM_XACT_ID is something that stresses me a bit though because this is in the main redo loop which is never free. The same can be said about GenericXLogFinish() because the extra computation happens while holding a buffer and marking it dirty. The ones in xlog.c are free of charge as they are called outside any critical portions. This makes me wonder how much we need to care about trace_recovery_messages, actually, and I've never used it. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-12-07 00:51 UTC | newest] Thread overview: 6+ 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]> 2023-12-02 14:06 Is WAL_DEBUG related code still relevant today? Bharath Rupireddy <[email protected]> 2023-12-06 11:27 ` Re: Is WAL_DEBUG related code still relevant today? Peter Eisentraut <[email protected]> 2023-12-06 12:46 ` Re: Is WAL_DEBUG related code still relevant today? Euler Taveira <[email protected]> 2023-12-07 00:51 ` Re: Is WAL_DEBUG related code still relevant today? Michael Paquier <[email protected]> 2023-12-06 15:06 ` Re: Is WAL_DEBUG related code still relevant today? Tom Lane <[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