From: | Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled. |
Date: | 2018-07-26 12:11:01 |
Message-ID: | 5B59BA55.30200@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
(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
From | Date | Subject | |
---|---|---|---|
Next Message | Andreas Karlsson | 2018-07-26 12:51:53 | Re: Early WIP/PoC for inlining CTEs |
Previous Message | Michael Banck | 2018-07-26 11:59:33 | Online verification of checksums |