Hi, On 11/25/2017 05:15 PM, Mark Dilger wrote: > >> On Nov 18, 2017, at 12:28 PM, Tomas Vondra <tomas.von...@2ndquadrant.com> >> wrote: >> >> Hi, >> >> Attached is an updated version of the patch, adopting the psql describe >> changes introduced by 471d55859c11b. >> >> regards >> >> -- >> Tomas Vondra http://www.2ndQuadrant.com >> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >> <0001-multivariate-MCV-lists.patch.gz><0002-multivariate-histograms.patch.gz> > > Thanks, Tomas, again for your work on this feature. > > Applying just the 0001-multivariate-MCV-lists.patch to the current master, and > then extending the stats_ext.sql test as follows, I am able to trigger an > error, > "ERROR: operator 4294934272 is not a valid ordering operator". >
Ah, that's a silly bug ... The code assumes that VacAttrStats->extra_data is always StdAnalyzeData, and attempts to extract the ltopr from that. But for arrays that's of course not true (array_typanalyze uses ArrayAnalyzeExtraData instead). The reason why this only fails after the second INSERT is that we need at least two occurrences of a value before considering it eligible for MCV list. So after the first INSERT we don't even call the serialize. Attached is a fix that should resolve this in MCV lists by looking up the operator using lookup_type_cache() when serializing the MCV. FWIW histograms have the same issue, but on more places (not just in serialize, but also when building the histogram). I'll send a properly updated patch series shortly, with tests checking correct behavior with arrays. Thanks for the report. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 1d546eb3d27507ee51824d5a8c348b86187d1754 Mon Sep 17 00:00:00 2001 From: Tomas Vondra <to...@2ndquadrant.com> Date: Sat, 25 Nov 2017 18:44:14 +0100 Subject: [PATCH] MCV fix --- src/backend/statistics/mcv.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c index 0586054..af4d894 100644 --- a/src/backend/statistics/mcv.c +++ b/src/backend/statistics/mcv.c @@ -515,7 +515,13 @@ statext_mcv_serialize(MCVList *mcvlist, VacAttrStats **stats) for (dim = 0; dim < ndims; dim++) { int ndistinct; - StdAnalyzeData *tmp = (StdAnalyzeData *) stats[dim]->extra_data; + TypeCacheEntry *typentry; + + /* + * Lookup the LT operator (can't get it from stats extra_data, as + * we don't know how to interpret that - scalar vs. array etc.). + */ + typentry = lookup_type_cache(stats[dim]->attrtypid, TYPECACHE_LT_OPR); /* copy important info about the data type (length, by-value) */ info[dim].typlen = stats[dim]->attrtype->typlen; @@ -543,7 +549,7 @@ statext_mcv_serialize(MCVList *mcvlist, VacAttrStats **stats) ssup[dim].ssup_collation = DEFAULT_COLLATION_OID; ssup[dim].ssup_nulls_first = false; - PrepareSortSupportFromOrderingOp(tmp->ltopr, &ssup[dim]); + PrepareSortSupportFromOrderingOp(typentry->lt_opr, &ssup[dim]); qsort_arg(values[dim], counts[dim], sizeof(Datum), compare_scalars_simple, &ssup[dim]); -- 2.9.5