Subject: Re: [HACKERS] PATCH: multivariate histograms and MCV lists
To: Mark Dilger <hornschnorter@gmail.com>
Cc: Adrien Nayrat <adrien.nayrat@dalibo.com>,
 Pg Hackers <pgsql-hackers@postgresql.org>
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>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <ea799763-9729-826a-d1a2-0c5210e26482@2ndquadrant.com>
Date: Sat, 25 Nov 2017 18:56:03 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <0895FFB4-DEB7-459B-ABD2-E7855D0291A8@gmail.com>
Content-Type: multipart/mixed;
 boundary="------------4D82E804C97853A5D795C3C2"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------4D82E804C97853A5D795C3C2
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

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

--------------4D82E804C97853A5D795C3C2
Content-Type: text/x-patch;
 name="0001-MCV-fix.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="0001-MCV-fix.patch"

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.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


--------------4D82E804C97853A5D795C3C2--