Re: Problem while updating a foreign table pointing to a partitionedtable on foreign server - Mailing list pgsql-hackers

(2018/08/09 22:04), Etsuro Fujita wrote:
> (2018/08/08 17:30), Kyotaro HORIGUCHI wrote:
>> I choosed to expand tuple descriptor for junk column added to
>> foreign relaions. We might be better to have new member in
>> ForeignScan but I didn't so that we can backpatch it.
>
> I've not looked at the patch closely yet, but I'm not sure that it's a
> good idea to expand the tuple descriptor of the target relation on the
> fly so that it contains the remotetableoid as a non-system attribute of
> the target table. My concern is: is there not any risk in affecting some
> other part of the planner and/or the executor? (I was a bit surprised
> that the patch passes the regression tests successfully.)

I spent more time looking at the patch.  ISTM that the patch well 
suppresses the effect of the tuple-descriptor expansion by making 
changes to code in the planner and executor (and ruleutils.c), but I'm 
still not sure that the patch is the right direction to go in, because 
ISTM that expanding the tuple descriptor on the fly might be a wart.

>> What the patch does are:
>>
>> - This abuses ForeignScan->fdw_scan_tlist to store the additional
>> junk columns when foreign simple relation scan (that is, not a
>> join).
>
> I think that this issue was introduced in 9.3, which added postgres_fdw
> in combination with support for writable foreign tables, but
> fdw_scan_tlist was added to 9.5 as part of join-pushdown infrastructure,
> so my concern is that we might not be able to backpatch your patch to
> 9.3 and 9.4.

Another concern about this:

You wrote:
 >    Several places seems to be assuming that fdw_scan_tlist may be
 >    used foreign scan on simple relation but I didn't find that
 >    actually happens.

Yeah, currently, postgres_fdw and file_fdw don't use that list for 
simple foreign table scans, but it could be used to improve the 
efficiency for those scans, as explained in fdwhandler.sgml:

      Another <structname>ForeignScan</structname> field that can be 
filled by FDWs
      is <structfield>fdw_scan_tlist</structfield>, which describes the 
tuples returned by
      the FDW for this plan node.  For simple foreign table scans this 
can be
      set to <literal>NIL</literal>, implying that the returned tuples 
have the
      row type declared for the foreign table.  A 
non-<symbol>NIL</symbol> value must be a
      target list (list of <structname>TargetEntry</structname>s) 
containing Vars and/or
      expressions representing the returned columns.  This might be 
used, for
      example, to show that the FDW has omitted some columns that it noticed
      won't be needed for the query.  Also, if the FDW can compute 
expressions
      used by the query more cheaply than can be done locally, it could add
      those expressions to <structfield>fdw_scan_tlist</structfield>. 
Note that join
      plans (created from paths made by 
<function>GetForeignJoinPaths</function>) must
      always supply <structfield>fdw_scan_tlist</structfield> to 
describe the set of
      columns they will return.

You wrote:
 > I'm not sure whether the following ponits are valid.
 >
 > - If fdw_scan_tlist is used for simple relation scans, this would
 >    break the case. (ExecInitForeignScan,  set_foreignscan_references)

Some FDWs might already use that list for the improved efficiency for 
simple foreign table scans as explained above, so we should avoid 
breaking that.

If we take the Param-based approach suggested by Tom, I suspect there 
would be no need to worry about at least those things, so I'll try to 
update your patch as such, if there are no objections from you (or 
anyone else).

Best regards,
Etsuro Fujita