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

On Tue, Jun 12, 2018 at 8:49 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Thanks for the discussion.
>
> At Thu, 7 Jun 2018 19:16:57 +0530, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote in <CAFjFpRd+Bz-DwpnwsY_3uFkALmQgDRTdp_DKhxgm1H20dXs=ow(at)mail(dot)gmail(dot)com>
>> On Tue, Jun 5, 2018 at 3:40 PM, Kyotaro HORIGUCHI
>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> > Hello.
>> >
>> > At Mon, 04 Jun 2018 20:58:28 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20180604(dot)205828(dot)208262556(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
>> >> It fails on some join-pushdown cases since it doesn't add tid
>> >> columns to join tlist. I suppose that build_tlist_to_deparse
>> >> needs something but I'll consider further tomorrow.
>> >
>> > I made it work with a few exceptions and bumped. PARAM_EXEC
>> > doesn't work at all in a case where Sort exists between
>> > ForeignUpdate and ForeignScan.
>> >
>> > =====
>> > explain (verbose, costs off)
>> > update bar set f2 = f2 + 100
>> > from
>> > ( select f1 from foo union all select f1+3 from foo ) ss
>> > where bar.f1 = ss.f1;
>> > QUERY PLAN
>> > -----------------------------------------------------------------------------
>> > Update on public.bar
>> > Update on public.bar
>> > Foreign Update on public.bar2
>> > Remote SQL: UPDATE public.loct2 SET f2 = $3 WHERE tableoid = $1 AND ctid = $2
>> > ...
>> > -> Merge Join
>> > Output: bar2.f1, (bar2.f2 + 100), bar2.f3, (ROW(foo.f1))
>> > Merge Cond: (bar2.f1 = foo.f1)
>> > -> Sort
>> > Output: bar2.f1, bar2.f2, bar2.f3, bar2.tableoid, bar2.ctid
>> > Sort Key: bar2.f1
>> > -> Foreign Scan on public.bar2
>> > Output: bar2.f1, bar2.f2, bar2.f3, bar2.tableoid, bar2.ctid
>> > Remote SQL: SELECT f1, f2, f3, ctid, tableoid FROM public.loct2 FOR UPDATE
>> > =====
>>
>> What's the problem that you faced?
>
> The required condtion for PARAM_EXEC to work is that executor
> ensures the correspondence between the setter the reader of a
> param like ExecNestLoop is doing. The Sort node breaks the
> correspondence between the tuple obtained from the Foreign Scan
> and that ForeignUpdate is updating. Specifically Foreign Update
> upadtes the first tuple using the tableoid for the last tuple
> from the Foreign Scan.

Ok. Thanks for the explanation.

>
>> > Even if this worked fine, it cannot be back-patched. We need an
>> > extra storage moves together with tuples or prevent sorts or
>> > something like from being inserted there.
>>
>> I think your approach still has the same problem that it's abusing the
>> tableOid field in the heap tuple to store tableoid from the remote as
>> well as local table. That's what Robert and Tom objected to [1], [2]
>
> It's wrong understanding. PARAM_EXEC conveys remote tableoids
> outside tuples and each tuple is storing correct (= local)
> tableoid.

In the patch I saw that we were setting tableoid field of HeapTuple to
the remote table oid somewhere. Hence the comment. I might be wrong.

>
>> > At Fri, 1 Jun 2018 10:21:39 -0400, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote in <CAFjFpRdraYcQnD4tKzNuP1uP6L-gnizi4HLU_UA=28Q2M4zoDA(at)mail(dot)gmail(dot)com>
>> >> I am not suggesting to commit 0003 in my patch set, but just 0001 and
>> >> 0002 which just raise an error when multiple rows get updated when
>> >> only one row is expected to be updated.
>> >
>> > So I agree to commit the two at least in order to prevent doing
>> > wrong silently.
>>
>> I haven't heard any committer's opinion on this one yet.
>>
>> [1] https://www.postgresql.org/message-id/CA+TgmobUHHZiDR=HCU4n30yi9_PE175itTbFK6T8JxzwkRAWAg@mail.gmail.com
>> [2] https://www.postgresql.org/message-id/7936.1526590932%40sss.pgh.pa.us
>
> Agreed. We need any comment to proceed.
>
> I have demonstrated and actually shown a problem of the
> PARAM_EXEC case. (It seems a bit silly that I actually found the
> problem after it became almost workable, though..)

I think the general idea behind Tom's suggestion is that we have to
use some node other than Var node when we update the targetlist with
junk columns. He suggested Param since that gives us some place to
store remote tableoid. But if that's not working, another idea (that
Tom mentioned during our discussion at PGCon) is to invent a new node
type like ForeignTableOid or something like that, which gets deparsed
to "tableoid" and evaluated to the table oid on the foreign server.
That will not work as it is since postgres_fdw code treats a foreign
table almost like a local table in many ways e.g. it uses attr_used to
know which attributes are to be requested from the foreign server,
build_tlist_to_deparse() only pulls Var nodes from the targelist of
foreign table and so on. All of those assumptions will need to change
with this approach. But good thing is because of join and aggregate
push-down we already have ability to push arbitrary kinds of nodes
down to the foreign server through the targetlist. We should be able
to leverage that capability. It looks like a lot of change, which
again doesn't seem to be back-portable.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company