public inbox for [email protected]
help / color / mirror / Atom feedFrom: Etsuro Fujita <[email protected]>
To: Robert Haas <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Ashutosh Bapat <[email protected]>
Cc: Rajkumar Raghuwanshi <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Tom Lane <[email protected]>
Subject: Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.
Date: Thu, 26 Jul 2018 21:11:01 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+TgmoazFzdp5ZVwUjk_b2cuB90WCeeWtzt1ShSbXGyg67c+Zw@mail.gmail.com>
References: <[email protected]>
<CAFjFpRfGL9w-4C5N-ioaG3m+iheC1ey7_RMZ4vJijGn=wWJyxw@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CAFjFpRffA-BfL0RXcUVLLbcOBF1ie7oObTB-RE2q_o45X1HTYg@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CA+TgmoZboTszGQk3OecBOZUFNNjzKE-SOo_Q46xtTJ4Yj95-RQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CA+TgmobR9XWEhmF3s_WsoLNnnjaFyS_xtbrVG3dCGiUgof20Nw@mail.gmail.com>
<CA+TgmoZSaKq-fYALn5jf6c_X3==Rb2s8eqLDwGpV=LNNhTXYwg@mail.gmail.com>
<[email protected]>
<CA+TgmoYYTCQL=TUYbUz2b=CsQeB4o1L2+_=zHynxGVmyKUtH1A@mail.gmail.com>
<[email protected]>
<CA+TgmoazFzdp5ZVwUjk_b2cuB90WCeeWtzt1ShSbXGyg67c+Zw@mail.gmail.com>
(2018/07/26 5:27), Robert Haas wrote:
> On Tue, Jul 24, 2018 at 7:51 AM, Etsuro Fujita
> <[email protected]> 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
view thread (115+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox