Date: Sat, 25 Sep 2021 14:53:22 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: pgsql-hackers@postgresql.org, David Rowley <dgrowleyml@gmail.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>
Subject: Re: extended stats on partitioned tables
Message-ID: <20210925195322.GV831@telsasoft.com>
References: <20210923212624.GI831@telsasoft.com>
 <e2196aa4-4a52-6168-6f13-f50b4d0edf48@enterprisedb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e2196aa4-4a52-6168-6f13-f50b4d0edf48@enterprisedb.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20210925195322.GV831%40telsasoft.com>
Precedence: bulk

On Sat, Sep 25, 2021 at 09:27:10PM +0200, Tomas Vondra wrote:
> On 9/23/21 11:26 PM, Justin Pryzby wrote:
> > extended stats objects are allowed on partitioned tables since v10.
> > https://www.postgresql.org/message-id/flat/CAKJS1f-BmGo410bh5RSPZUvOO0LhmHL2NYmdrC_Jm8pk_FfyCA%40mail.gmail.com
> > 8c5cdb7f4f6e1d6a6104cb58ce4f23453891651b
> > 
> > But since 859b3003de they're not populated - pg_statistic_ext(_data) is empty.
> > This was the consequence of a commit to avoid an error I reported with stats on
> > inheritence parents (not partitioned tables).
> > 
> > preceding 859b3003de, stats on the parent table *did* improve the estimate,
> > so this part of the commit message seems to have been wrong?
> > |commit 859b3003de87645b62ee07ef245d6c1f1cd0cedb
> > |    Don't build extended statistics on inheritance trees
> > ...
> > |    Moreover, the current selectivity estimation code only works with individual
> > |    relations, so building statistics on inheritance trees would be pointless
> > |    anyway.
> > 
> > |CREATE TABLE p (i int, a int, b int) PARTITION BY RANGE (i);
> > |CREATE TABLE pd PARTITION OF p FOR VALUES FROM (1)TO(100);
> > |TRUNCATE p; INSERT INTO p SELECT 1, a/100, a/100 FROM generate_series(1,999)a;
> > |CREATE STATISTICS pp ON (a),(b) FROM p;
> > |VACUUM ANALYZE p;
> > |SELECT * FROM pg_statistic_ext WHERE stxrelid ='p'::regclass;
> > 
> > |postgres=# begin; DROP STATISTICS pp; explain analyze SELECT a,b FROM p GROUP BY 1,2; abort;
> > | HashAggregate  (cost=20.98..21.98 rows=100 width=8) (actual time=1.088..1.093 rows=10 loops=1)
> > 
> > |postgres=# explain analyze SELECT a,b FROM p GROUP BY 1,2;
> > | HashAggregate  (cost=20.98..21.09 rows=10 width=8) (actual time=1.082..1.086 rows=10 loops=1)
> > 
> > So I think this is a regression, and extended stats should be populated for
> > partitioned tables - I had actually done that for some parent tables and hadn't
> > noticed that the stats objects no longer do anything.
...
> Agreed, that seems like a regression, but I don't see how to fix that
> without having the extra flag in the catalog. Otherwise we can store just
> one version for each statistics object :-(

Do you think it's possible to backpatch a fix to handle partitioned tables
specifically ?

The "tuple already updated" error which I reported and which was fixed by
859b3003 involved inheritence children.  Since partitioned tables have no data
themselves, the !inh check could be relaxed.  It's not totally clear to me if
the correct statistics would be used in that case.  I suppose the wrong
(inherited) stats would be wrongly applied affect queries FROM ONLY a
partitioned table, which seems pointless to write and also hard for the
estimates to be far off :)

> Attached is a PoC that I quickly bashed together today. It's pretty raw, but
> it passed "make check" and I think it does most of the things right. Can you
> try if this fixes the estimates with partitioned tables?

I think pg_stats_ext_exprs also needs to expose the inherited flag.

Thanks,
-- 
Justin