On Tue, Oct 27, 2020 at 09:27:06PM -0400, Tom Lane wrote:
Tomas Vondra <tomas.von...@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
