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

(2018/08/30 20:37), Kyotaro HORIGUCHI wrote:
> At Fri, 24 Aug 2018 21:45:35 +0900, Etsuro Fujita<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote in<5B7FFDEF(dot)6020302(at)lab(dot)ntt(dot)co(dot)jp>
>> (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?
>
> Nothing. The reason was that core just doesn't know about the
> extended portion. So only problematic case was
> ExprEvalWholeRowVar, where explicit sanity check is
> perfomed. But, I think it is a ugly wart as you said. So the
> latest patch generates full fdw_scan_tlist.

Will review.

>>>> 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.
>
> As I wrote above, the problem was not param id propagation but
> the per-query storage for a parameter holded in econtext.
>
> PARAM_EXEC is assumed to be used between outer and inner
> relations of a nestloop or retrieval from sub-query retrieval as
> commented in primnodes.h.
>
>> PARAM_EXEC: The parameter is an internal executor parameter, used
>> for passing values into and out of sub-queries or from
>> nestloop joins to their inner scans.
>> For historical reasons, such parameters are numbered from 0.
>> These numbers are independent of PARAM_EXTERN numbers.

Yeah, but IIUC, I think that #2 would allow us to propagate up the param
values, not the param ids.

> I'm waiting for the patch.

OK, but I will review your patch first.

Best regards,
Etsuro Fujita