Re: extended stats on partitioned tables

On Sat, Dec 11, 2021 at 9:14 PM Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>
wrote:

>
>
> On 12/12/21 05:38, Zhihong Yu wrote:
> >
> >
> > On Sat, Dec 11, 2021 at 8:17 PM Tomas Vondra
> > <tomas(dot)vondra(at)enterprisedb(dot)com <mailto:tomas(dot)vondra(at)enterprisedb(dot)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.
>

Hi,
I agree that skipping should be fine (to avoid too much logging).

Thanks

> > + * 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
>