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.
> 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.
regards, tom lane