Re: Patch to fix FK-related selectivity estimates with constants

On Tue, Oct 27, 2020 at 09:27:06PM -0400, Tom Lane wrote:
>Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> On Tue, Oct 27, 2020 at 01:58:56PM -0400, Tom Lane wrote:
>>>> Attached is a patch series that attacks it that way.
>
>> The patch sems fine to me, thanks for investigating and fixing this.
>
>Thanks for looking at it!
>
>> I find it a bit strange that generate_base_implied_equalities_const adds
>> the rinfo to ec_derives, while generate_base_implied_equalities_no_const
>> does not. I understand it's correct as we don't lookup the non-const
>> clauses, and we want to keep the list as short as possible, but it seems
>> like a bit surprising/unexpected difference in behavior.
>
>Yeah, perhaps.  I considered replacing ec_derives with two lists, one
>for base-level derived clauses and one for join-level derived clauses,
>but it didn't really seem worth the trouble.  This is something we
>could change later if a need arises to be able to look back at non-const
>base-level derived clauses.
>
>> I think casting the 'clause' to (Node*) in process_implied_equality is
>> unnecessary - it was probably needed when it was declared as Expr* but
>> the patch changes that.
>
>Hm, thought I got rid of the unnecessary casts ... I'll look again.
>

Apologies, the casts are fine. I got it mixed up somehow.

>> As for the backpatching, I don't feel very strongly about it. It's
>> clearly a bug/thinko in the code, and I don't see any obvious risks in
>> backpatching it (no ABI breaks etc.).
>
>I had two concerns about possible extension breakage from a back-patch:
>
>* Changing the set of fields in ForeignKeyOptInfo is an ABI break.
>We could minimize the risk by adding the new fields at the end in
>the back branches, but it still wouldn't be zero-risk.
>
>* Changing the expectation about whether process_implied_equality()
>will fill left_ec/right_ec is an API break.  It's somewhat doubtful
>whether there exist any callers outside equivclass.c, but again it's
>not zero risk.
>
>The other issue, entirely unrelated to code hazards, is whether this
>is too big a change in planner behavior to be back-patched.  We've
>often felt that destabilizing stable-branch plan choices is something
>to be avoided.
>
>Not to mention the whole issue of whether this patch has any bugs of
>its own.
>
>So on the whole I wouldn't want to back-patch, or at least not do so
>very far.  Maybe there's an argument that v13 is still new enough to
>deflect the concern about plan stability.
>

OK, understood.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services