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

Hello.

At Fri, 15 Jun 2018 11:19:21 +0530, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote in <CAFjFpRd+7h7FrZC1NKLfizXJM=bjyKrh8YezZX7ExjpQdi28Tw(at)mail(dot)gmail(dot)com>
> 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>
> >> 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.

You should have seen make_tuple_from_result_row. The patch sets
real tableOid to returning tuples since I didn't find an usable
storage for the per-tuple value. Afterwards the parameters are
set from tup->t_tableOid in postgresIterateForeignScan.

ForeignNext overwrites t_tableOid of returned tuples with the
foreign table's OID if system column is requested.

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

I think treating a foreign table as a local object is right. But
anyway it doesn't work.

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

Maybe. I agree.

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

After some struggles as you know, I agree to the opinion. As my
first impression, giving (physical) base relations (*1) an
ability to have junk attribute is rather straightforward.

Well, is our conclusion here like this?

- For existing versions, check the errorneous situation and ERROR out.
(documentaion will be needed.)

- For 12, we try the above thing.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center