(2018/09/21 20:03), Etsuro Fujita wrote:
> (2018/09/18 21:14), Kyotaro HORIGUCHI wrote:
>> At Fri, 14 Sep 2018 22:01:39 +0900, Etsuro
>> Fujita<fujita.etsuro@lab.ntt.co.jp> wrote
>> in<5B9BB133.1060107@lab.ntt.co.jp>
>>> I wrote a patch using
>>> the Param-based approach, and compared the two approaches.
>>> I don't think
>>> there would be any warts as discussed above in the Param-based
>>> approach for now. (That approach modifies the planner so that the
>>> targetrel's tlist would contain Params as well as Vars/PHVs, so
>>> actually, it breaks the planner assumption that a rel's tlist would
>>> only include Vars/PHVs, but I don't find any issues on that at least
>>> for now. Will look into that in more detail.)
I spent quite a bit of time looking into that, but I couldn't find any
issues, including ones discussed in [1]:
* In contrib/postgres_fdw, the patch does the special handling of the
Param representing the remote table OID in deparsing a remote SELECT
query and building fdw_scan_tlist, but it wouldn't need the
pull_var_clause change as proposed in [1]. And ISTM that that handling
would be sufficient to avoid errors like 'variable not found in subplan
target lists' as in [1].
* Params as extra target expressions can never be used as Pathkeys or
something like that, so it seems unlikely that that approach would cause
'could not find pathkey item to sort' errors in
prepare_sort_from_pathkeys() as in [1].
* I checked other parts of the planner such as subselect.c and
setrefs.c, but I couldn't find any issues.
>>> What do you think about that?
>> I confirmed that a FOREIGN_PARAM_EXEC is evaluated and stored
>> into the parent node. For the mentioned Merge/Sort/ForeignScan
>> case, Sort node takes the parameter value via projection. I
>> didn't know PARAM_EXEC works that way. I consulted nodeNestLoop
>> but not fully understood.
>>
>> So I think it works. I still don't think expanded tupledesc is
>> not wart but this is smarter than that. Addition to that, it
>> seems back-patchable. I must admit that yours is better.
I also think that approach would be back-patchable to PG9.3, where
contrib/postgres_fdw landed with the writable functionality, so I'm
inclined to vote for the Param-based approach. Attached is an updated
version of the patch. Changes:
* Added this to use_physical_tlist():
> One thing I noticed is: in any approach, I think use_physical_tlist
> needs to be modified so that it disables doing build_physical_tlist for
> a foreign scan in the case where the FDW added resjunk columns for
> UPDATE/DELETE that are different from user/system columns of the foreign
> table; else such columns would not be emitted from the foreign scan.
* Fixed a bug in conversion_error_callback() in contrib/postgres_fdw.c
* Simplified your contrib/postgres_fdw.c tests as discussed
* Revise code/comments a bit
* Added docs to fdwhandler.sgml
* Rebased the patch against the latest HEAD
Best regards,
Etsuro Fujita
[1]
https://www.postgresql.org/message-id/flat/CAKcux6ktu-8tefLWtQuuZBYFaZA83vUzuRd7c1YHC-yEWyYFpg@mail.gmail.com
