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

(2018/08/21 11:01), Kyotaro HORIGUCHI wrote:
> At Tue, 14 Aug 2018 20:49:02 +0900, Etsuro Fujita<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote in<5B72C1AE(dot)8010408(at)lab(dot)ntt(dot)co(dot)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