From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Daniel Gustafsson <daniel@yesql.se>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Additional improvements to extended statistics
Date: Wed, 2 Dec 2020 17:34:04 +0100
Message-ID: <01a57d32-e442-b57f-6154-fd42129d6307@enterprisedb.com> (raw)
In-Reply-To: <CAEZATCWFFU1NYqJ3pRDohDJZaDbv4NWXMgyRtph8zomagzeZag@mail.gmail.com>
References: <20200315000809.ruwpx7ktnpnzly7c@development>
<20200318193128.53soruotqdqnlaoy@development>
<CAEZATCVuV+UWp9v51BRbrXibUOSWFpQeeZkZTs-=G6d0dayZBQ@mail.gmail.com>
<20200321215946.epx3dfi74stlwiab@development>
<CAEZATCW3dy1g=tEmP3oW-4FH=Lpp516xfXfQyPXiEYm4v3hJfQ@mail.gmail.com>
<20200324010838.hbv5gzhexcv5cilg@development>
<CAEZATCX8u9bZzcWEzqA_t7f_OQHu2oxeTUGnFHNEOXnJo35AQg@mail.gmail.com>
<20200324143306.rz5siohm4jsoveqm@development>
<C68FE2F1-94B5-4286-A48A-6B62D155745A@yesql.se>
<20200703011030.smzijlphrjjodhxx@development>
<57eb142c-d5a4-cfca-675b-0dfd9ef7c2e2@2ndquadrant.com>
<CAEZATCVE2Jx1QB0FFazvgjKAY25tv-EWX9C5XSefEZY-gxn75A@mail.gmail.com>
<56df91cd-c7dc-f46b-6bc3-cb36982ff9ae@enterprisedb.com>
<CAEZATCU4dVJkW1sNgOP800=mvzQMvXTw_Y=tnkt3dg=Q+BJVdw@mail.gmail.com>
<CAEZATCVi6Lxooa8YFq5F6tkeu9kB_o3SUqjo95_kuSs2Sq-rHQ@mail.gmail.com>
<ec21ebff-67ff-3d80-462d-131bdde91b58@enterprisedb.com>
<CAEZATCWFFU1NYqJ3pRDohDJZaDbv4NWXMgyRtph8zomagzeZag@mail.gmail.com>
On 12/2/20 4:51 PM, Dean Rasheed wrote:
> On Sun, 29 Nov 2020 at 21:02, Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> I wonder how much of the comment before clauselist_selectivity should
>> move to clauselist_selectivity_ext - it does talk about range clauses
>> and so on, but clauselist_selectivity does not really deal with that.
>> But maybe that's just an implementation detail and it's better to keep
>> the comment the way it is.
>
> I think it's better to keep it the way it is, so that the entirety of
> what clauselist_selectivity() does (via clauselist_selectivity_ext())
> can be read in one place, but I have added separate comments for the
> new "_ext" functions to explain how they differ. That matches similar
> examples elsewhere.
>
+1
>
>> I noticed this outdated comment:
>>
>> /* Always compute the selectivity using clause_selectivity */
>> s2 = clause_selectivity_ext(root, clause, varRelid, jointype, sjinfo,
>
> Updated.
>
>
>> Also, the comment at clauselist_selectivity_or seems to not follow the
>> usual pattern, which I think is
>>
>> /*
>> * function name
>> * short one-sentence description
>> *
>> * ... longer description ...
>> */
>
> Hmm, it seems OK to me. The first part is basically copied from
> clauselist_selectivity(). The "longer description" doesn't really have
> much more to say because it's much simpler than
> clauselist_selectivity(), but it seems neater to keep the two roughly
> in sync.
>
I see. In that case it's OK, I guess.
>
> I've been hacking on this a bit more and attached is an updated
> (hopefully final) version with some other comment improvements and
> also a couple of other tweaks:
>
> The previous version had duplicated code blocks that implemented the
> same MCV-correction algorithm using simple_sel, mcv_sel, base_sel,
> other_sel and total_sel, which was quite messy. So I refactored that
> into a new function mcv_combine_selectivities(). About half the
> comments from statext_mcv_clauselist_selectivity() then move over to
> mcv_combine_selectivities(). I also updated the comments for
> mcv_clauselist_selectivity() and mcv_clause_selectivity_or() to
> explain how their outputs are expected to be used by
> mcv_combine_selectivities(). That hopefully makes for a clean
> separation of concerns, and makes it easier to tweak the way MCV stats
> are applied on top of simple stats, if someone thinks of a better
> approach in the future.
>
> In the previous version, for an ORed list of clauses, the MCV
> correction was only applied to the overlaps between clauses. That's OK
> as long as each clause only refers to a single column, since the
> per-column statistics ought to be the best way to estimate each
> individual clause in that case. However, if the individual clauses
> refer to more than one column, I think the MCV correction should be
> applied to each individual clause as well as to the overlaps. That
> turns out to be pretty straightforward, since we're already doing all
> the hard work of computing the match bitmap for each clause. The sort
> of queries I had in mind were things like this:
>
> WHERE (a = 1 AND b = 1) OR (a = 2 AND b = 2)
>
> I added a new test case along those lines and the new estimates are
> much better than they are without this patch, but not for the reason I
> thought --- the old code consistently over-estimated queries like that
> because it actually applied the MCV correction twice (once while
> processing each AND list, via clause_selectivity(), called from
> clauselist_selectivity_simple(), and once for the top-level OR clause,
> contained in a single-element implicitly-ANDed list). The way the new
> code is structured avoids any kind of double application of extended
> stats, producing a much better estimate, which is good.
>
Nice.
> However, the new code doesn't apply the extended stats directly using
> clauselist_selectivity_or() for this kind of query because there are
> no RestrictInfos for the nested AND clauses, so
> find_single_rel_for_clauses() (and similarly
> statext_is_compatible_clause()) regards those clauses as not
> compatible with extended stats. So what ends up happening is that
> extended stats are used only when we descend down to the two AND
> clauses, and their results are combined using the original "s1 + s2 -
> s1 * s2" formula. That actually works OK in this case, because there
> is no overlap between the two AND clauses, but it wouldn't work so
> well if there was.
>
> I'm pretty sure that can be fixed by teaching
> find_single_rel_for_clauses() and statext_is_compatible_clause() to
> handle BoolExpr clauses, looking for RestrictInfos underneath them,
> but I think that should be left for a follow-in patch. I have left a
> regression test in place, whose estimates ought to be improved by such
> a fix.
>
Yeah, I agree with leaving this for a separate patch. We can't do
everything at the same time.
> The upshot of all that is that the new code that applies the MCV
> correction to the individual clauses in an ORed list doesn't help with
> queries like the one above at the moment, and it's not obvious whether
> it is currently reachable, but I think it's worth leaving in because
> it seems more principled, and makes that code more future-proof. I
> also think it's neater because now the signature of
> mcv_clause_selectivity_or() is more natural --- it's primary return
> value is now the clause's MCV selectivity, as suggested by the
> function's name, rather than the overlap selectivity that the previous
> version was returning. Also, after your "Var Op Var" patch is applied,
> I think it would be possible to construct queries that would benefit
> from this, so it would be good to get that committed too.
>
> Barring any further comments, I'll push this sometime soon.
>
+1
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@enterprisedb.com, dean.a.rasheed@gmail.com, tomas.vondra@2ndquadrant.com, daniel@yesql.se
Subject: Re: Additional improvements to extended statistics
In-Reply-To: <01a57d32-e442-b57f-6154-fd42129d6307@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox