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

On Tue, Jul 24, 2018 at 7:51 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Isn't that assumption fundamental to your whole approach?
>
> I don't think so. What I mean here is: currently the subplan would be a
> scan/join node, but in future we might have eg, a Sort node atop the
> scan/join node, so it would be better to update the patch to handle such a
> case as well.

But how would you do that?

>>>> I think that's a bad idea. The target list affects lots
>>>> of things, like costing. If we don't insert a ConvertRowTypeExpr into
>>>> the child's target list, the costing will be wrong to the extent that
>>>> ConvertRowTypeExpr has any cost, which it does.
>>>
>>>
>>> Actually, this is not true at least currently, because
>>> set_append_rel_size
>>> doesn't do anything about the costing:
>>
>>
>> Why would it? Append can't project, so the cost of any expressions
>> that appear in its target list is irrelevant. What is affected is the
>> cost of the scans below the Append -- see e.g. cost_seqscan(), which
>> uses the data produced by set_pathtarget_cost_width().
>
> By set_rel_size()?

Sorry, I don't understand what you mean by this.

> I'm not sure that's a good idea, because I think we have a trade-off
> relation; the more we make create_plan simple, the more we need to make
> earlier states of the planner complicated.
>
> And it looks to me like the partitionwise join code is making earlier (and
> later) stages of the planner too complicated, to make create_plan simple.

I think that create_plan is *supposed* to be simple. Its purpose is
to prune away data that's only needed during planning and add things
that can be computed at the last minute which are needed at execution
time. Making it do anything else is, in my opinion, not good.

> When considering paritionwise joining, it would make things complicated to
> have a ConvertRowtypeExpr in a partrel's targetlist, because as discussed
> upthread, it deviates from the planner's assumption that a rel's targetlist
> would only include Vars and PHVs. So, I propose to include a child
> whole-row Var in the targetlist instead, in which case, we need to fix the
> Var after the fact, but can avoid making many other parts of the planner
> complicated.

Well, I could have the wrong idea here, but I tend to think allowing
for ConvertRowTypeExpr elsewhere won't be that bad.

Does anyone else want to weigh in on this issue? Tom, perhaps?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company