Hello. At Fri, 15 Jun 2018 11:19:21 +0530, Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> wrote in <CAFjFpRd+7h7FrZC1NKLfizXJM=bjykrh8yezzx7exjpqdi2...@mail.gmail.com> > On Tue, Jun 12, 2018 at 8:49 AM, Kyotaro HORIGUCHI > <horiguchi.kyot...@lab.ntt.co.jp> wrote: > > Thanks for the discussion. > > > > At Thu, 7 Jun 2018 19:16:57 +0530, Ashutosh Bapat > > <ashutosh.ba...@enterprisedb.com> wrote in > > <CAFjFpRd+Bz-DwpnwsY_3uFkALmQgDRTdp_DKhxgm1H20dXs=o...@mail.gmail.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.ba...@enterprisedb.com> wrote in > >> > <CAFjFpRdraYcQnD4tKzNuP1uP6L-gnizi4HLU_UA=28q2m4z...@mail.gmail.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_pe175ittbfk6t8jxzwkra...@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
