Re: Oddity in tuple routing for foreign partitions

Hello.

At Tue, 24 Apr 2018 15:49:20 -0400, Robert Haas <robertmhaas(at)gmail(dot)com> wrote in <CA+Tgmob+CiHt+9JEUiXzNVmUcUf1zBb-bUeffVmbZWJHV0YVtw(at)mail(dot)gmail(dot)com>
> On Tue, Apr 24, 2018 at 12:21 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Mon, Apr 23, 2018 at 11:25 AM, Alvaro Herrera
> > <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> >> Robert, I think this is your turf, per 3d956d9562aa. Are you looking
> >> into it?
> >
> > Thanks for the ping. I just had a look over the proposed patch and I
> > guess I don't like it very much. Temporarily modifying the range
> > table in place and then changing it back before we return seems ugly
> > and error-prone. I hope we can come up with a solution that doesn't
> > involve needing to do that.
>
> I have done some refactoring to avoid that. See attached.
>
> I didn't really get beyond the refactoring stage with this today.
> This version still seems to work, but I don't really understand the
> logic in postgresBeginForeignInsert which decides whether to use the
> RTE from the range table or create our own. We seem to need to do one
> sometimes and the other sometimes, but I don't know why that is,
> really. Nor do I understand why we need the other changes in the
> patch. There's probably a good reason, but I haven't figured it out
> yet.

FWIW, the refactoring drops the condition that the ForeignInsert
is a part of UPDATE. The code exists just for avoiding temprary
RTE generation in 2 cases out of the 3 possible cases mentioned
in [1]. If it looks too complex for the gain, we can always
create an RTE for deparsing as Fujita-san's first patch in this
thread did. Anyway the condition for "dostuff" + "is update"
might be a bit too arbitrary.

[1] https://www.postgresql.org/message-id/f970d875-9711-b8cb-f270-965fa3e40334@lab.ntt.co.jp

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center