From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Pierre Ducroquet <p.psql@pinaraf.info>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: PATCH: add support for IN and @> in functional-dependency statistics use
Date: Wed, 25 Mar 2020 01:28:01 +0100
Message-ID: <20200325002801.i45f34l45rqmik4v@development> (raw)
In-Reply-To: <CAEZATCX31UBq=SF+UP+mrgQ-dWiknoT3Jr5HhhfcFTtckn6fJQ@mail.gmail.com>
References: <CAEZATCVb76OcPrA_zkBLk9J3CWPN8bRNZTCtCehbm+RFj=W-Aw@mail.gmail.com>
<20200313160944.aqrbkyf7tkasoxfk@development>
<20200314152159.5webesf6pfluatqj@development>
<20200314184535.4jgkxcyg5yprvsra@development>
<CAEZATCVV2NM0S=hZ1_qH-9B_psrC+5Tqx2xjmL9kHc9EsACSyA@mail.gmail.com>
<20200317153706.z6pdtijr25iyr424@development>
<CAEZATCWCszNJv4QvXYGo6JpgtM09mGpjraA=1_Bk+7MPENeARw@mail.gmail.com>
<20200317170517.hutarq3sioyny3wa@development>
<20200318002946.6dvblukm3cfmgir2@development>
<CAEZATCX31UBq=SF+UP+mrgQ-dWiknoT3Jr5HhhfcFTtckn6fJQ@mail.gmail.com>
On Thu, Mar 19, 2020 at 07:53:39PM +0000, Dean Rasheed wrote:
>On Wed, 18 Mar 2020 at 00:29, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>>
>> OK, I took a look. I think from the correctness POV the patch is OK, but
>> I think the dependencies_clauselist_selectivity() function now got a bit
>> too complex. I've been able to parse it now, but I'm sure I'll have
>> trouble in the future :-(
>>
>> Can we refactor / split it somehow and move bits of the logic to smaller
>> functions, or something like that?
>>
>
>Yeah, it has gotten a bit long. It's somewhat tricky splitting it up,
>because of the number of shared variables used throughout the
>function, but here's an updated patch splitting it into what seemed
>like the 2 most logical pieces. The first piece (still in
>dependencies_clauselist_selectivity()) works out what dependencies
>can/should be applied, and the second piece in a new function does the
>actual work of applying the list of functional dependencies to the
>clause list.
>
>I think that has made it easier to follow, and it has also reduced the
>complexity of the final "no applicable stats" branch.
>
Seems OK to me.
I'd perhaps name deps_clauselist_selectivity differently, it's a bit too
similar to dependencies_clauselist_selectivity. Perhaps something like
clauselist_apply_dependencies? But that's a minor detail.
>> Another thing I'd like to suggest is keeping the "old" formula, and
>> instead of just replacing it with
>>
>> P(a,b) = f * Min(P(a), P(b)) + (1-f) * P(a) * P(b)
>>
>> but explaining how the old formula may produce nonsensical selectivity,
>> and how the new formula addresses that issue.
>>
>
>I think this is purely a comment issue? I've added some more extensive
>comments attempting to justify the formulae.
>
Yes, it was purely a comment issue. Seems fine now.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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@2ndquadrant.com, dean.a.rasheed@gmail.com, p.psql@pinaraf.info, pgsql-hackers@lists.postgresql.org
Subject: Re: PATCH: add support for IN and @> in functional-dependency statistics use
In-Reply-To: <20200325002801.i45f34l45rqmik4v@development>
* 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