Re: postgres_fdw behaves oddly

Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> (2014/11/19 18:21), Ashutosh Bapat wrote:
>> Ok. I added that comment to the commitfest and changed the status to
>> "ready for commiter".

> Many thanks!

I committed this with some cosmetic adjustments, and one not-so-cosmetic
one: I left it continuing to allow CTID conditions to be sent to the
remote server. That works today and we shouldn't disable it, especially
not in the back branches where it could mean a performance regression
in a minor release.

As for the question of whether reltargetlist or tlist should be examined,
reltargetlist is the correct thing. I do not see a need to modify
use_physical_tlist either. If you trace through what happens in e.g.
postgres_fdw, you'll notice that it only bothers to retrieve actually-used
columns (which it computes correctly) from the remote side. It then
constructs a scan tuple that corresponds to the foreign table's physical
column set, inserting NULLs for any unfetched columns. This is handed
back to execScan.c and matches what a heapscan or indexscan would produce.
The point of the use_physical_tlist optimization is to avoid a projection
step *on this tuple* if we don't really need to do one (ie, if it'd be
just as good for the scan node's output tuple to be identical to the row's
physical tuple). If we disabled use_physical_tlist then we'd be forcing
a projection step that would have the effect of removing the NULLs for
unfetched columns, but it would not actually be of value, just as it isn't
in the corresponding cases for heap/index scans.

BTW, given that there wasn't any very good way to test the createplan.c
fix except by adding test cases to postgres_fdw, I didn't see much value
in splitting the patch in two. The committed patch includes tests that
the createplan.c bug is fixed as well as the postgres_fdw one.

regards, tom lane