Date: Wed, 3 Nov 2021 18:19:04 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Jaime Casanova <jcasanov@systemguards.com.ec>,
	pgsql-hackers@postgresql.org
Subject: Re: extended stats on partitioned tables
Message-ID: <20211103231904.GP17618@telsasoft.com>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <46f37069-55cb-db6a-a15a-e351a2606592@enterprisedb.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20211103231904.GP17618%40telsasoft.com>
Precedence: bulk

On Wed, Nov 03, 2021 at 11:48:44PM +0100, Tomas Vondra wrote:
> On 10/8/21 12:45 AM, Justin Pryzby wrote:
> > On Thu, Oct 07, 2021 at 03:26:46PM -0500, Jaime Casanova wrote:
> >> On Sun, Sep 26, 2021 at 03:25:50PM -0500, Justin Pryzby wrote:
> >>> On Sat, Sep 25, 2021 at 05:31:52PM -0500, Justin Pryzby wrote:
> >>>> It seems like your patch should also check "inh" in examine_variable and
> >>>> statext_expressions_load.
> >>>
> >>> I tried adding that - I mostly kept my patches separate.
> >>> Hopefully this is more helpful than a complication.
> >>> I added at: https://commitfest.postgresql.org/35/3332/
> >>>
> >>
> >> Actually, this is confusing. Which patch is the one we should be
> >> reviewing?
> > 
> > It is confusing, but not as much as I first thought.  Please check the commit
> > messages.
> > 
> > The first two patches are meant to be applied to master *and* backpatched.  The
> > first one intends to fixes the bug that non-inherited stats are being used for
> > queries of inheritance trees.  The 2nd one fixes the regression that stats are
> > not collected for inheritence trees of partitioned tables (which is the only
> > type of stats they could ever possibly have).
> 
> I think 0001 and 0002 seem mostly fine, but it seems a bit strange to do
> the (!rte->inh) check in the rel->statlist loops. AFAICS both places
> could do that right at the beginning, because it does not depend on the
> statistics object at all, just the RelOptInfo.

I probably did this to make the code change small, to avoid indentin the whole
block.

> > And the 3rd+4th patches (Tomas' plus my changes) allow collecting both
> > inherited and non-inherited stats, only in master, since it requires a catalog
> > change.  It's a bit confusing that patch #4 removes most what I added in
> > patches 1 and 2.  But that's exactly what's needed to collect and apply both
> > inherited and non-inherited stats: the first two patches avoid applying stats
> > collected with the wrong inheritence.  That's also what's needed for the
> > patchset to follow the normal "apply to master and backpatch" process, rather
> > than 2 patches which are backpatched but not applied to master, and one which
> > is applied to master and not backpatched..
> > 
> 
> Yeah. Af first I was a bit confused because after applying 0003 there
> are both the fixes and the "correct" way, but then I realized 0004
> removes the unnecessary bits.

This was to leave your 0003 (mostly) unchanged, so you can see and/or apply my
changes.  They should be squished together.

> The one thing 0003 still needs is to rework the places that need to
> touch both inh and !inh stats. The patch simply does
> 
>     for (inh = 0; inh <= 1; inh++) { ... }
> 
> but that feels a bit too hackish. But if we don't know which of the two
> stats exist, I'm not sure what to do about it. 

There's also this:

On Sun, Sep 26, 2021 at 03:25:50PM -0500, Justin Pryzby wrote:
> +       /* create only the "stxdinherit=false", because that always exists */
> +       datavalues[Anum_pg_statistic_ext_data_stxdinherit - 1] = ObjectIdGetDatum(false);
> 
> That'd be confusing for partitioned tables, no?
> They'd always have an row with no data.
> I guess it could be stxdinherit = BoolGetDatum(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE).
> (not ObjectIdGetDatum).
> Then, that affects the loops which delete the tuples - neither inh nor !inh is
> guaranteed, unless you check relkind there, too.

Maybe the for inh<=1 loop should instead be two calls to new functions factored
out of get_relation_statistics() and RemoveStatisticsById(), which take "bool
inh".

> And I'm not sure we do the right thing after removing children, for example
> (that should drop the inheritance stats, I guess).

Do you mean for inheritance only ?  Or partitions too ?
I think for partitions, the stats should stay.
And for inheritence, they can stay, for consistency with partitions, and since
it does no harm.