Date: Wed, 25 Mar 2020 01:28:01 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Pierre Ducroquet <p.psql@pinaraf.info>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: PATCH: add support for IN and @> in functional-dependency
 statistics use
Message-ID: <20200325002801.i45f34l45rqmik4v@development>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAEZATCX31UBq=SF+UP+mrgQ-dWiknoT3Jr5HhhfcFTtckn6fJQ@mail.gmail.com>
Precedence: bulk

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