Date: Tue, 3 Nov 2020 22:39:42 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: James Coleman <jtc331@gmail.com>
Cc: Jaime Casanova <jaime.casanova@2ndquadrant.com>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: enable_incremental_sort changes query behavior
Message-ID: <20201103213942.sj3qohewdshrw4oz@development>
References: 
 <CAAaqYe8_Y7Ydfwdr_q0NWCpr+E_Hc_e=ScEhi9MUmuLT45cNDA@mail.gmail.com>
 <CAAaqYe9C3f6A_tZCRfr9Dm7hPpgGwpp4i-K_=NS9GWXuNiFANg@mail.gmail.com>
 <CAAaqYe80-V8q7PozVh22EbBhMDU40W286na3RahxJiZ-Pz8S2w@mail.gmail.com>
 <CAJGNTeNjGZYFBjFv055aFgkeAurAOVVe2xp7XB1V4Qd3de8Fuw@mail.gmail.com>
 <CAAaqYe8e9A9SC1BgWPgzyL+v8uC-=y2+yQ0z6CTXBTy1gxjvyg@mail.gmail.com>
 <20201029220652.glf4xk2p7wkfisb6@development>
 <CAAaqYe9P261NfjqhAkDbPGEjqf_8ZyayxTYUEdEnPOqmLRb6EA@mail.gmail.com>
 <20201030210316.cbwv4arwp4lcfgx2@development>
 <CAAaqYe_izhFTq1FLgUxa+J+_bY8FGBNiHjTj=EzrSA72tvacEw@mail.gmail.com>
 <20201103023743.sswkl6h74dkbcfmp@development>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20201103023743.sswkl6h74dkbcfmp@development>
Precedence: bulk

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