On Fri, Oct 30, 2020 at 07:37:33PM -0400, James Coleman wrote:
>
> ...
>
>I was looking at this some more, and I'm still convinced that this is
>correct, but I don't think a comment about it being an optimization
>(though I suspect that that its major benefit), since it came from,
>and must parallel, find_ec_member_for_tle, and there is no such
>explanation there. I don't want to backfill reasoning onto that
>decision, but I do think it's important to parallel it since it's
>ultimately what is going to cause errors if we're out of sync with it.
>
>As to why find_em_expr_for_rel is different, I think the answer is
>that it's used by the FDW code, and it seems to me to make sense that
>in that case we'd only send sort conditions to the foreign server if
>the em actually contains rels.
>
>The new series attached includes the primary fix for this issue, the
>additional comments that could either go in at the same time or not,
>and my patch with semi-related questions (which isn't intended to be
>committed, but to keep track of the questions).
>
Thanks. Attached are two patches that I plan to get committed
0001 is what you sent, with slightly reworded commit message. This is
the actual fix.
I'm still thinking about Robert's comment that perhaps we should be
considering only expressions already in the relation's target, which
would imply checking for volatile expressions is not enough. But I've
been unable to convince myself it's correct or incorrect. In any case
0001 is a clear improvement - in the worst case we'll have to make it
stricter in the future.
0002 partially reverts ba3e76cc571 by moving find_em_expr_for_rel back
to postgres_fdw.c. We've moved it to equivclass.c so that it could be
called from both the FDW and allpaths.c, but now that the functions
diverged it's only called from the FDW again. So maybe we should move
it back, but I guess that's not a good thing in a minor release.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
