Re: extended stats on partitioned tables - Mailing list pgsql-hackers

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