On 7 February 2018 at 07:30, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> At Tue, 6 Feb 2018 13:50:28 -0500, Robert Haas <robertmhaas@gmail.com> wrote in
<CA+TgmoYqdC+9U8mLYkUgM=CaBt6Pzz4R_YNboqDbW-LvUaHO+g@mail.gmail.com>
>> On Tue, Feb 6, 2018 at 11:32 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
>> > Yeah, I think it looks equally good that way, and like you said, the
>> > current code does it that way. So in the attached patch, I have
>> > swapped the two conditions.
>>
>> I prefer to avoid introducing 2 new variables and instead just prevent
>> the looping directly in the case where we started with a non-partial
>> plan.
>>
>> See attached. Does this look OK?
>
> Ah, we can bail out when starting from the first partial plan.
> It's a bit uneasy that pa_next_plan can be -1
I also feel that way. In the new condition (node->as_whichplan >
append->first_partial_plan), there is an implicit assumption that
INVALID_SUBPLAN_INDEX is equal to -1. So in our issue, if
node->as_whichplan is -1, this condition becomes false and so the loop
breaks correctly. But like I said, it works correctly because
INVALID_SUBPLAN_INDEX is defined to be less than zero. I guess, this
assumption might be safe in the long term also.
But another thing is that, node->as_whichplan can point to some
non-partial plan because it is the value of the earlier chosen plan.
And in that case (node->as_whichplan > append->first_partial_plan) is
false, indicating we should not wrap around. But there still might be
unfinished partial plans, and the pa_next_plan might be pointing to,
say, a plan at the tail end. Here, it still makes sense to wrap around
and see if there are unfinished partial plans. But the above condition
would not allow us to wrap around.
For this, we should check whether we have started with a partial plan.
That is, use this condition like how I used : (initial_plan >=
append->first_partial_plan) . Or else, if we don't want to add a new
variable, set node->as_whichplan to pa_next_plan initially.
Also, the first condition is not necessary here :
- else if (append->first_partial_plan < node->as_nplans)
+ else if (append->first_partial_plan < node->as_nplans &&
+ node->as_whichplan > append->first_partial_plan)
Just the fact that we are starting from a non-partial plan is enough
to determine that we should not wrap around, regardless of whether
there are any partial plans.
So we can just keep this single condition :
- else if (append->first_partial_plan < node->as_nplans)
+ else if (node->as_whichplan > append->first_partial_plan)
Attached is the patch accordingly.
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company