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

(2018/07/26 5:27), Robert Haas wrote:
> 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?

What I had in mind was to insert a Rusult node with
inject_projection_plan and adjust the tlist of the Result, as done for
adding sort columns to a tlist in prepare_sort_from_pathkeys.

>>>>> 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 think the data used by such a costing function is computed by
set_rel_size in set_append_rel_size, not set_pathtarget_cost_width; in
the case of a plain partition, for example, set_rel_size would call
set_plain_rel_size, and then set_plain_rel_size would eventually call
set_rel_width, which sets reltarget->cost, which I think would be used
by e.g., cost_seqscan. cost_qual_eval_node, which is called in
set_rel_width for computing the cost of ConvertRowTypeExpr, ignores that
expression, so currently, we don't charge any cost for it to the
partition's reltarget->cost, and to the cost of a scan below the Append.

>> 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.

I agree on that point.

>> 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.

I still don't like that because in my opinion, changes needed for that
would not be localized, and that would make code complicated more than
necessary.

As I mentioned in a previous email, another idea to avoid that would be
to adjust tlists for children at path creation time, not plan creation
time; we could adjust the tlist for each of subpaths accumulated for an
Append/MergeAppend path in add_paths_to_append_rel called from
set_append_rel_pathlist or generate_partitionwise_join_paths, with
create_projection_path adding ConvertRowTypeExpr. It seems unlikely
that performing create_projection_path to such a subpath would change
its property of being the cheapest, so it would be safe to adjust the
tlists that way. This would not require making create_plan complicated
anymore. I might be missing something, though.

Best regards,
Etsuro Fujita