public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 5/5] WIP unify handling of attributes and expressions 11+ messages / 5 participants [nested] [flat]
* [PATCH 5/5] WIP unify handling of attributes and expressions @ 2021-03-04 03:53 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Tomas Vondra @ 2021-03-04 03:53 UTC (permalink / raw) --- src/backend/statistics/dependencies.c | 82 ++------ src/backend/statistics/extended_stats.c | 180 +++++++++++------- src/backend/statistics/mcv.c | 89 ++------- src/backend/statistics/mvdistinct.c | 138 +++----------- .../statistics/extended_stats_internal.h | 40 ++-- 5 files changed, 183 insertions(+), 346 deletions(-) diff --git a/src/backend/statistics/dependencies.c b/src/backend/statistics/dependencies.c index 602301b724..14d6503e1a 100644 --- a/src/backend/statistics/dependencies.c +++ b/src/backend/statistics/dependencies.c @@ -70,10 +70,7 @@ static void generate_dependencies(DependencyGenerator state); static DependencyGenerator DependencyGenerator_init(int n, int k); static void DependencyGenerator_free(DependencyGenerator state); static AttrNumber *DependencyGenerator_next(DependencyGenerator state); -static double dependency_degree(int numrows, HeapTuple *rows, - ExprInfo *exprs, int k, - AttrNumber *dependency, VacAttrStats **stats, - Bitmapset *attrs); +static double dependency_degree(StatBuildData *data, int k, AttrNumber *dependency); static bool dependency_is_fully_matched(MVDependency *dependency, Bitmapset *attnums); static bool dependency_is_compatible_clause(Node *clause, Index relid, @@ -222,17 +219,13 @@ DependencyGenerator_next(DependencyGenerator state) * the last one. */ static double -dependency_degree(int numrows, HeapTuple *rows, ExprInfo *exprs, int k, - AttrNumber *dependency, VacAttrStats **stats, - Bitmapset *attrs) +dependency_degree(StatBuildData *data, int k, AttrNumber *dependency) { int i, nitems; MultiSortSupport mss; SortItem *items; - AttrNumber *attnums; AttrNumber *attnums_dep; - int numattrs; /* counters valid within a group */ int group_size = 0; @@ -247,16 +240,9 @@ dependency_degree(int numrows, HeapTuple *rows, ExprInfo *exprs, int k, /* sort info for all attributes columns */ mss = multi_sort_init(k); - /* - * Transform the attrs from bitmap to an array to make accessing the i-th - * member easier, and then construct a filtered version with only attnums - * referenced by the dependency we validate. - */ - attnums = build_attnums_array(attrs, exprs->nexprs, &numattrs); - attnums_dep = (AttrNumber *) palloc(k * sizeof(AttrNumber)); for (i = 0; i < k; i++) - attnums_dep[i] = attnums[dependency[i]]; + attnums_dep[i] = data->attnums[dependency[i]]; /* * Verify the dependency (a,b,...)->z, using a rather simple algorithm: @@ -274,7 +260,7 @@ dependency_degree(int numrows, HeapTuple *rows, ExprInfo *exprs, int k, /* prepare the sort function for the dimensions */ for (i = 0; i < k; i++) { - VacAttrStats *colstat = stats[dependency[i]]; + VacAttrStats *colstat = data->stats[dependency[i]]; TypeCacheEntry *type; type = lookup_type_cache(colstat->attrtypid, TYPECACHE_LT_OPR); @@ -293,8 +279,7 @@ dependency_degree(int numrows, HeapTuple *rows, ExprInfo *exprs, int k, * descriptor. For now that assumption holds, but it might change in the * future for example if we support statistics on multiple tables. */ - items = build_sorted_items(numrows, &nitems, rows, exprs, - stats[0]->tupDesc, mss, k, attnums_dep); + items = build_sorted_items(data, &nitems, mss, k, attnums_dep); /* * Walk through the sorted array, split it into rows according to the @@ -340,11 +325,10 @@ dependency_degree(int numrows, HeapTuple *rows, ExprInfo *exprs, int k, pfree(items); pfree(mss); - pfree(attnums); pfree(attnums_dep); /* Compute the 'degree of validity' as (supporting/total). */ - return (n_supporting_rows * 1.0 / numrows); + return (n_supporting_rows * 1.0 / data->numrows); } /* @@ -364,54 +348,15 @@ dependency_degree(int numrows, HeapTuple *rows, ExprInfo *exprs, int k, * (c) -> b */ MVDependencies * -statext_dependencies_build(int numrows, HeapTuple *rows, - ExprInfo *exprs, Bitmapset *attrs, - VacAttrStats **stats) +statext_dependencies_build(StatBuildData *data) { int i, k; - int numattrs; - AttrNumber *attnums; - int nattnums; /* result */ MVDependencies *dependencies = NULL; - /* - * 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); - - /* - * and then regular attributes - * - * XXX Maybe add this in the opposite order, just like in MCV? first - * regular attnums, then exressions. - */ - 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); + Assert(data->nattnums >= 2); /* * We'll try build functional dependencies starting from the smallest ones @@ -419,12 +364,12 @@ statext_dependencies_build(int numrows, HeapTuple *rows, * included in the statistics object. We start from the smallest ones * because we want to be able to skip already implied ones. */ - for (k = 2; k <= numattrs; k++) + for (k = 2; k <= data->nattnums; k++) { AttrNumber *dependency; /* array with k elements */ /* prepare a DependencyGenerator of variation */ - DependencyGenerator DependencyGenerator = DependencyGenerator_init(numattrs, k); + DependencyGenerator DependencyGenerator = DependencyGenerator_init(data->nattnums, k); /* generate all possible variations of k values (out of n) */ while ((dependency = DependencyGenerator_next(DependencyGenerator))) @@ -433,8 +378,7 @@ statext_dependencies_build(int numrows, HeapTuple *rows, MVDependency *d; /* compute how valid the dependency seems */ - degree = dependency_degree(numrows, rows, exprs, k, dependency, - stats, attrs); + degree = dependency_degree(data, k, dependency); /* * if the dependency seems entirely invalid, don't store it @@ -449,7 +393,7 @@ statext_dependencies_build(int numrows, HeapTuple *rows, d->degree = degree; d->nattributes = k; for (i = 0; i < k; i++) - d->attributes[i] = attnums[dependency[i]]; + d->attributes[i] = data->attnums[dependency[i]]; /* initialize the list of dependencies */ if (dependencies == NULL) @@ -477,8 +421,6 @@ statext_dependencies_build(int numrows, HeapTuple *rows, DependencyGenerator_free(DependencyGenerator); } - pfree(attrs); - return dependencies; } diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c index 95b2cc683e..5b3fa523e9 100644 --- a/src/backend/statistics/extended_stats.c +++ b/src/backend/statistics/extended_stats.c @@ -96,8 +96,11 @@ static Datum serialize_expr_stats(AnlExprData *exprdata, int nexprs); static Datum expr_fetch_func(VacAttrStatsP stats, int rownum, bool *isNull); static AnlExprData *build_expr_data(List *exprs); static VacAttrStats *examine_expression(Node *expr); -static ExprInfo *evaluate_expressions(Relation rel, List *exprs, - int numrows, HeapTuple *rows); + +static StatBuildData *make_build_data(Relation onerel, StatExtEntry *stat, + int numrows, HeapTuple *rows, + VacAttrStats **stats); + /* * Compute requested extended stats, using the rows sampled for the plain @@ -156,7 +159,7 @@ BuildRelationExtStatistics(Relation onerel, double totalrows, VacAttrStats **stats; ListCell *lc2; int stattarget; - ExprInfo *exprs; + StatBuildData *data; /* * Check if we can build these stats based on the column analyzed. If @@ -191,7 +194,7 @@ BuildRelationExtStatistics(Relation onerel, double totalrows, continue; /* evaluate expressions (if the statistics has any) */ - exprs = evaluate_expressions(onerel, stat->exprs, numrows, rows); + data = make_build_data(onerel, stat, numrows, rows, stats); /* compute statistic of each requested type */ foreach(lc2, stat->types) @@ -199,16 +202,11 @@ BuildRelationExtStatistics(Relation onerel, double totalrows, char t = (char) lfirst_int(lc2); if (t == STATS_EXT_NDISTINCT) - ndistinct = statext_ndistinct_build(totalrows, numrows, rows, - exprs, stat->columns, - stats); + ndistinct = statext_ndistinct_build(totalrows, data); else if (t == STATS_EXT_DEPENDENCIES) - dependencies = statext_dependencies_build(numrows, rows, - exprs, stat->columns, - stats); + dependencies = statext_dependencies_build(data); else if (t == STATS_EXT_MCV) - mcv = statext_mcv_build(numrows, rows, exprs, stat->columns, - stats, totalrows, stattarget); + mcv = statext_mcv_build(data, totalrows, stattarget); else if (t == STATS_EXT_EXPRESSIONS) { AnlExprData *exprdata; @@ -236,7 +234,8 @@ BuildRelationExtStatistics(Relation onerel, double totalrows, pgstat_progress_update_param(PROGRESS_ANALYZE_EXT_STATS_COMPUTED, ++ext_cnt); - pfree(exprs); + /* free the build data (allocated as a single chunk) */ + pfree(data); } table_close(pg_stext, RowExclusiveLock); @@ -937,30 +936,31 @@ build_attnums_array(Bitmapset *attrs, int nexprs, int *numattrs) * can simply pfree the return value to release all of it. */ SortItem * -build_sorted_items(int numrows, int *nitems, HeapTuple *rows, ExprInfo *exprs, - TupleDesc tdesc, MultiSortSupport mss, +build_sorted_items(StatBuildData *data, int *nitems, + MultiSortSupport mss, int numattrs, AttrNumber *attnums) { int i, j, len, - idx; - int nvalues = numrows * numattrs; + nrows; + int nvalues = data->numrows * numattrs; SortItem *items; Datum *values; bool *isnull; char *ptr; + int *typlen; /* Compute the total amount of memory we need (both items and values). */ - len = numrows * sizeof(SortItem) + nvalues * (sizeof(Datum) + sizeof(bool)); + len = data->numrows * sizeof(SortItem) + nvalues * (sizeof(Datum) + sizeof(bool)); /* Allocate the memory and split it into the pieces. */ ptr = palloc0(len); /* items to sort */ items = (SortItem *) ptr; - ptr += numrows * sizeof(SortItem); + ptr += data->numrows * sizeof(SortItem); /* values and null flags */ values = (Datum *) ptr; @@ -973,13 +973,24 @@ build_sorted_items(int numrows, int *nitems, HeapTuple *rows, ExprInfo *exprs, Assert((ptr - (char *) items) == len); /* fix the pointers to Datum and bool arrays */ - idx = 0; - for (i = 0; i < numrows; i++) + nrows = 0; + for (i = 0; i < data->numrows; i++) { - bool toowide = false; + items[nrows].values = &values[nrows * numattrs]; + items[nrows].isnull = &isnull[nrows * numattrs]; + + nrows++; + } + + /* build a local cache of typlen for all attributes */ + typlen = (int *) palloc(sizeof(int) * data->nattnums); + for (i = 0; i < data->nattnums; i++) + typlen[i] = get_typlen(data->stats[i]->attrtypid); - items[idx].values = &values[idx * numattrs]; - items[idx].isnull = &isnull[idx * numattrs]; + nrows = 0; + for (i = 0; i < data->numrows; i++) + { + bool toowide = false; /* load the values/null flags from sample rows */ for (j = 0; j < numattrs; j++) @@ -989,22 +1000,20 @@ build_sorted_items(int numrows, int *nitems, HeapTuple *rows, ExprInfo *exprs, int attlen; AttrNumber attnum = attnums[j]; - if (AttrNumberIsForUserDefinedAttr(attnum)) + int idx; + + /* match attnum to the pre-calculated data */ + for (idx = 0; idx < data->nattnums; idx++) { - value = heap_getattr(rows[i], attnum, tdesc, &isnull); - attlen = TupleDescAttr(tdesc, attnum - 1)->attlen; + if (attnum == data->attnums[idx]) + break; } - else - { - int idx = -(attnums[j] + 1); - - Assert((idx >= 0) && (idx < exprs->nexprs)); - value = exprs->values[idx][i]; - isnull = exprs->nulls[idx][i]; + Assert(idx < data->nattnums); - attlen = get_typlen(exprs->types[idx]); - } + value = data->values[idx][i]; + isnull = data->nulls[idx][i]; + attlen = typlen[idx]; /* * If this is a varlena value, check if it's too wide and if yes @@ -1026,21 +1035,21 @@ build_sorted_items(int numrows, int *nitems, HeapTuple *rows, ExprInfo *exprs, value = PointerGetDatum(PG_DETOAST_DATUM(value)); } - items[idx].values[j] = value; - items[idx].isnull[j] = isnull; + items[nrows].values[j] = value; + items[nrows].isnull[j] = isnull; } if (toowide) continue; - idx++; + nrows++; } /* store the actual number of items (ignoring the too-wide ones) */ - *nitems = idx; + *nitems = nrows; /* all items were too wide */ - if (idx == 0) + if (nrows == 0) { /* everything is allocated as a single chunk */ pfree(items); @@ -1048,7 +1057,7 @@ build_sorted_items(int numrows, int *nitems, HeapTuple *rows, ExprInfo *exprs, } /* do the sort, using the multi-sort */ - qsort_arg((void *) items, idx, sizeof(SortItem), + qsort_arg((void *) items, nrows, sizeof(SortItem), multi_sort_compare, mss); return items; @@ -2434,59 +2443,61 @@ statext_expressions_load(Oid stxoid, int idx) * all the requested statistics types. This matters especially for * expensive expressions, of course. */ -static ExprInfo * -evaluate_expressions(Relation rel, List *exprs, int numrows, HeapTuple *rows) +static StatBuildData * +make_build_data(Relation rel, StatExtEntry *stat, int numrows, HeapTuple *rows, + VacAttrStats **stats) { /* evaluated expressions */ - ExprInfo *result; + StatBuildData *result; char *ptr; Size len; int i; + int k; int idx; TupleTableSlot *slot; EState *estate; ExprContext *econtext; List *exprstates = NIL; - int nexprs = list_length(exprs); + int nkeys = bms_num_members(stat->columns) + list_length(stat->exprs); ListCell *lc; /* allocate everything as a single chunk, so we can free it easily */ - len = MAXALIGN(sizeof(ExprInfo)); - len += MAXALIGN(sizeof(Oid) * nexprs); /* types */ - len += MAXALIGN(sizeof(Oid) * nexprs); /* collations */ + len = MAXALIGN(sizeof(StatBuildData)); + len += MAXALIGN(sizeof(AttrNumber) * nkeys); /* attnums */ + len += MAXALIGN(sizeof(VacAttrStats *) * nkeys); /* stats */ /* values */ - len += MAXALIGN(sizeof(Datum *) * nexprs); - len += nexprs * MAXALIGN(sizeof(Datum) * numrows); + len += MAXALIGN(sizeof(Datum *) * nkeys); + len += nkeys * MAXALIGN(sizeof(Datum) * numrows); /* nulls */ - len += MAXALIGN(sizeof(bool *) * nexprs); - len += nexprs * MAXALIGN(sizeof(bool) * numrows); + len += MAXALIGN(sizeof(bool *) * nkeys); + len += nkeys * MAXALIGN(sizeof(bool) * numrows); ptr = palloc(len); /* set the pointers */ - result = (ExprInfo *) ptr; - ptr += MAXALIGN(sizeof(ExprInfo)); + result = (StatBuildData *) ptr; + ptr += MAXALIGN(sizeof(StatBuildData)); - /* types */ - result->types = (Oid *) ptr; - ptr += MAXALIGN(sizeof(Oid) * nexprs); + /* attnums */ + result->attnums = (AttrNumber *) ptr; + ptr += MAXALIGN(sizeof(AttrNumber) * nkeys); - /* collations */ - result->collations = (Oid *) ptr; - ptr += MAXALIGN(sizeof(Oid) * nexprs); + /* stats */ + result->stats = (VacAttrStats **) ptr; + ptr += MAXALIGN(sizeof(VacAttrStats *) * nkeys); /* values */ result->values = (Datum **) ptr; - ptr += MAXALIGN(sizeof(Datum *) * nexprs); + ptr += MAXALIGN(sizeof(Datum *) * nkeys); /* nulls */ result->nulls = (bool **) ptr; - ptr += MAXALIGN(sizeof(bool *) * nexprs); + ptr += MAXALIGN(sizeof(bool *) * nkeys); - for (i = 0; i < nexprs; i++) + for (i = 0; i < nkeys; i++) { result->values[i] = (Datum *) ptr; ptr += MAXALIGN(sizeof(Datum) * numrows); @@ -2497,17 +2508,46 @@ evaluate_expressions(Relation rel, List *exprs, int numrows, HeapTuple *rows) Assert((ptr - (char *) result) == len); - result->nexprs = list_length(exprs); + /* we have it allocated, so let's fill the values */ + result->nattnums = nkeys; + result->numrows = numrows; + /* fill the attribute info - first attributes, then expressions */ idx = 0; - foreach (lc, exprs) + k = -1; + while ((k = bms_next_member(stat->columns, k)) >= 0) + { + result->attnums[idx] = k; + result->stats[idx] = stats[idx]; + + idx++; + } + + k = -1; + foreach (lc, stat->exprs) { Node *expr = (Node *) lfirst(lc); - result->types[idx] = exprType(expr); - result->collations[idx] = exprCollation(expr); + result->attnums[idx] = k; + result->stats[idx] = examine_expression(expr); idx++; + k--; + } + + /* first extract values for all the regular attributes */ + for (i = 0; i < numrows; i++) + { + idx = 0; + k = -1; + while ((k = bms_next_member(stat->columns, k)) >= 0) + { + result->values[idx][i] = heap_getattr(rows[i], k, + result->stats[idx]->tupDesc, + &result->nulls[idx][i]); + + idx++; + } } /* @@ -2526,7 +2566,7 @@ evaluate_expressions(Relation rel, List *exprs, int numrows, HeapTuple *rows) econtext->ecxt_scantuple = slot; /* Set up expression evaluation state */ - exprstates = ExecPrepareExprList(exprs, estate); + exprstates = ExecPrepareExprList(stat->exprs, estate); for (i = 0; i < numrows; i++) { @@ -2539,7 +2579,7 @@ evaluate_expressions(Relation rel, List *exprs, int numrows, HeapTuple *rows) /* Set up for predicate or expression evaluation */ ExecStoreHeapTuple(rows[i], slot, false); - idx = 0; + idx = bms_num_members(stat->columns); foreach (lc, exprstates) { Datum datum; diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c index 323d476814..844ba6f71f 100644 --- a/src/backend/statistics/mcv.c +++ b/src/backend/statistics/mcv.c @@ -74,8 +74,7 @@ ((ndims) * sizeof(DimensionInfo)) + \ ((nitems) * ITEM_SIZE(ndims))) -static MultiSortSupport build_mss(VacAttrStats **stats, int numattrs, - ExprInfo *exprs); +static MultiSortSupport build_mss(StatBuildData *data); static SortItem *build_distinct_groups(int numrows, SortItem *items, MultiSortSupport mss, int *ndistinct); @@ -182,16 +181,11 @@ get_mincount_for_mcv_list(int samplerows, double totalrows) * */ MCVList * -statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs, - Bitmapset *attrs, VacAttrStats **stats, - double totalrows, int stattarget) +statext_mcv_build(StatBuildData *data, double totalrows, int stattarget) { int i, - k, - numattrs, ngroups, nitems; - AttrNumber *attnums; double mincount; SortItem *items; SortItem *groups; @@ -199,38 +193,11 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs, MultiSortSupport mss; /* comparator for all the columns */ - mss = build_mss(stats, bms_num_members(attrs), exprs); - - /* - * treat expressions as special attributes with high attnums - * - * XXX We do this after build_mss, because that expects the bitmapset - * to only contain simple attributes (with a matching VacAttrStats) - */ - - /* - * 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)); - + mss = build_mss(data); /* sort the rows */ - items = build_sorted_items(numrows, &nitems, rows, exprs, - stats[0]->tupDesc, mss, numattrs, attnums); + items = build_sorted_items(data, &nitems, mss, + data->nattnums, data->attnums); if (!items) return NULL; @@ -265,7 +232,7 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs, * using get_mincount_for_mcv_list() and then keep all items that seem to * be more common than that. */ - mincount = get_mincount_for_mcv_list(numrows, totalrows); + mincount = get_mincount_for_mcv_list(data->numrows, totalrows); /* * Walk the groups until we find the first group with a count below the @@ -301,7 +268,7 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs, + sizeof(SortSupportData)); /* compute frequencies for values in each column */ - nfreqs = (int *) palloc0(sizeof(int) * numattrs); + nfreqs = (int *) palloc0(sizeof(int) * data->nattnums); freqs = build_column_frequencies(groups, ngroups, mss, nfreqs); /* @@ -312,12 +279,12 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs, mcvlist->magic = STATS_MCV_MAGIC; mcvlist->type = STATS_MCV_TYPE_BASIC; - mcvlist->ndimensions = numattrs; + mcvlist->ndimensions = data->nattnums; mcvlist->nitems = nitems; /* store info about data type OIDs */ - for (i = 0; i < numattrs; i++) - mcvlist->types[i] = stats[i]->attrtypid; + for (i = 0; i < data->nattnums; i++) + mcvlist->types[i] = data->stats[i]->attrtypid; /* Copy the first chunk of groups into the result. */ for (i = 0; i < nitems; i++) @@ -325,22 +292,22 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs, /* just pointer to the proper place in the list */ MCVItem *item = &mcvlist->items[i]; - item->values = (Datum *) palloc(sizeof(Datum) * numattrs); - item->isnull = (bool *) palloc(sizeof(bool) * numattrs); + item->values = (Datum *) palloc(sizeof(Datum) * data->nattnums); + item->isnull = (bool *) palloc(sizeof(bool) * data->nattnums); /* copy values for the group */ - memcpy(item->values, groups[i].values, sizeof(Datum) * numattrs); - memcpy(item->isnull, groups[i].isnull, sizeof(bool) * numattrs); + memcpy(item->values, groups[i].values, sizeof(Datum) * data->nattnums); + memcpy(item->isnull, groups[i].isnull, sizeof(bool) * data->nattnums); /* groups should be sorted by frequency in descending order */ Assert((i == 0) || (groups[i - 1].count >= groups[i].count)); /* group frequency */ - item->frequency = (double) groups[i].count / numrows; + item->frequency = (double) groups[i].count / data->numrows; /* base frequency, if the attributes were independent */ item->base_frequency = 1.0; - for (j = 0; j < numattrs; j++) + for (j = 0; j < data->nattnums; j++) { SortItem *freq; @@ -356,7 +323,7 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs, sizeof(SortItem), multi_sort_compare, tmp); - item->base_frequency *= ((double) freq->count) / numrows; + item->base_frequency *= ((double) freq->count) / data->numrows; } } @@ -375,17 +342,17 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs, * build MultiSortSupport for the attributes passed in attrs */ static MultiSortSupport -build_mss(VacAttrStats **stats, int numattrs, ExprInfo *exprs) +build_mss(StatBuildData *data) { int i; /* Sort by multiple columns (using array of SortSupport) */ - MultiSortSupport mss = multi_sort_init(numattrs + exprs->nexprs); + MultiSortSupport mss = multi_sort_init(data->nattnums); /* prepare the sort functions for all the attributes */ - for (i = 0; i < numattrs; i++) + for (i = 0; i < data->nattnums; i++) { - VacAttrStats *colstat = stats[i]; + VacAttrStats *colstat = data->stats[i]; TypeCacheEntry *type; type = lookup_type_cache(colstat->attrtypid, TYPECACHE_LT_OPR); @@ -396,20 +363,6 @@ build_mss(VacAttrStats **stats, int numattrs, ExprInfo *exprs) multi_sort_add_dimension(mss, i, type->lt_opr, colstat->attrcollid); } - /* prepare the sort functions for all the expressions */ - for (i = 0; i < exprs->nexprs; i++) - { - TypeCacheEntry *type; - - type = lookup_type_cache(exprs->types[i], TYPECACHE_LT_OPR); - if (type->lt_opr == InvalidOid) /* shouldn't happen */ - elog(ERROR, "cache lookup failed for ordering operator for type %u", - exprs->types[i]); - - multi_sort_add_dimension(mss, numattrs + i, type->lt_opr, - exprs->collations[i]); - } - return mss; } diff --git a/src/backend/statistics/mvdistinct.c b/src/backend/statistics/mvdistinct.c index 5e796e7123..7ca59d9785 100644 --- a/src/backend/statistics/mvdistinct.c +++ b/src/backend/statistics/mvdistinct.c @@ -36,9 +36,7 @@ #include "utils/syscache.h" #include "utils/typcache.h" -static double ndistinct_for_combination(double totalrows, int numrows, - HeapTuple *rows, ExprInfo *exprs, - int nattrs, VacAttrStats **stats, +static double ndistinct_for_combination(double totalrows, StatBuildData *data, int k, int *combination); static double estimate_ndistinct(double totalrows, int numrows, int d, int f1); static int n_choose_k(int n, int k); @@ -88,17 +86,12 @@ static void generate_combinations(CombinationGenerator *state); * allow using Bitmapsets. */ MVNDistinct * -statext_ndistinct_build(double totalrows, int numrows, HeapTuple *rows, - ExprInfo *exprs, Bitmapset *attrs, - VacAttrStats **stats) +statext_ndistinct_build(double totalrows, StatBuildData *data) { MVNDistinct *result; - int i; int k; int itemcnt; - int numattrs = bms_num_members(attrs); - int numcombs = num_combinations(numattrs + exprs->nexprs); - Bitmapset *tmp = NULL; + int numcombs = num_combinations(data->nattnums); result = palloc(offsetof(MVNDistinct, items) + numcombs * sizeof(MVNDistinctItem)); @@ -106,38 +99,14 @@ statext_ndistinct_build(double totalrows, int numrows, HeapTuple *rows, result->type = STATS_NDISTINCT_TYPE_BASIC; result->nitems = numcombs; - /* - * 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); - itemcnt = 0; - for (k = 2; k <= bms_num_members(attrs); k++) + for (k = 2; k <= data->nattnums; k++) { int *combination; CombinationGenerator *generator; /* generate combinations of K out of N elements */ - generator = generator_init(bms_num_members(attrs), k); + generator = generator_init(data->nattnums, k); while ((combination = generator_next(generator))) { @@ -147,36 +116,16 @@ statext_ndistinct_build(double totalrows, int numrows, HeapTuple *rows, item->attributes = palloc(sizeof(AttrNumber) * k); item->nattributes = k; + /* translate the indexes to attnums */ for (j = 0; j < k; j++) { - AttrNumber attnum = InvalidAttrNumber; - - /* - * 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] >= 0); - Assert(combination[j] < exprs->nexprs); - - attnum = -(combination[j] + 1); - } - - Assert(attnum != InvalidAttrNumber); - - item->attributes[j] = attnum; + item->attributes[j] = data->attnums[combination[j]]; + + Assert(AttributeNumberIsValid(item->attributes[j])); } item->ndistinct = - ndistinct_for_combination(totalrows, numrows, rows, - exprs, numattrs, - stats, k, combination); + ndistinct_for_combination(totalrows, data, k, combination); itemcnt++; Assert(itemcnt <= result->nitems); @@ -471,9 +420,8 @@ pg_ndistinct_send(PG_FUNCTION_ARGS) * combination of multiple columns. */ static double -ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows, - ExprInfo *exprs, int nattrs, - VacAttrStats **stats, int k, int *combination) +ndistinct_for_combination(double totalrows, StatBuildData *data, + int k, int *combination) { int i, j; @@ -493,11 +441,11 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows, * using the specified column combination as dimensions. We could try to * sort in place, but it'd probably be more complex and bug-prone. */ - items = (SortItem *) palloc(numrows * sizeof(SortItem)); - values = (Datum *) palloc0(sizeof(Datum) * numrows * k); - isnull = (bool *) palloc0(sizeof(bool) * numrows * k); + items = (SortItem *) palloc(data->numrows * sizeof(SortItem)); + values = (Datum *) palloc0(sizeof(Datum) * data->numrows * k); + isnull = (bool *) palloc0(sizeof(bool) * data->numrows * k); - for (i = 0; i < numrows; i++) + for (i = 0; i < data->numrows; i++) { items[i].values = &values[i * k]; items[i].isnull = &isnull[i * k]; @@ -514,24 +462,11 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows, { Oid typid; TypeCacheEntry *type; - AttrNumber attnum = InvalidAttrNumber; - TupleDesc tdesc = NULL; Oid collid = InvalidOid; + VacAttrStats *colstat = data->stats[combination[i]]; - /* first nexprs indexes are for expressions, then regular attributes */ - if (combination[i] >= exprs->nexprs) - { - VacAttrStats *colstat = stats[combination[i] - exprs->nexprs]; - typid = colstat->attrtypid; - attnum = colstat->attr->attnum; - collid = colstat->attrcollid; - tdesc = colstat->tupDesc; - } - else - { - typid = exprs->types[combination[i]]; - collid = exprs->collations[combination[i]]; - } + typid = colstat->attrtypid; + collid = colstat->attrcollid; type = lookup_type_cache(typid, TYPECACHE_LT_OPR); if (type->lt_opr == InvalidOid) /* shouldn't happen */ @@ -542,38 +477,15 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows, multi_sort_add_dimension(mss, i, type->lt_opr, collid); /* accumulate all the data for this dimension into the arrays */ - for (j = 0; j < numrows; j++) + for (j = 0; j < data->numrows; j++) { - /* - * 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] >= exprs->nexprs) - items[j].values[i] = - heap_getattr(rows[j], - attnum, - tdesc, - &items[j].isnull[i]); - else - { - /* 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)); - - items[j].values[i] = exprs->values[idx][j]; - items[j].isnull[i] = exprs->nulls[idx][j]; - } + items[j].values[i] = data->values[combination[i]][j]; + items[j].isnull[i] = data->nulls[combination[i]][j]; } } /* We can sort the array now ... */ - qsort_arg((void *) items, numrows, sizeof(SortItem), + qsort_arg((void *) items, data->numrows, sizeof(SortItem), multi_sort_compare, mss); /* ... and count the number of distinct combinations */ @@ -581,7 +493,7 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows, f1 = 0; cnt = 1; d = 1; - for (i = 1; i < numrows; i++) + for (i = 1; i < data->numrows; i++) { if (multi_sort_compare(&items[i], &items[i - 1], mss) != 0) { @@ -598,7 +510,7 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows, if (cnt == 1) f1 += 1; - return estimate_ndistinct(totalrows, numrows, d, f1); + return estimate_ndistinct(totalrows, data->numrows, d, f1); } /* The Duj1 estimator (already used in analyze.c). */ diff --git a/src/include/statistics/extended_stats_internal.h b/src/include/statistics/extended_stats_internal.h index 1f09799deb..7acf82aa0e 100644 --- a/src/include/statistics/extended_stats_internal.h +++ b/src/include/statistics/extended_stats_internal.h @@ -57,35 +57,26 @@ typedef struct SortItem int count; } SortItem; -/* - * Used to pass pre-computed information about expressions the stats - * object is defined on. - */ -typedef struct ExprInfo -{ - int nexprs; /* number of expressions */ - Oid *collations; /* collation for each expression */ - Oid *types; /* type of each expression */ - Datum **values; /* values for each expression */ - bool **nulls; /* nulls for each expression */ -} ExprInfo; - -extern MVNDistinct *statext_ndistinct_build(double totalrows, - int numrows, HeapTuple *rows, - ExprInfo *exprs, Bitmapset *attrs, - VacAttrStats **stats); +/* a unified representation of the data the statistics is built on */ +typedef struct StatBuildData { + int numrows; + int nattnums; + AttrNumber *attnums; + VacAttrStats **stats; + Datum **values; + bool **nulls; +} StatBuildData; + + +extern MVNDistinct *statext_ndistinct_build(double totalrows, StatBuildData *data); extern bytea *statext_ndistinct_serialize(MVNDistinct *ndistinct); extern MVNDistinct *statext_ndistinct_deserialize(bytea *data); -extern MVDependencies *statext_dependencies_build(int numrows, HeapTuple *rows, - ExprInfo *exprs, Bitmapset *attrs, - VacAttrStats **stats); +extern MVDependencies *statext_dependencies_build(StatBuildData *data); extern bytea *statext_dependencies_serialize(MVDependencies *dependencies); extern MVDependencies *statext_dependencies_deserialize(bytea *data); -extern MCVList *statext_mcv_build(int numrows, HeapTuple *rows, - ExprInfo *exprs, Bitmapset *attrs, - VacAttrStats **stats, +extern MCVList *statext_mcv_build(StatBuildData *data, double totalrows, int stattarget); extern bytea *statext_mcv_serialize(MCVList *mcv, VacAttrStats **stats); extern MCVList *statext_mcv_deserialize(bytea *data); @@ -108,8 +99,7 @@ extern void *bsearch_arg(const void *key, const void *base, 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, +extern SortItem *build_sorted_items(StatBuildData *data, int *nitems, MultiSortSupport mss, int numattrs, AttrNumber *attnums); -- 2.26.2 --------------614DDB87AFFED893713AC0E9-- ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set @ 2022-02-13 04:50 Andres Freund <[email protected]> 0 siblings, 2 replies; 11+ messages in thread From: Andres Freund @ 2022-02-13 04:50 UTC (permalink / raw) To: Michael Paquier <[email protected]>; Thomas Munro <[email protected]>; Noah Misch <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]> Hi, On 2022-01-18 11:20:16 +0900, Michael Paquier wrote: > +# required for 002_pg_upgrade.pl > +REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX) > +export REGRESS_SHLIB It seems weird to propagate this into multiple places. Why don't we define that centrally? Although it's weird for this to use REGRESS_SHLIB, given it's just doing dirname() on it. 027_stream_regress.pl has the "defense" of not wanting to duplicate the variable with 017_shm.pl... Not that I understand why 017_shm.pl and all the regression test source fileseven need $(DLSUFFIX) - expand_dynamic_library_name() should take care of it? > +REGRESS_OUTPUTDIR=$(abs_top_builddir)/src/bin/pg_upgrade > +export REGRESS_OUTPUTDIR I don't really understand why 027_stream_regress.pl is using this (and thus not why it's used here). The tap tests create files all the time, why is this different? It's not like make / msvc put the data in different places: src/test/recovery/Makefile:REGRESS_OUTPUTDIR=$(abs_top_builddir)/src/test/recovery/tmp_check src/tools/msvc/vcregress.pl: $ENV{REGRESS_OUTPUTDIR} = "$topdir/src/test/recovery/tmp_check"; > + > +# From now on, the test of pg_upgrade consists in setting up an instance. What does "from now on" mean? > +# Default is the location of this source code for both nodes used with > +# the upgrade. Can't quite parse. > +# Initialize a new node for the upgrade. This is done early so as it is > +# possible to know with which node's PATH the initial dump needs to be > +# taken. > +my $newnode = PostgreSQL::Test::Cluster->new('new_node'); > +$newnode->init(extra => [ '--locale=C', '--encoding=LATIN1' ]); > +my $newbindir = $newnode->config_data('--bindir'); > +my $oldbindir = $oldnode->config_data('--bindir'); Why C/LATIN? Right now pg_upgrade test.sh uses --wal-segsize 1, and that has helped identify several bugs. So I'd rather not give it up, even if it's a bit weird. > + my @regress_command = [ > + $ENV{PG_REGRESS}, > + '--schedule', "$oldsrc/src/test/regress/parallel_schedule", > + '--bindir', $oldnode->config_data('--bindir'), > + '--dlpath', $dlpath, > + '--port', $oldnode->port, > + '--outputdir', $outputdir, > + '--inputdir', $inputdir, > + '--use-existing' > + ]; I think this should use --host (c.f. 7340aceed72). Or is it intending to use the host via env? If so, why is the port specified? > + @regress_command = (@regress_command, @extra_opts); > + > + $oldnode->command_ok(@regress_command, > + 'regression test run on old instance'); I also think this should take EXTRA_REGRESS_OPTS into account - test.sh did. > +# After dumping, update references to the old source tree's regress.so > +# to point to the new tree. > +if (defined($ENV{oldinstall})) > +{ Kinda asking for its own function... > + > +# Update the instance. > +$oldnode->stop; > + > +# Time for the real run. As opposed to the unreal one? > +# pg_upgrade would complain if PGHOST, so as there are no attempts to > +# connect to a different server than the upgraded ones. "complain if PGHOST"? > +# Take a second dump on the upgraded instance. Sounds like you're taking to post-upgrade pg_dumps. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set @ 2022-02-13 05:07 Thomas Munro <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 2 replies; 11+ messages in thread From: Thomas Munro @ 2022-02-13 05:07 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]> On Sun, Feb 13, 2022 at 5:50 PM Andres Freund <[email protected]> wrote: > On 2022-01-18 11:20:16 +0900, Michael Paquier wrote: > > +REGRESS_OUTPUTDIR=$(abs_top_builddir)/src/bin/pg_upgrade > > +export REGRESS_OUTPUTDIR > > I don't really understand why 027_stream_regress.pl is using this (and thus > not why it's used here). The tap tests create files all the time, why is this > different? > > It's not like make / msvc put the data in different places: > src/test/recovery/Makefile:REGRESS_OUTPUTDIR=$(abs_top_builddir)/src/test/recovery/tmp_check > src/tools/msvc/vcregress.pl: $ENV{REGRESS_OUTPUTDIR} = "$topdir/src/test/recovery/tmp_check"; As I wrote in https://www.postgresql.org/message-id/CA%2BhUKGK-%2Bmg6RWiDu0JudF6jWeL5%2BgPmi8EKUm1eAzmdbwiE_A%40ma..., >> > +# required for 027_stream_regress.pl >> > +REGRESS_OUTPUTDIR=$(abs_top_builddir)/src/test/recovery >> > +export REGRESS_OUTPUTDIR >> >> Why do we need this? > > The Make macro "prove_check" (src/Makefile.global.in) always changes > to the source directory to run TAP tests. Without an explicit > directive to control where regression test output goes, it got > splattered all over the source tree in VPATH builds. I didn't see an > existing way to adjust that (did I miss something?). Hence desire to > pass down a path in the build tree. Better ideas welcome. I thought it was a goal that VPATH builds shouldn't pollute the source tree, but the Make macro prove_check is explicitly doing so by default. Perhaps *that* should be fixed? ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set @ 2022-02-13 05:13 Tom Lane <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 0 replies; 11+ messages in thread From: Tom Lane @ 2022-02-13 05:13 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]> Thomas Munro <[email protected]> writes: > I thought it was a goal that VPATH builds shouldn't pollute the source > tree, but the Make macro prove_check is explicitly doing so by > default. Perhaps *that* should be fixed? Indeed. That seems broken by definition. More generally, I thought we'd established a convention that all files made by TAP tests should be put inside the tmp_check directory, to simplify cleanup and .gitignore rules. But in a VPATH build, tmp_check ought to live in the build tree. regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set @ 2022-02-13 05:29 Andres Freund <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Andres Freund @ 2022-02-13 05:29 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Michael Paquier <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]> Hi, On 2022-02-13 18:07:30 +1300, Thomas Munro wrote: > On Sun, Feb 13, 2022 at 5:50 PM Andres Freund <[email protected]> wrote: > >> > +# required for 027_stream_regress.pl > >> > +REGRESS_OUTPUTDIR=$(abs_top_builddir)/src/test/recovery > >> > +export REGRESS_OUTPUTDIR > >> > >> Why do we need this? > > > > The Make macro "prove_check" (src/Makefile.global.in) always changes > > to the source directory to run TAP tests. Without an explicit > > directive to control where regression test output goes, it got > > splattered all over the source tree in VPATH builds. I didn't see an > > existing way to adjust that (did I miss something?). Hence desire to > > pass down a path in the build tree. Better ideas welcome. > > I thought it was a goal that VPATH builds shouldn't pollute the source > tree, but the Make macro prove_check is explicitly doing so by > default. Perhaps *that* should be fixed? Sure, prove changes into the source dir. But we don't put test data / output into the source? That's what TESTDIR is used for: # Determine output directories, and create them. The base path is the # TESTDIR environment variable, which is normally set by the invoking # Makefile. $tmp_check = $ENV{TESTDIR} ? "$ENV{TESTDIR}/tmp_check" : "tmp_check"; $log_path = "$tmp_check/log"; Afaics all the "regress test inside tap test" cases would need to do is to pass --outputdir=${PostgreSQL::Test::Utils::tmp_check} and you'd get exactly the same path as REGRESS_OUTPUTDIR currently provides. I only use vpath builds, and I don't see any tap test data / log in the source tree.... Greetings, Andres Freund ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set @ 2022-02-13 22:23 Thomas Munro <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Thomas Munro @ 2022-02-13 22:23 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]> On Sun, Feb 13, 2022 at 6:29 PM Andres Freund <[email protected]> wrote: > Afaics all the "regress test inside tap test" cases would need to do is to pass > --outputdir=${PostgreSQL::Test::Utils::tmp_check} and you'd get exactly the same path as > REGRESS_OUTPUTDIR currently provides. Ahh, right. I assume it still needs perl2host() treatment for MSYS2 systems, because jacana's log shows TESTDIR is set to a Unixoid path that I assume pg_regress's runtime can't use. That leads to the attached. Attachments: [application/octet-stream] 0001-Remove-REGRESS_OUTPUTDIR-environment-variable.not-for-cfbot-patch (2.2K, ../../CA+hUKG+z7fpFOHRZfRn9iJKuCK5h-Nyfj+evFSMGu9nt=2TwmA@mail.gmail.com/2-0001-Remove-REGRESS_OUTPUTDIR-environment-variable.not-for-cfbot-patch) download ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set @ 2022-02-13 22:55 Andres Freund <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Andres Freund @ 2022-02-13 22:55 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Michael Paquier <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]> On 2022-02-14 11:23:18 +1300, Thomas Munro wrote: > On Sun, Feb 13, 2022 at 6:29 PM Andres Freund <[email protected]> wrote: > > Afaics all the "regress test inside tap test" cases would need to do is to pass > > --outputdir=${PostgreSQL::Test::Utils::tmp_check} and you'd get exactly the same path as > > REGRESS_OUTPUTDIR currently provides. > > Ahh, right. I assume it still needs perl2host() treatment for MSYS2 > systems, because jacana's log shows TESTDIR is set to a Unixoid path > that I assume pg_regress's runtime can't use. That leads to the > attached. Looks sane to me. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set @ 2022-02-14 06:40 Michael Paquier <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Michael Paquier @ 2022-02-14 06:40 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]> On Sun, Feb 13, 2022 at 02:55:26PM -0800, Andres Freund wrote: > On 2022-02-14 11:23:18 +1300, Thomas Munro wrote: >> Ahh, right. I assume it still needs perl2host() treatment for MSYS2 >> systems, because jacana's log shows TESTDIR is set to a Unixoid path >> that I assume pg_regress's runtime can't use. That leads to the >> attached. > > Looks sane to me. This looks like a nice cleanup, indeed. Nice catch. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../Ygn5V7n4dmst2AO%[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set @ 2022-02-15 04:02 Michael Paquier <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Michael Paquier @ 2022-02-15 04:02 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]> On Sat, Feb 12, 2022 at 08:50:41PM -0800, Andres Freund wrote: > On 2022-01-18 11:20:16 +0900, Michael Paquier wrote: >> +# required for 002_pg_upgrade.pl >> +REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX) >> +export REGRESS_SHLIB > > It seems weird to propagate this into multiple places. Why don't we define > that centrally? > > Although it's weird for this to use REGRESS_SHLIB, given it's just doing > dirname() on it. 027_stream_regress.pl has the "defense" of not wanting to > duplicate the variable with 017_shm.pl... > > Not that I understand why 017_shm.pl and all the regression test source > fileseven need $(DLSUFFIX) - expand_dynamic_library_name() should take care of > it? I agree that we should be able to get rid of that in the long-term, but this also feels like a separate issue to me and the patch is already doing a lot. I am wondering about the interactions of installcheck with abs_top_builddir, though. Should it be addressed first? It does not feel like a mandatory requirement for this thread, anyway. > It's not like make / msvc put the data in different places: > src/test/recovery/Makefile:REGRESS_OUTPUTDIR=$(abs_top_builddir)/src/test/recovery/tmp_check > src/tools/msvc/vcregress.pl: $ENV{REGRESS_OUTPUTDIR} = "$topdir/src/test/recovery/tmp_check"; Yeah, removed. >> +# From now on, the test of pg_upgrade consists in setting up an instance. > > What does "from now on" mean? In this context, the next steps of the test. Removed. >> +# Default is the location of this source code for both nodes used with >> +# the upgrade. > > Can't quite parse. Reworded, to something hopefully better. >> +# Initialize a new node for the upgrade. This is done early so as it is >> +# possible to know with which node's PATH the initial dump needs to be >> +# taken. >> +my $newnode = PostgreSQL::Test::Cluster->new('new_node'); >> +$newnode->init(extra => [ '--locale=C', '--encoding=LATIN1' ]); >> +my $newbindir = $newnode->config_data('--bindir'); >> +my $oldbindir = $oldnode->config_data('--bindir'); > > Why C/LATIN? Well, these are bits from the patch that I have played with extensively, and it took me some time to remember why this was needed. The reason why I introduced this option is that the patch created the database "regression" using a createdb command that would feed from template1 as pg_regress used --use-existing. And this combination required to enforce --locale=C to avoid two regression diffs in int8.sql and numeric.sql. It is possible to simplify things by removing --use-existing and the database creation, so as pg_regress handles the creation of the database "regression" with template0 to avoid any problems related to locales. Now, if you do *not* do that, I have noticed that we run into problems when testing the TAP script with older versions, where pg_regress would may not create the "regression" database, hence requiring an extra createdb (perhaps that's better with --locale=C and --template=template0) with --use-existing present for the pg_regress command, command coming from the old branch. Hmm. At the end of the day, I am wondering whether we should not give up entirely on the concept of running the regression tests on older branches in the TAP script of a newer branch. pg_regress needs to come from the old source tree, meaning that we would most likely need to maintain a set of compatibility tweaks that would most probably rot over the time, and the buildfarm only cares about the possibility to set up old instances by loading dumps rather than running pg_regress. This would also make the switch to TAP much easier (no need for the extra createdb or --locale AFAIK). So attempting to maintain all that is going to be a PITA in the long term, and there is nothing running that automatically anyway. There is also the extra requirement to adjust dump files, but that's independent of setting up the old instance to upgrade, and I don't really plan to tackle that as of this thread (note that the buildfarm client has extra tweaks regarding that). Any thoughts about that? > Right now pg_upgrade test.sh uses --wal-segsize 1, and that has helped > identify several bugs. So I'd rather not give it up, even if it's a bit weird. --allow-group-access was missing as well. >> + my @regress_command = [ >> + $ENV{PG_REGRESS}, >> + '--schedule', "$oldsrc/src/test/regress/parallel_schedule", >> + '--bindir', $oldnode->config_data('--bindir'), >> + '--dlpath', $dlpath, >> + '--port', $oldnode->port, >> + '--outputdir', $outputdir, >> + '--inputdir', $inputdir, >> + '--use-existing' >> + ]; > > I think this should use --host (c.f. 7340aceed72). Or is it intending to use > the host via env? If so, why is the port specified? Hm. It looks like you are right here, so added. >> + @regress_command = (@regress_command, @extra_opts); >> + >> + $oldnode->command_ok(@regress_command, >> + 'regression test run on old instance'); > > I also think this should take EXTRA_REGRESS_OPTS into account - test.sh did. This is already taken into account, as of the @extra_opts bits. >> +# After dumping, update references to the old source tree's regress.so >> +# to point to the new tree. >> +if (defined($ENV{oldinstall})) >> +{ > > Kinda asking for its own function... I am not sure this is a gain in readability just for this part, FWIW, and once you drop support for setting up an old instance with pg_regress, that would not be needed. >> +# Update the instance. >> +$oldnode->stop; >> + >> +# Time for the real run. > > As opposed to the unreal one? Removed that. >> +# pg_upgrade would complain if PGHOST, so as there are no attempts to >> +# connect to a different server than the upgraded ones. > > "complain if PGHOST"? There is no need for this tweak once check_pghost_envvar() is fixed to be able to understand Windows paths. This was not working under the CI on Windows anyway, but the check_pghost_envvar() fix does. A last thing that was missing from the patch, AFAIK, is to scan the contents of pg_upgrade_output.d/log, if anything is left around after a failure so as the buildfarm is able to report all the logs. pg_upgrade's .gitignore has no need for a refresh, as well. I have split the patch set into two parts: - 0001 is a fix for check_pghost_envvar() with the addition of a call to is_unixsock_path() to make sure that Windows paths are handled. This has proved to be enough to make the CI report green on Windows. - 0002 is the test, with all the fixes and adjustments mentioned upthread, including making sure that the tests can be run with older branches, for now. -- Michael Attachments: [text/x-diff] v8-0001-Fix-sanity-check-for-PGHOST-ADDR-in-pg_upgrade-wi.patch (1.6K, ../../Ygsl4Q7MARNY%[email protected]/2-v8-0001-Fix-sanity-check-for-PGHOST-ADDR-in-pg_upgrade-wi.patch) download | inline diff: From 5a3b714f2b7e9aaa5efa23dbc103a8f057f54708 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Tue, 15 Feb 2022 12:05:28 +0900 Subject: [PATCH v8 1/2] Fix sanity check for PGHOST[ADDR] in pg_upgrade with Windows paths The checks currently done at the startup of pg_upgrade for PGHOST and PGHOSTADDR to avoid any attempt to access to an external cluster would fail when attempting to use Windows paths, or even temporary paths prefixed by '@'. is_unixsock_path() is designed to detect such cases, so use it rather than assuming that all valid paths are prefixed with a slash. Issue found while testing the tests of pg_upgrade through the CI on Windows. Based on an analysis from me and a solution from Andres Freund. --- src/bin/pg_upgrade/server.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c index 7878d233de..265137e86b 100644 --- a/src/bin/pg_upgrade/server.c +++ b/src/bin/pg_upgrade/server.c @@ -11,6 +11,7 @@ #include "common/connect.h" #include "fe_utils/string_utils.h" +#include "libpq/pqcomm.h" #include "pg_upgrade.h" static PGconn *get_db_conn(ClusterInfo *cluster, const char *db_name); @@ -368,7 +369,7 @@ check_pghost_envvar(void) if (value && strlen(value) > 0 && /* check for 'local' host values */ (strcmp(value, "localhost") != 0 && strcmp(value, "127.0.0.1") != 0 && - strcmp(value, "::1") != 0 && value[0] != '/')) + strcmp(value, "::1") != 0 && !is_unixsock_path(value))) pg_fatal("libpq environment variable %s has a non-local server value: %s\n", option->envvar, value); } -- 2.34.1 [text/x-diff] v8-0002-Switch-tests-of-pg_upgrade-to-use-TAP.patch (27.6K, ../../Ygsl4Q7MARNY%[email protected]/3-v8-0002-Switch-tests-of-pg_upgrade-to-use-TAP.patch) download | inline diff: From 5e63bc80e0513bb9bf916db59d9673c2b4fb2948 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Tue, 15 Feb 2022 12:52:50 +0900 Subject: [PATCH v8 2/2] Switch tests of pg_upgrade to use TAP --- src/bin/pg_upgrade/Makefile | 21 +- src/bin/pg_upgrade/TESTING | 33 ++- src/bin/pg_upgrade/t/001_basic.pl | 9 + src/bin/pg_upgrade/t/002_pg_upgrade.pl | 311 +++++++++++++++++++++++++ src/bin/pg_upgrade/test.sh | 279 ---------------------- src/tools/msvc/vcregress.pl | 92 +------- 6 files changed, 357 insertions(+), 388 deletions(-) create mode 100644 src/bin/pg_upgrade/t/001_basic.pl create mode 100644 src/bin/pg_upgrade/t/002_pg_upgrade.pl delete mode 100644 src/bin/pg_upgrade/test.sh diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile index 49b94f0ac7..1f5d757548 100644 --- a/src/bin/pg_upgrade/Makefile +++ b/src/bin/pg_upgrade/Makefile @@ -28,6 +28,10 @@ OBJS = \ override CPPFLAGS := -DDLSUFFIX=\"$(DLSUFFIX)\" -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS) LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport) +# required for 002_pg_upgrade.pl +REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX) +export REGRESS_SHLIB + all: pg_upgrade pg_upgrade: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils @@ -47,17 +51,8 @@ clean distclean maintainer-clean: rm -rf delete_old_cluster.sh log/ tmp_check/ \ reindex_hash.sql -# When $(MAKE) is present, make automatically infers that this is a -# recursive make. which is not actually what we want here, as that -# e.g. prevents output synchronization from working (as make thinks -# that the subsidiary make knows how to deal with that itself, but -# we're invoking a shell script that doesn't know). Referencing -# $(MAKE) indirectly avoids that behaviour. -# See https://www.gnu.org/software/make/manual/html_node/MAKE-Variable.html#MAKE-Variable -NOTSUBMAKEMAKE=$(MAKE) +check: + $(prove_check) -check: test.sh all temp-install - MAKE=$(NOTSUBMAKEMAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $< - -# installcheck is not supported because there's no meaningful way to test -# pg_upgrade against a single already-running server +installcheck: + $(prove_installcheck) diff --git a/src/bin/pg_upgrade/TESTING b/src/bin/pg_upgrade/TESTING index 78b9747908..43a71566e2 100644 --- a/src/bin/pg_upgrade/TESTING +++ b/src/bin/pg_upgrade/TESTING @@ -2,21 +2,30 @@ THE SHORT VERSION ----------------- On non-Windows machines, you can execute the testing process -described below by running +described below by running the following command in this directory: make check -in this directory. This will run the shell script test.sh, performing -an upgrade from the version in this source tree to a new instance of -the same version. -To test an upgrade from a different version, you must have a built -source tree for the old version as well as this version, and you -must have done "make install" for both versions. Then do: +This will run the TAP tests to run pg_upgrade, performing an upgrade +from the version in this source tree to a new instance of the same +version. + +To test an upgrade from a different version, there are two options +available: + +1) You have a built source tree for the old version as well as this +version's binaries. Then set up the following variables before +launching the test: export oldsrc=...somewhere/postgresql (old version's source tree) -export oldbindir=...otherversion/bin (old version's installed bin dir) -export bindir=...thisversion/bin (this version's installed bin dir) -export libdir=...thisversion/lib (this version's installed lib dir) -sh test.sh +export oldinstall=...otherversion/ (old version's install base path) + +2) You have a dump that can be used to set up the old version, as well +as this version's binaries. Then set up the following variables: +export olddump=...somewhere/dump.sql (old version's dump) +export oldinstall=...otherversion/ (old version's install base path) + +Finally, the tests can be done by running + make check In this case, you will have to manually eyeball the resulting dump diff for version-specific differences, as explained below. @@ -77,3 +86,5 @@ steps: 7) Diff the regression database dump file with the regression dump file loaded into the old server. + +The generated dump may be reusable with "olddump", as defined above. diff --git a/src/bin/pg_upgrade/t/001_basic.pl b/src/bin/pg_upgrade/t/001_basic.pl new file mode 100644 index 0000000000..75b0f98b08 --- /dev/null +++ b/src/bin/pg_upgrade/t/001_basic.pl @@ -0,0 +1,9 @@ +use strict; +use warnings; + +use PostgreSQL::Test::Utils; +use Test::More tests => 8; + +program_help_ok('pg_upgrade'); +program_version_ok('pg_upgrade'); +program_options_handling_ok('pg_upgrade'); diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl new file mode 100644 index 0000000000..94c5e45fe2 --- /dev/null +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl @@ -0,0 +1,311 @@ +# Set of tests for pg_upgrade, including cross-version checks. +use strict; +use warnings; + +use Cwd qw(abs_path getcwd); +use File::Basename qw(dirname); + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More tests => 4; + +# Generate a database with a name made of a range of ASCII characters. +sub generate_db +{ + my ($node, $from_char, $to_char) = @_; + + my $dbname = ''; + for my $i ($from_char .. $to_char) + { + next if $i == 7 || $i == 10 || $i == 13; # skip BEL, LF, and CR + $dbname = $dbname . sprintf('%c', $i); + } + $node->run_log( + [ 'createdb', '--host', $node->host, '--port', $node->port, $dbname ] + ); +} + +# The test of pg_upgrade consists in setting up an instance. This is the +# source instance used for the upgrade. Then a new and fresh instance is +# created, and is used as the target instance for the upgrade. Before +# running an upgrade, a logical dump of the old instance is taken, and a +# second logical dump of the new instance is taken after the upgrade. +# The upgrade test passes if there are no differences in these two dumps. + +# Testing upgrades with an older instance of PostgreSQL requires setting up +# two environment variables, among the following: +# - "oldsrc", to point to the code source of the older version. +# This is required to set up the old instance with pg_upgrade. +# - "olddump", to point to a dump file that will be used to set +# up the old instance to upgrade from. +# - "oldinstall", to point to the installation path of the older +# version. + +# "oldsrc" and "olddump" cannot be used together. Setting up +# "olddump" and "oldinstall" will use the dump pointed to to +# set up the old instance. If "oldsrc" is used instead of "olddump", +# the full set of regression tests of the old instance is run +# instead. + +if (defined($ENV{oldsrc}) && defined($ENV{olddump})) +{ + die "oldsrc and olddump are both defined"; +} +elsif (defined($ENV{oldsrc})) +{ + if ( (defined($ENV{oldsrc}) && !defined($ENV{oldinstall})) + || (!defined($ENV{oldsrc}) && defined($ENV{oldinstall}))) + { + # Not all variables are defined, so leave and die if test is + # done with an older installation. + die "oldsrc or oldinstall is undefined"; + } +} +elsif (defined($ENV{olddump})) +{ + if ( (defined($ENV{olddump}) && !defined($ENV{oldinstall})) + || (!defined($ENV{olddump}) && defined($ENV{oldinstall}))) + { + # Not all variables are defined, so leave and die if test is + # done with an older installation. + die "olddump or oldinstall is undefined"; + } +} + +if ((defined($ENV{oldsrc}) || defined($ENV{olddump})) && $windows_os) +{ + # This configuration is not supported on Windows, as regress.so + # location diverges across the compilation methods used on this + # platform. + die "No support for older version tests on Windows"; +} + +# The default location of the source code is the root of this directory for the +# new node. The source tree to use for the old node may be different. +my $newsrc = abs_path("../../.."); +my $oldsrc = $ENV{oldsrc} || $newsrc; +$oldsrc = abs_path($oldsrc); + +# Temporary location for the dumps taken +my $tempdir = PostgreSQL::Test::Utils::tempdir; + +# Initialize node to upgrade +my $oldnode = PostgreSQL::Test::Cluster->new('old_node', + install_path => $ENV{oldinstall}); + +$oldnode->init(extra => + [ '--wal-segsize', '1', '--allow-group-access', '--locale', 'C' ]); +$oldnode->start; + +# Set up the data of the old instance with pg_regress or an old dump. +if (defined($ENV{olddump})) +{ + # Use the dump specified. + my $olddumpfile = $ENV{olddump}; + die "no dump file found!" unless -e $olddumpfile; + + # Load the dump, and we are done here. + $oldnode->command_ok( + [ + 'psql', '-X', '-f', $olddumpfile, + '--port', $oldnode->port, 'regression' + ]); +} +else +{ + # Default is to just use pg_regress to set up the old instance + # Creating databases with names covering most ASCII bytes + generate_db($oldnode, 1, 45); + generate_db($oldnode, 46, 90); + generate_db($oldnode, 91, 127); + + # Grab any regression options that may be passed down by caller. + my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || ""; + my @extra_opts = split(/\s+/, $extra_opts_val); + + # Run core regression tests on the old instance. + $oldnode->run_log( + [ + "createdb", '--port', $oldnode->port, '--template', + 'template0', 'regression' + ]); + + # --dlpath is needed to be able to find the location of regress.so and + # any libraries the regression tests required. This needs to point to + # the old instance when using it. In the default case, fallback to + # what the caller provided for REGRESS_SHLIB. + my $dlpath; + + # --outputdir points to the path where to place the output files, which + # requires its location to be in the old cluster's source tree to allow + # pg_regress to work. + my $outputdir; + my $pg_regress; + if (defined($ENV{oldinstall})) + { + $dlpath = "$oldsrc/src/test/regress"; + $pg_regress = "$oldsrc/src/test/regress/pg_regress"; + $outputdir = "$oldsrc/src/test/regress"; + } + else + { + $dlpath = dirname($ENV{REGRESS_SHLIB}); + $pg_regress = $ENV{PG_REGRESS}; + $outputdir = $PostgreSQL::Test::Utils::tmp_check; + } + $dlpath = PostgreSQL::Test::Utils::perl2host($dlpath); + $outputdir = PostgreSQL::Test::Utils::perl2host($outputdir); + + # --inputdir needs to point to the location of the input files, from the + # cluster to-be-upgraded. + my $inputdir = + PostgreSQL::Test::Utils::perl2host("$oldsrc/src/test/regress"); + + my @regress_command = [ + $pg_regress, + '--dlpath', + $dlpath, + '--max-concurrent-tests', + '20', + '--bindir', + $oldnode->config_data('--bindir'), + '--host', + $oldnode->host, + '--port', + $oldnode->port, + '--schedule', + "$oldsrc/src/test/regress/parallel_schedule", + '--outputdir', + $outputdir, + '--inputdir', + $inputdir, + '--use-existing' + ]; + @regress_command = (@regress_command, @extra_opts); + + $oldnode->command_ok(@regress_command, + 'regression test run on old instance'); +} + +# Before dumping, get rid of objects not existing or not supported in later +# versions. This depends on the version of the old server used, and matters +# only if different versions are used for the dump. +if (defined($ENV{oldinstall})) +{ + # Note that upgrade_adapt.sql from the new version is used, to + # cope with an upgrade to this version. + $oldnode->run_log( + [ + 'psql', '-X', '-f', + PostgreSQL::Test::Utils::perl2host( + "$newsrc/src/bin/pg_upgrade/upgrade_adapt.sql"), + '--port', + $oldnode->port, + 'regression' + ]); +} + +# Initialize a new node for the upgrade. This is done early so as it is +# possible to know with which node's PATH the initial dump needs to be +# taken. +my $newnode = PostgreSQL::Test::Cluster->new('new_node'); +$newnode->init(extra => + [ '--wal-segsize', '1', '--allow-group-access', '--locale', 'C' ]); +my $newbindir = $newnode->config_data('--bindir'); +my $oldbindir = $oldnode->config_data('--bindir'); + +# Take a dump before performing the upgrade as a base comparison. Note +# that we need to use pg_dumpall from the new node here. +$newnode->command_ok( + [ + 'pg_dumpall', '--no-sync', + '-d', $oldnode->connstr('postgres'), + '-f', "$tempdir/dump1.sql" + ], + 'dump before running pg_upgrade'); + +# After dumping, update references to the old source tree's regress.so +# to point to the new tree. +if (defined($ENV{oldinstall})) +{ + # First, fetch all the references to libraries that are not part + # of the default path $libdir. + my $output = $oldnode->safe_psql('regression', + "SELECT DISTINCT probin::text FROM pg_proc WHERE probin NOT LIKE '\$libdir%';" + ); + chomp($output); + my @libpaths = split("\n", $output); + + my $dump_data = slurp_file("$tempdir/dump1.sql"); + + my $newregresssrc = "$newsrc/src/test/regress"; + foreach (@libpaths) + { + my $libpath = $_; + $libpath = dirname($libpath); + $dump_data =~ s/$libpath/$newregresssrc/g; + } + + open my $fh, ">", "$tempdir/dump1.sql" or die "could not open dump file"; + print $fh $dump_data; + close $fh; + + # This replaces any references to the old tree's regress.so + # the new tree's regress.so. Any references that do *not* + # match $libdir are switched so as this request does not + # depend on the path of the old source tree. This is useful + # when using an old dump. Do the operation on all the databases + # that allow connections so as this includes the regression + # database and anything the user has set up. + $output = $oldnode->safe_psql('postgres', + "SELECT datname FROM pg_database WHERE datallowconn;"); + chomp($output); + my @datnames = split("\n", $output); + foreach (@datnames) + { + my $datname = $_; + $oldnode->safe_psql( + $datname, "UPDATE pg_proc SET probin = + regexp_replace(probin, '.*/', '$newregresssrc/') + WHERE probin NOT LIKE '\$libdir/%'"); + } +} + +# Upgrade the instance. +$oldnode->stop; +command_ok( + [ + 'pg_upgrade', '--no-sync', '-d', $oldnode->data_dir, + '-D', $newnode->data_dir, '-b', $oldbindir, + '-B', $newbindir, '-p', $oldnode->port, + '-P', $newnode->port + ], + 'run of pg_upgrade for new instance'); +$newnode->start; + +# Check if there are any logs coming from pg_upgrade, that would only be +# retained on failure. +my $log_path = $newnode->data_dir . "/pg_upgrade_output.d/log"; +if (-d $log_path) +{ + foreach my $log (glob("$log_path/*")) + { + note "###########################"; + note "Contents of log file $log"; + note "###########################"; + my $log_contents = slurp_file($log); + print "$log_contents\n"; + } +} + +# Second dump from the upgraded instance. +$newnode->run_log( + [ + 'pg_dumpall', '--no-sync', + '-d', $newnode->connstr('postgres'), + '-f', "$tempdir/dump2.sql" + ]); + +# Compare the two dumps, there should be no differences. +command_ok([ 'diff', '-q', "$tempdir/dump1.sql", "$tempdir/dump2.sql" ], + 'old and new dump match after pg_upgrade'); diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh deleted file mode 100644 index ef328b3062..0000000000 --- a/src/bin/pg_upgrade/test.sh +++ /dev/null @@ -1,279 +0,0 @@ -#!/bin/sh - -# src/bin/pg_upgrade/test.sh -# -# Test driver for pg_upgrade. Initializes a new database cluster, -# runs the regression tests (to put in some data), runs pg_dumpall, -# runs pg_upgrade, runs pg_dumpall again, compares the dumps. -# -# Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group -# Portions Copyright (c) 1994, Regents of the University of California - -set -e - -: ${MAKE=make} - -# Guard against parallel make issues (see comments in pg_regress.c) -unset MAKEFLAGS -unset MAKELEVEL - -# Run a given "initdb" binary and overlay the regression testing -# authentication configuration. -standard_initdb() { - # To increase coverage of non-standard segment size and group access - # without increasing test runtime, run these tests with a custom setting. - # Also, specify "-A trust" explicitly to suppress initdb's warning. - # --allow-group-access and --wal-segsize have been added in v11. - "$1" -N --wal-segsize 1 --allow-group-access -A trust - if [ -n "$TEMP_CONFIG" -a -r "$TEMP_CONFIG" ] - then - cat "$TEMP_CONFIG" >> "$PGDATA/postgresql.conf" - fi - ../../test/regress/pg_regress --config-auth "$PGDATA" -} - -# What flavor of host are we on? -# Treat MINGW* (msys1) and MSYS* (msys2) the same. -testhost=`uname -s | sed 's/^MSYS/MINGW/'` - -# Establish how the server will listen for connections -case $testhost in - MINGW*) - LISTEN_ADDRESSES="localhost" - PG_REGRESS_SOCKET_DIR="" - PGHOST=localhost - ;; - *) - LISTEN_ADDRESSES="" - # Select a socket directory. The algorithm is from the "configure" - # script; the outcome mimics pg_regress.c:make_temp_sockdir(). - if [ x"$PG_REGRESS_SOCKET_DIR" = x ]; then - set +e - dir=`(umask 077 && - mktemp -d /tmp/pg_upgrade_check-XXXXXX) 2>/dev/null` - if [ ! -d "$dir" ]; then - dir=/tmp/pg_upgrade_check-$$-$RANDOM - (umask 077 && mkdir "$dir") - if [ ! -d "$dir" ]; then - echo "could not create socket temporary directory in \"/tmp\"" - exit 1 - fi - fi - set -e - PG_REGRESS_SOCKET_DIR=$dir - trap 'rm -rf "$PG_REGRESS_SOCKET_DIR"' 0 - trap 'exit 3' 1 2 13 15 - fi - PGHOST=$PG_REGRESS_SOCKET_DIR - ;; -esac - -POSTMASTER_OPTS="-F -c listen_addresses=\"$LISTEN_ADDRESSES\" -k \"$PG_REGRESS_SOCKET_DIR\"" -export PGHOST - -# don't rely on $PWD here, as old shells don't set it -temp_root=`pwd`/tmp_check -rm -rf "$temp_root" -mkdir "$temp_root" - -: ${oldbindir=$bindir} - -: ${oldsrc=../../..} -oldsrc=`cd "$oldsrc" && pwd` -newsrc=`cd ../../.. && pwd` - -# We need to make pg_regress use psql from the desired installation -# (likely a temporary one), because otherwise the installcheck run -# below would try to use psql from the proper installation directory -# of the target version, which might be outdated or not exist. But -# don't override anything else that's already in EXTRA_REGRESS_OPTS. -EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --bindir='$oldbindir'" -export EXTRA_REGRESS_OPTS - -# While in normal cases this will already be set up, adding bindir to -# path allows test.sh to be invoked with different versions as -# described in ./TESTING -PATH=$bindir:$PATH -export PATH - -BASE_PGDATA="$temp_root/data" -PGDATA="${BASE_PGDATA}.old" -export PGDATA - -# Send installcheck outputs to a private directory. This avoids conflict when -# check-world runs pg_upgrade check concurrently with src/test/regress check. -# To retrieve interesting files after a run, use pattern tmp_check/*/*.diffs. -outputdir="$temp_root/regress" -EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --outputdir=$outputdir" -export EXTRA_REGRESS_OPTS -mkdir "$outputdir" - -# pg_regress --make-tablespacedir would take care of that in 14~, but this is -# still required for older versions where this option is not supported. -if [ "$newsrc" != "$oldsrc" ]; then - mkdir "$outputdir"/testtablespace - mkdir "$outputdir"/sql - mkdir "$outputdir"/expected -fi - -logdir=`pwd`/log -rm -rf "$logdir" -mkdir "$logdir" - -# Clear out any environment vars that might cause libpq to connect to -# the wrong postmaster (cf pg_regress.c) -# -# Some shells, such as NetBSD's, return non-zero from unset if the variable -# is already unset. Since we are operating under 'set -e', this causes the -# script to fail. To guard against this, set them all to an empty string first. -PGDATABASE=""; unset PGDATABASE -PGUSER=""; unset PGUSER -PGSERVICE=""; unset PGSERVICE -PGSSLMODE=""; unset PGSSLMODE -PGREQUIRESSL=""; unset PGREQUIRESSL -PGCONNECT_TIMEOUT=""; unset PGCONNECT_TIMEOUT -PGHOSTADDR=""; unset PGHOSTADDR - -# Select a non-conflicting port number, similarly to pg_regress.c -PG_VERSION_NUM=`grep '#define PG_VERSION_NUM' "$newsrc"/src/include/pg_config.h | awk '{print $3}'` -PGPORT=`expr $PG_VERSION_NUM % 16384 + 49152` -export PGPORT - -i=0 -while psql -X postgres </dev/null 2>/dev/null -do - i=`expr $i + 1` - if [ $i -eq 16 ] - then - echo port $PGPORT apparently in use - exit 1 - fi - PGPORT=`expr $PGPORT + 1` - export PGPORT -done - -# buildfarm may try to override port via EXTRA_REGRESS_OPTS ... -EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --port=$PGPORT" -export EXTRA_REGRESS_OPTS - -standard_initdb "$oldbindir"/initdb -"$oldbindir"/pg_ctl start -l "$logdir/postmaster1.log" -o "$POSTMASTER_OPTS" -w - -# Create databases with names covering the ASCII bytes other than NUL, BEL, -# LF, or CR. BEL would ring the terminal bell in the course of this test, and -# it is not otherwise a special case. PostgreSQL doesn't support the rest. -dbname1=`awk 'BEGIN { for (i= 1; i < 46; i++) - if (i != 7 && i != 10 && i != 13) printf "%c", i }' </dev/null` -# Exercise backslashes adjacent to double quotes, a Windows special case. -dbname1='\"\'$dbname1'\\"\\\' -dbname2=`awk 'BEGIN { for (i = 46; i < 91; i++) printf "%c", i }' </dev/null` -dbname3=`awk 'BEGIN { for (i = 91; i < 128; i++) printf "%c", i }' </dev/null` -createdb "regression$dbname1" || createdb_status=$? -createdb "regression$dbname2" || createdb_status=$? -createdb "regression$dbname3" || createdb_status=$? - -# Extra options to apply to the dump. This may be changed later. -extra_dump_options="" - -if "$MAKE" -C "$oldsrc" installcheck-parallel; then - oldpgversion=`psql -X -A -t -d regression -c "SHOW server_version_num"` - - # Before dumping, tweak the database of the old instance depending - # on its version. - if [ "$newsrc" != "$oldsrc" ]; then - # This SQL script has its own idea of the cleanup that needs to be - # done on the cluster to-be-upgraded, and includes version checks. - # Note that this uses the script stored on the new branch. - psql -X -d regression -f "$newsrc/src/bin/pg_upgrade/upgrade_adapt.sql" \ - || psql_fix_sql_status=$? - - # Handling of --extra-float-digits gets messy after v12. - # Note that this changes the dumps from the old and new - # instances if involving an old cluster of v11 or older. - if [ $oldpgversion -lt 120000 ]; then - extra_dump_options="--extra-float-digits=0" - fi - fi - - pg_dumpall $extra_dump_options --no-sync \ - -f "$temp_root"/dump1.sql || pg_dumpall1_status=$? - - if [ "$newsrc" != "$oldsrc" ]; then - # update references to old source tree's regress.so etc - fix_sql="" - case $oldpgversion in - *) - fix_sql="UPDATE pg_proc SET probin = replace(probin, '$oldsrc', '$newsrc') WHERE probin LIKE '$oldsrc%';" - ;; - esac - psql -X -d regression -c "$fix_sql;" || psql_fix_sql_status=$? - - mv "$temp_root"/dump1.sql "$temp_root"/dump1.sql.orig - sed "s;$oldsrc;$newsrc;g" "$temp_root"/dump1.sql.orig >"$temp_root"/dump1.sql - fi -else - make_installcheck_status=$? -fi -"$oldbindir"/pg_ctl -m fast stop -if [ -n "$createdb_status" ]; then - exit 1 -fi -if [ -n "$make_installcheck_status" ]; then - exit 1 -fi -if [ -n "$psql_fix_sql_status" ]; then - exit 1 -fi -if [ -n "$pg_dumpall1_status" ]; then - echo "pg_dumpall of pre-upgrade database cluster failed" - exit 1 -fi - -PGDATA="$BASE_PGDATA" - -standard_initdb 'initdb' - -pg_upgrade $PG_UPGRADE_OPTS --no-sync -d "${PGDATA}.old" -D "$PGDATA" -b "$oldbindir" -p "$PGPORT" -P "$PGPORT" - -# make sure all directories and files have group permissions, on Unix hosts -# Windows hosts don't support Unix-y permissions. -case $testhost in - MINGW*|CYGWIN*) ;; - *) if [ `find "$PGDATA" -type f ! -perm 640 | wc -l` -ne 0 ]; then - echo "files in PGDATA with permission != 640"; - exit 1; - fi ;; -esac - -case $testhost in - MINGW*|CYGWIN*) ;; - *) if [ `find "$PGDATA" -type d ! -perm 750 | wc -l` -ne 0 ]; then - echo "directories in PGDATA with permission != 750"; - exit 1; - fi ;; -esac - -pg_ctl start -l "$logdir/postmaster2.log" -o "$POSTMASTER_OPTS" -w - -pg_dumpall $extra_dump_options --no-sync \ - -f "$temp_root"/dump2.sql || pg_dumpall2_status=$? -pg_ctl -m fast stop - -if [ -n "$pg_dumpall2_status" ]; then - echo "pg_dumpall of post-upgrade database cluster failed" - exit 1 -fi - -case $testhost in - MINGW*) MSYS2_ARG_CONV_EXCL=/c cmd /c delete_old_cluster.bat ;; - *) sh ./delete_old_cluster.sh ;; -esac - -if diff "$temp_root"/dump1.sql "$temp_root"/dump2.sql >/dev/null; then - echo PASSED - exit 0 -else - echo "Files $temp_root/dump1.sql and $temp_root/dump2.sql differ" - echo "dumps were not identical" - exit 1 -fi diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl index a994626239..39d1e49721 100644 --- a/src/tools/msvc/vcregress.pl +++ b/src/tools/msvc/vcregress.pl @@ -285,6 +285,10 @@ sub bincheck foreach my $dir (@bin_dirs) { next unless -d "$dir/t"; + # Do not consider pg_upgrade, as it is handled by + # upgradecheck. + next if ($dir =~ "/pg_upgrade/"); + my $status = tap_check($dir); $mstat ||= $status; } @@ -589,91 +593,9 @@ sub generate_db sub upgradecheck { - my $status; - my $cwd = getcwd(); - - # Much of this comes from the pg_upgrade test.sh script, - # but it only covers the --install case, and not the case - # where the old and new source or bin dirs are different. - # i.e. only this version to this version check. That's - # what pg_upgrade's "make check" does. - - $ENV{PGHOST} = 'localhost'; - $ENV{PGPORT} ||= 50432; - my $tmp_root = "$topdir/src/bin/pg_upgrade/tmp_check"; - rmtree($tmp_root); - mkdir $tmp_root || die $!; - my $upg_tmp_install = "$tmp_root/install"; # unshared temp install - print "Setting up temp install\n\n"; - Install($upg_tmp_install, "all", $config); - - # Install does a chdir, so change back after that - chdir $cwd; - my ($bindir, $libdir, $oldsrc, $newsrc) = - ("$upg_tmp_install/bin", "$upg_tmp_install/lib", $topdir, $topdir); - $ENV{PATH} = "$bindir;$ENV{PATH}"; - my $data = "$tmp_root/data"; - $ENV{PGDATA} = "$data.old"; - my $outputdir = "$tmp_root/regress"; - my @EXTRA_REGRESS_OPTS = ("--outputdir=$outputdir"); - mkdir "$outputdir" || die $!; - - my $logdir = "$topdir/src/bin/pg_upgrade/log"; - rmtree($logdir); - mkdir $logdir || die $!; - print "\nRunning initdb on old cluster\n\n"; - standard_initdb() or exit 1; - print "\nStarting old cluster\n\n"; - my @args = ('pg_ctl', 'start', '-l', "$logdir/postmaster1.log"); - system(@args) == 0 or exit 1; - - print "\nCreating databases with names covering most ASCII bytes\n\n"; - generate_db("\\\"\\", 1, 45, "\\\\\"\\\\\\"); - generate_db('', 46, 90, ''); - generate_db('', 91, 127, ''); - - print "\nSetting up data for upgrading\n\n"; - installcheck_internal('parallel', @EXTRA_REGRESS_OPTS); - - # now we can chdir into the source dir - chdir "$topdir/src/bin/pg_upgrade"; - print "\nDumping old cluster\n\n"; - @args = ('pg_dumpall', '-f', "$tmp_root/dump1.sql"); - system(@args) == 0 or exit 1; - print "\nStopping old cluster\n\n"; - system("pg_ctl stop") == 0 or exit 1; - $ENV{PGDATA} = "$data"; - print "\nSetting up new cluster\n\n"; - standard_initdb() or exit 1; - print "\nRunning pg_upgrade\n\n"; - @args = ( - 'pg_upgrade', '-d', "$data.old", '-D', $data, '-b', $bindir, - '--no-sync'); - system(@args) == 0 or exit 1; - print "\nStarting new cluster\n\n"; - @args = ('pg_ctl', '-l', "$logdir/postmaster2.log", 'start'); - system(@args) == 0 or exit 1; - print "\nDumping new cluster\n\n"; - @args = ('pg_dumpall', '-f', "$tmp_root/dump2.sql"); - system(@args) == 0 or exit 1; - print "\nStopping new cluster\n\n"; - system("pg_ctl stop") == 0 or exit 1; - print "\nDeleting old cluster\n\n"; - system(".\\delete_old_cluster.bat") == 0 or exit 1; - print "\nComparing old and new cluster dumps\n\n"; - - @args = ('diff', '-q', "$tmp_root/dump1.sql", "$tmp_root/dump2.sql"); - system(@args); - $status = $?; - if (!$status) - { - print "PASSED\n"; - } - else - { - print "dumps not identical!\n"; - exit(1); - } + InstallTemp(); + my $mstat = tap_check("$topdir/src/bin/pg_upgrade"); + exit $mstat if $mstat; return; } -- 2.34.1 [application/pgp-signature] signature.asc (833B, ../../Ygsl4Q7MARNY%[email protected]/4-signature.asc) download ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set @ 2022-03-02 08:01 Andres Freund <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Andres Freund @ 2022-03-02 08:01 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]> On 2022-02-15 13:02:41 +0900, Michael Paquier wrote: > >> + @regress_command = (@regress_command, @extra_opts); > >> + > >> + $oldnode->command_ok(@regress_command, > >> + 'regression test run on old instance'); > > > > I also think this should take EXTRA_REGRESS_OPTS into account - test.sh did. > > This is already taken into account, as of the @extra_opts bits. But in a bad way, because EXTRA_REGRESS_OPTS now always wins, even for stuff we want to override. Note how test.sh explicitly specifies port, bindir etc after the pre-existing EXTRA_REGRESS_OPTS. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set @ 2022-03-02 08:11 Michael Paquier <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Michael Paquier @ 2022-03-02 08:11 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Thomas Munro <[email protected]>; Noah Misch <[email protected]>; Julien Rouhaud <[email protected]>; Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; Postgres hackers <[email protected]> On Wed, Mar 02, 2022 at 12:01:17AM -0800, Andres Freund wrote: > But in a bad way, because EXTRA_REGRESS_OPTS now always wins, even for stuff > we want to override. Note how test.sh explicitly specifies port, bindir etc > after the pre-existing EXTRA_REGRESS_OPTS. Ah, right. Will fix. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2022-03-02 08:11 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-04 03:53 [PATCH 5/5] WIP unify handling of attributes and expressions Tomas Vondra <[email protected]> 2022-02-13 04:50 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]> 2022-02-13 05:07 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Thomas Munro <[email protected]> 2022-02-13 05:13 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Tom Lane <[email protected]> 2022-02-13 05:29 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]> 2022-02-13 22:23 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Thomas Munro <[email protected]> 2022-02-13 22:55 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]> 2022-02-14 06:40 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]> 2022-02-15 04:02 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[email protected]> 2022-03-02 08:01 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Andres Freund <[email protected]> 2022-03-02 08:11 ` Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set Michael Paquier <[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