Date: Tue, 26 Mar 2019 16:21:08 +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: <20190326152108.GB3088@development>
References: 
 <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>
 <CAEZATCV=W+9LKu=BVbBAgOn3GChGcm=Qdhgco3e3aDxkRqfcVg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAEZATCV=W+9LKu=BVbBAgOn3GChGcm=Qdhgco3e3aDxkRqfcVg@mail.gmail.com>
User-Agent: Mutt/1.11.3 (2019-02-01)
Precedence: bulk

On Tue, Mar 26, 2019 at 01:37:33PM +0000, Dean Rasheed wrote:
>On Tue, 26 Mar 2019 at 11:59, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>>
>> On Mon, 25 Mar 2019 at 23:36, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>> >
>> > Attached is an updated patch...
>>
>> I just looked through the latest set of changes and I have a couple of
>> additional review comments:
>>
>
>I just spotted another issue while reading the code:
>
>It's possible to build an MCV list with more than
>STATS_MCVLIST_MAX_ITEMS = 8192 items, which then causes an error when
>the code tries to read it back in:
>
>create temp table foo(a int, b int);
>insert into foo select x,x from generate_series(1,10000) g(x);
>insert into foo select x,x from generate_series(1,10000) g(x);
>alter table foo alter column a set statistics 10000;
>alter table foo alter column b set statistics 10000;
>create statistics s (mcv) on a,b from foo;
>analyse foo;
>select * from foo where a=1 and b=1;
>
>ERROR:  invalid length (10000) item array in MCVList
>
>So this needs to be checked when building the MCV list.
>
>In fact, the stats targets for table columns can be as large as 10000
>(a hard-coded constant in tablecmds.c, which is pretty ugly, but
>that's a different matter), so I think STATS_MCVLIST_MAX_ITEMS
>probably ought to match that.
>
>There are also a couple of comments that refer to the 8k limit, which
>would need updating, if you change it.
>

I think we can simply ditch this separate limit, and rely on the
statistics target. The one issue with it is that if we ever allows the
statistics target to we may end up overflowing uint16 (which is used in
the serialized representation). But I think it's OK to just check that in
an assert, or so.

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