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

(2018/08/02 4:30), Robert Haas wrote:
> On Wed, Aug 1, 2018 at 7:44 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> I updated the patch that way. Updated patch attached. I fixed a bug and
>> did a bit of cleanups as well.
>
> Looking this over from a technical point of view, I think it's better
> than what you proposed before but I still don't agree with the
> approach. I don't think there is any getting around the fact that
> converted whole-row vars are going to result in some warts. Ashutosh
> inserted most of the necessary warts in the original partition-wise
> join patch, but missed some cases in postgres_fdw; his approach is,
> basically, to add the matching warts there. Your approach is to rip
> out all of those warts and insert different ones. You've simplified
> build_tlist_index_other_vars() and add_placeholders_to_joinrel() and
> build_joinrel_tlist() to basically what they were before
> partition-wise join went in. On the other hand, RelOptInfo grows
> three new fields,

Sorry, I forgot to remove one of the fields that isn't needed anymore.
RelOptInfo needs two new fields, in the new approach.

> set_append_rel_size() ends up more complicated than
> it was before when you include the node code added in the
> build_childrel_tlist function it calls, build_child_joinrel() has a
> different set of complications, and most importantly
> create_append_path() and create_merge_append_path() now do surgery on
> their input paths that knows specifically about the
> converted-whole-row var case. I would be glad to be rid of the stuff
> that you're ripping out, but in terms of complexity, I don't think we
> really come out ahead with the stuff you're adding.

The new approach seems to me more localized than what Ashutosh had
proposed. One obvious advantage of the new approach is that we no
longer need changes to postgres_fdw for it to work for partitionwise
joins, which would apply for any other FDWs, making the FDW authors'
life easy.

> I'm also still concerned about the design. In your old approach, you
> were creating the paths with with the wrong target list and then
> fixing it when you turned the paths into a plan. In your new
> approach, you're still creating the paths with the wrong target list,
> but now you're fixing it when you put an Append or MergeAppend path on
> top of them. I still think it's a bad idea to have anything other
> than the correct paths in the target list from the beginning.

I don't think so; actually, the child's targetlist would not have to be
the same as the parent's until the child gets in under the parent's
Append or MergeAppend. So, I modified the child's target list so as to
make it easy to join the child relations.

> For one
> thing, what happens if no Append or MergeAppend is ever put on top of
> them? One way that can happen today is partition-wise aggregate,
> although I haven't been able to demonstrate a bug, so maybe that's
> covered somehow.

Right. In the new approach, the targetlist for the topmost child
scan/join is guaranteed to be the same as that for the topmost parent
scan/join by the planner work that I added.

> But in general, with your approach, any code that
> looks at the tlist for a child-join has to know that the tlist is to
> be used as-is *except that* ConvertRowtypeExpr may need to be
> inserted. I think that special case could be easy to overlook, and it
> seems pretty arbitrary.

I think we can check to see whether the conversion is needed, from the
flags added to RelOptInfo.

> A tlist is a list of arbitrary expressions to
> be produced; with this patch, we'd end up with the tlist being the
> list of expressions to be produced in every case *except for*
> child-joins containing whole-row-vars. I just can't get behind a
> strange exception like that.

I have to admit that it's weird to adjust the child's targetlist in that
case when putting the child under the parent's Append/MergeAppend, but
IMHO I think that would be much localized.

Thanks for the comments!

Best regards,
Etsuro Fujita