Message-ID: <fb1c759c-559c-c95a-9e76-8ca2c9bd2c61@enterprisedb.com>
Date: Sun, 12 Dec 2021 06:14:21 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Subject: Re: extended stats on partitioned tables
Content-Language: en-US
To: Zhihong Yu <zyu@yugabyte.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>,
 Jaime Casanova <jcasanov@systemguards.com.ec>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <20210923212624.GI831@telsasoft.com>
 <e2196aa4-4a52-6168-6f13-f50b4d0edf48@enterprisedb.com>
 <20210925223152.GA7877@telsasoft.com> <20210926202550.GA831@telsasoft.com>
 <20211007202646.GA9522@ahch-to> <20211007224543.GH27491@telsasoft.com>
 <46f37069-55cb-db6a-a15a-e351a2606592@enterprisedb.com>
 <20211103231904.GP17618@telsasoft.com>
 <689accb1-76e8-5115-6ed0-336749cdca57@enterprisedb.com>
 <20211203052411.GU17618@telsasoft.com>
 <CALNJ-vQSpBxw-gjZ--p-vo_yr2yHaXXOn_g8i=DQam_aAW3GwQ@mail.gmail.com>
 <8052505e-2dd2-e8cb-26c9-cbb5dff7d750@enterprisedb.com>
 <CALNJ-vTQkBy9_3BQbWJFodVhYMtYP63LppjdbponUgVbqtnnvQ@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: 
 <CALNJ-vTQkBy9_3BQbWJFodVhYMtYP63LppjdbponUgVbqtnnvQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/fb1c759c-559c-c95a-9e76-8ca2c9bd2c61%40enterprisedb.com>
Precedence: bulk


On 12/12/21 05:38, Zhihong Yu wrote:
> 
> 
> On Sat, Dec 11, 2021 at 8:17 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com <mailto:tomas.vondra@enterprisedb.com>>
> wrote:
> 
>     Hi,
> 
>     Attached is a rebased and cleaned-up version of these patches, with more
>     comments, refactorings etc. Justin and Zhihong, can you take a look?
> 
> 
>     0001 - Ignore extended statistics for inheritance trees
> 
>     0002 - Build inherited extended stats on partitioned tables
> 
>     Those are mostly just Justin's patches, with more detailed comments and
>     updated commit message. I've considered moving the rel->inh check to
>     statext_clauselist_selectivity, and then removing the check from
>     dependencies and MCV. But I decided no to do that, because someone might
>     be calling those functions directly (even if that's very unlikely).
> 
>     The one thing bugging me a bit is that the regression test checks only a
>     GROUP BY query. It'd be nice to add queries testing MCV/dependencies
>     too, but that seems tricky because most queries will use per-partitions
>     stats.
> 
> 
>     0003 - Add stxdinherit flag to pg_statistic_ext_data
> 
>     This is the patch for master, allowing to build stats for both inherits
>     flag values. It adds the flag to pg_stats_ext_exprs view to, reworked
>     how we deal with iterating both flags etc. I've adopted most of the
>     Justin's fixup patches, except that in plancat.c I've refactored how we
>     load the stats to process keys/expressions just once.
> 
>     It has the same issue with regression test using just a GROUP BY query,
>     but if we add a test to 0001/0002, that'll fix this too.
> 
> 
>     0004 - Refactor parent ACL check
> 
>     Not sure about this - I doubt saving 30 rows in an 8kB file is really
>     worth it. Maybe it is, but then maybe we should try cleaning up the
>     other ACL checks in this file too? Seems mostly orthogonal to this
>     thread, though.
> 
> 
> Hi,
> For patch 3, in commit message:
> 
> and there no clear winner. -> and there is no clear winner. 
> 
> and it seem wasteful -> and it seems wasteful
> 
> The there may be -> There may be
> 

Thanks, will fix.

> +       /* skip statistics with mismatching stxdinherit value */
> +       if (stat->inherit != rte->inh)
> 
> Should a log be added for the above case ?
> 

Why should we log this? It's an entirely expected case - there's a
mismatch between inheritance for the relation and statistics, simply
skipping it is the right thing to do.

> +                    * Determine if we'redealing with inheritance tree.
> 
> There should be a space between re and dealing.
> 

Thanks, will fix.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company