Date: Tue, 26 Mar 2019 16:12:35 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: David Rowley <david.rowley@2ndquadrant.com>,
	Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Michael Paquier <michael@paquier.xyz>,
	Andres Freund <andres@anarazel.de>,
	Thomas Munro <thomas.munro@enterprisedb.com>,
	Bruce Momjian <bruce@momjian.us>,
	Mark Dilger <hornschnorter@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] PATCH: multivariate histograms and MCV lists
Message-ID: <20190326151235.GA3088@development>
References: <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>
 <add2f0d3-4fbc-af7c-e02e-0f2b2d6dedc3@2ndquadrant.com>
 <CAEZATCURhZv0kMsy9PoWvx+4nuvmfGi42f9kte7ZNyWOJRKKjw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAEZATCURhZv0kMsy9PoWvx+4nuvmfGi42f9kte7ZNyWOJRKKjw@mail.gmail.com>
User-Agent: Mutt/1.11.3 (2019-02-01)
Precedence: bulk

On Tue, Mar 26, 2019 at 11:59:56AM +0000, Dean Rasheed wrote:
>On Mon, 25 Mar 2019 at 23:36, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>>
>> 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.
>>
>
>Yes, I think that's reasonable. It looks to be in pretty good shape. I
>have reviewed most of the actual code, but note that I haven't
>reviewed the docs changes and I didn't spend much time reading code
>comments. It might benefit from a quick once-over comment tidy up.
>
>I just looked through the latest set of changes and I have a couple of
>additional review comments:
>
>In the comment about WIDTH_THRESHOLD, s/pg_statistic/pg_statistic_ext/.
>
>In statext_mcv_build(), I'm not convinced by the logic around keeping
>the whole MCV list if it fits. Suppose there were a small number of
>very common values, and then a bunch of uniformly distributed less
>common values. The sample might consist of all the common values, plus
>one or two instances of some of the uncommon ones, leading to a list
>that would fit, but it would not be appropriate to keep the uncommon
>values on the basis of having seen them only one or two times. The
>fact that the list of items seen fits doesn't by itself mean that
>they're all common enough to justify being kept. In the per-column
>stats case, there are a bunch of other checks that have to pass, which
>are intended to test not just that the list fits, but that it believes
>that those are all the items in the table. For MV stats, you don't
>have that, and so I think it would be best to just remove that test
>(the "if (ngroups > nitems)" test) and *always* call
>get_mincount_for_mcv_list() to determine how many MCV items to keep.
>Otherwise there is a risk of keeping too many MCV items, with the ones
>at the tail end of the list producing poor estimates.
>

I need to think about it a bit, but I think you're right we may not need
to keep those items. The reason why I decided to keep them is that they
may represent cases where the (actual frequency << base frequency). But
now that I think about it, that can probably be handled as

  ((1 - total_sel) / ndistinct)

So yeah, I think I'll just change the code to always us the mincount.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services