Re: Problem while updating a foreign table pointing to a partitionedtable on foreign server

(2018/08/21 11:01), Kyotaro HORIGUCHI wrote:
> At Tue, 14 Aug 2018 20:49:02 +0900, Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp>  wrote
in<5B72C1AE.8010408@lab.ntt.co.jp>
>> (2018/08/09 22:04), Etsuro Fujita wrote:
>>> (2018/08/08 17:30), Kyotaro HORIGUCHI wrote:

>> 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.

> The exapansion should be safe if the expanded descriptor has the
> same defitions for base columns and all the extended coulumns are
> junks. The junk columns should be ignored by unrelated nodes and
> they are passed safely as far as ForeignModify passes tuples as
> is from underlying ForeignScan to ForeignUpdate/Delete.

I'm not sure that would be really safe.  Does that work well when 
EvalPlanQual, for example?

>> 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.
>
> https://www.postgresql.org/docs/devel/static/fdw-planning.html
>
> Hmm. Thanks for the pointer, it seems to need rewrite. However,
> it doesn't seem to work for non-join foreign scans, since the
> core igonres it and uses local table definition.

Really?

>> 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.
>
> I considered to use fdw_scan_tlist in that way but the core is
> assuming that foreign scans with scanrelid>  0 uses the relation
> descriptor.

Could you elaborate a bit more on this?

> Do you have any example for that?

I don't know such an example, but in my understanding, the core allows 
the FDW to do 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).

> PARAM_EXEC is single storage side channel that can work as far as
> it is set and read while each tuple is handled. In this case
> postgresExecForeignUpdate/Delete must be called before
> postgresIterateForeignScan returns the next tuple. An apparent
> failure case for this usage is the join-update case below.
>
> https://www.postgresql.org/message-id/20180605.191032.256535589.horiguchi.kyotaro@lab.ntt.co.jp

What I have in mind would be to 1) create a tlist that contains not only 
Vars/PHVs but Params, for each join rel involving the target rel so we 
ensure that the Params will propagate up through all join plan steps, 
and 2) convert a join rel's tlist Params into Vars referencing the same 
Params in the tlists for the outer/inner rels, by setrefs.c.  I think 
that would probably work well even for the case you mentioned above. 
Maybe I'm missing something, though.

Sorry for the delay.

Best regards,
Etsuro Fujita