Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Aug 3, 2018 at 10:07 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> As far as I can see from the example that started this thread,
>> postgres_fdw injects WRVs into a PWJ whether or not the query involves
>> FOR UPDATE; that's why this bug is reproducible in a query without FOR
>> UPDATE. But we shouldn't need any EPQ support in that case.

> I might be missing something, but the original reproducer involves a
> FOR UPDATE clause, and the expected regression test output in
> postgres_fdw.out appears to show a whole-row var in the foreign scan's
> target list only in those examples where a locking clause is present.

Oh, sigh, never mind that --- I was thinking that only the first of
the two example queries involved FOR UPDATE, but they both do.
So no mystery there after all.

Anyway, what I'm basically suggesting is that we just disable support for
PWJ in the problematic cases in v11. As long as PWJ isn't even on by
default, that's not much of a loss. Obviously we'll want to fix it in the
future, but the hour grows late for v11, and I think either of these
patches would need quite a bit more work to be committable.

BTW, I forgot to respond to this:

>> I do not trust Ashutosh's patch because of the point you noted that it
>> will kick in on ConvertRowtypeExprs that are not related to partitioning.
> I don't remember making that point -- can you clarify?

I'm looking at
https://www.postgresql.org/message-id/CA%2BTgmoZSaKq-fYALn5jf6c_X3%3D%3DRb2s8eqLDwGpV%3DLNNhTXYwg%40mail.gmail.com
wherein you said

> There are definitely some things not to like about this approach. In
> particular, I definitely agree that treating a converted whole-row
> reference specially is not very nice. It feels like it might be
> substantially cleaner to be able to represent this idea as a single
> node rather than a combination of ConvertRowtypeExpr and var with
> varattno = 0. Perhaps in the future we ought to consider either (a)
> trying to use a RowExpr for this rather than ConvertRowtypeExpr or (b)
> inventing a new WholeRowExpr node that stores two RTIs, one for the
> relation that's generating the whole-row reference and the other for
> the relation that is controlling the column ordering of the result or
> (c) allowing a Var to represent a whole-row expression that has to
> produce its outputs according to the ordering of some other RTE. But
> I don't think it's wise or necessary to whack that around just to fix
> this bug; it is refactoring or improvement work best left to a future
> release.

I agree with all of that except your conclusion that it's unnecessary
to do it to fix this bug. I find that entirely unsupported and over-
optimistic, especially in view of the number of iterations Ashutosh
went through trying to fix the fallout from not making a clear
distinction.

regards, tom lane