Re: enable_incremental_sort changes query behavior

On Tue, Nov 03, 2020 at 03:37:43AM +0100, Tomas Vondra wrote:
>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.
>

I've pushed the 0001 part, i.e. the main fix. Not sure about the other
parts (comments and moving the code back to postgres_fdw) yet.

regards

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