From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: David Rowley <david.rowley@2ndquadrant.com>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>
Cc: Thomas Munro <thomas.munro@enterprisedb.com>
Cc: Bruce Momjian <bruce@momjian.us>
Cc: Mark Dilger <hornschnorter@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] PATCH: multivariate histograms and MCV lists
Date: Tue, 26 Mar 2019 00:36:25 +0100
Message-ID: <add2f0d3-4fbc-af7c-e02e-0f2b2d6dedc3@2ndquadrant.com> (raw)
In-Reply-To: <CAKJS1f9o23x-utM3Jk2wji_tQR-CZgyPP5cqQgt1e_zhHMYQpw@mail.gmail.com>
References: <19f76496-dcf3-ccea-dd82-26fbed57b8f5@2ndquadrant.com>
<20190313.131951.178195776.horiguchi.kyotaro@lab.ntt.co.jp>
<CAKJS1f_6qDQj9m2H0jF4bRkZVLpfc7O9E+MxdXrq0wgv0z1NrQ@mail.gmail.com>
<20190314.205623.235434357.horiguchi.kyotaro@lab.ntt.co.jp>
<c2406825-6ebe-26e6-91b3-ba94026546a6@2ndquadrant.com>
<CAEZATCVazGRDjbZpRF6r-Asiv_-U8vcT-VA0oSZribhhmDUQHQ@mail.gmail.com>
<37ce47c5-e0cf-c6d3-6923-6b8cfd1cc6b1@2ndquadrant.com>
<CAEZATCXfk-Syby-MWLFfN-v=7HFEm-QYyX+tP=NVZ+iNw8ELUw@mail.gmail.com>
<c08dd2ce-14f0-463b-763c-13af49926d03@2ndquadrant.com>
<CAKJS1f_naKM6kzeUdDs61=++fYdtsk4T5586DZObRvUXPVmL9Q@mail.gmail.com>
<fd4c5223-6c1a-c2d5-c0e0-53db789a0bdc@2ndquadrant.com>
<CAKJS1f9o23x-utM3Jk2wji_tQR-CZgyPP5cqQgt1e_zhHMYQpw@mail.gmail.com>
Hi,
Attached is an updated patch, fixing all the issues pointed out so far.
Unless there are some objections, I plan to commit the 0001 part by the
end of this CF. Part 0002 is a matter for PG13, as previously agreed.
On 3/24/19 1:17 AM, David Rowley wrote:
> On Sun, 24 Mar 2019 at 12:41, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>>
>> On 3/21/19 4:05 PM, David Rowley wrote:
>>> 11. In get_mincount_for_mcv_list() it's probably better to have the
>>> numerical literals of 0.0 instead of just 0.
>>
>> Why?
>
> Isn't it what we do for float and double literals?
>
OK, fixed.
>>
>>> 12. I think it would be better if you modified build_attnums_array()
>>> to add an output argument that sets the size of the array. It seems
>>> that most places you call this function you perform bms_num_members()
>>> to determine the array size.
>>
>> Hmmm. I've done this, but I'm not sure I like it very much - there's no
>> protection the value passed in is the right one, so the array might be
>> allocated either too small or too large. I think it might be better to
>> make it work the other way, i.e. pass the value out instead.
>
> When I said "that sets the size", I meant "that gets set to the size",
> sorry for the confusion. I mean, if you're doing bms_num_members()
> inside build_attnums_array() anyway, then this will save you from
> having to do that in the callers too.
>
OK, I've done this now, and I'm fairly happy with it.
>>> 28. Can you explain what this is?
>>>
>>> uint32 type; /* type of MCV list (BASIC) */
>>>
>>> I see: #define STATS_MCV_TYPE_BASIC 1 /* basic MCV list type */
>>>
>>> but it's not really clear to me what else could exist. Maybe the
>>> "type" comment can explain there's only one type for now, but more
>>> might exist in the future?
>>>
>>
>> It's the same idea as for dependencies/mvdistinct stats, i.e.
>> essentially a version number for the data structure so that we can
>> perhaps introduce some improved version of the data structure in the future.
>>
>> But now that I think about it, it seems a bit pointless. We would only
>> do that in a major version anyway, and we don't keep statistics during
>> upgrades. So we could just as well introduce the version/flag/... if
>> needed. We can't do this for regular persistent data, but for stats it
>> does not matter.
>>
>> So I propose we just remove this thingy from both the existing stats and
>> this patch.
>
> I see. I wasn't aware that existed for the other types. It certainly
> gives some wiggle room if some mistakes were discovered after the
> release, but thinking about it, we could probably just change the
> "magic" number and add new code in that branch only to ignore the old
> magic number, perhaps with a WARNING to analyze the table again. The
> magic field seems sufficiently early in the struct that we could do
> that. In the master branch we'd just error if the magic number didn't
> match, since we wouldn't have to deal with stats generated by the
> previous version's bug.
>
OK. I've decided to keep the field for now, for sake of consistency with
the already existing statistic types. I think we can rethink that in the
future, if needed.
>>> 29. Looking at the tests I see you're testing that you get bad
>>> estimates without extended stats. That does not really seem like
>>> something that should be done in tests that are meant for extended
>>> statistics.
>>>
>>
>> True, it might be a bit unnecessary. Initially the tests were meant to
>> show old/new estimates for development purposes, but it might not be
>> appropriate for regression tests. I don't think it's a big issue, it's
>> not like it'd slow down the tests significantly. Opinions?
>
> My thoughts were that if someone did something to improve non-MV
> stats, then is it right for these tests to fail? What should the
> developer do in the case? update the expected result? remove the test?
> It's not so clear.
>
I think such changes would affect a number of other places in regression
tests (changing plans, ...), so I don't see why fixing these tests would
be any different.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
[application/gzip] 0001-multivariate-MCV-lists-20190325.patch.gz (39.9K, ../add2f0d3-4fbc-af7c-e02e-0f2b2d6dedc3@2ndquadrant.com/2-0001-multivariate-MCV-lists-20190325.patch.gz)
download
[application/gzip] 0002-multivariate-histograms-20190325.patch.gz (48.5K, ../add2f0d3-4fbc-af7c-e02e-0f2b2d6dedc3@2ndquadrant.com/3-0002-multivariate-histograms-20190325.patch.gz)
download
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, david.rowley@2ndquadrant.com, dean.a.rasheed@gmail.com, horiguchi.kyotaro@lab.ntt.co.jp, alvherre@2ndquadrant.com, michael@paquier.xyz, andres@anarazel.de, thomas.munro@enterprisedb.com, bruce@momjian.us, hornschnorter@gmail.com
Subject: Re: [HACKERS] PATCH: multivariate histograms and MCV lists
In-Reply-To: <add2f0d3-4fbc-af7c-e02e-0f2b2d6dedc3@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