From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Mark Dilger <hornschnorter@gmail.com>
Cc: Adrien Nayrat <adrien.nayrat@dalibo.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] PATCH: multivariate histograms and MCV lists
Date: Sat, 25 Nov 2017 18:56:03 +0100
Message-ID: <ea799763-9729-826a-d1a2-0c5210e26482@2ndquadrant.com> (raw)
In-Reply-To: <0895FFB4-DEB7-459B-ABD2-E7855D0291A8@gmail.com>
References: <dfdac334-9cf2-2597-fb27-f0fb3753f435@2ndquadrant.com>
<31ee8686-6d62-7f10-4129-7580ec74a710@dalibo.com>
<b3688203-7c5d-2d7e-58b6-a6bb04f29b5c@2ndquadrant.com>
<340ffd44-2d67-d39d-e54f-5a97a842cd3a@2ndquadrant.com>
<b3e23e18-afbb-2a39-8e15-fe0a6c39a128@2ndquadrant.com>
<67EAB5B3-F9FB-4753-8102-4D77DCFFFCD4@gmail.com>
<cf43d6de-617f-83c0-8c14-740c3fa23795@2ndquadrant.com>
<0895FFB4-DEB7-459B-ABD2-E7855D0291A8@gmail.com>
Hi,
On 11/25/2017 05:15 PM, Mark Dilger wrote:
>
>> On Nov 18, 2017, at 12:28 PM, Tomas Vondra <tomas.vondra@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
Attachments:
[text/x-patch] 0001-MCV-fix.patch (1.4K, ../ea799763-9729-826a-d1a2-0c5210e26482@2ndquadrant.com/2-0001-MCV-fix.patch)
download | inline diff:
From 1d546eb3d27507ee51824d5a8c348b86187d1754 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, hornschnorter@gmail.com, adrien.nayrat@dalibo.com
Subject: Re: [HACKERS] PATCH: multivariate histograms and MCV lists
In-Reply-To: <ea799763-9729-826a-d1a2-0c5210e26482@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox