Обсуждение: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)
Hi,
At function parallel_vacuum_process_all_indexes there is
a typo with a logical connector.
I think that correct is &&, because both of the operators are
bool types [1].
As a result, parallel vacuum workers can be incorrectly enabled.
Attached a trivial fix.
regards,
Ranier Vilela
Вложения
Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)
От
Bharath Rupireddy
Дата:
On Fri, Aug 19, 2022 at 5:35 PM Ranier Vilela <ranier.vf@gmail.com> wrote: > > Hi, > > At function parallel_vacuum_process_all_indexes there is > a typo with a logical connector. > > I think that correct is &&, because both of the operators are > bool types [1]. > > As a result, parallel vacuum workers can be incorrectly enabled. > > Attached a trivial fix. > > [1] https://wiki.sei.cmu.edu/confluence/display/c/EXP46-C.+Do+not+use+a+bitwise+operator+with+a+Boolean-like+operand Good catch! Patch LGTM. -- Bharath Rupireddy RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/
On Fri, Aug 19, 2022 at 5:40 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Fri, Aug 19, 2022 at 5:35 PM Ranier Vilela <ranier.vf@gmail.com> wrote: > > > > Hi, > > > > At function parallel_vacuum_process_all_indexes there is > > a typo with a logical connector. > > > > I think that correct is &&, because both of the operators are > > bool types [1]. > > > > As a result, parallel vacuum workers can be incorrectly enabled. > > > > Attached a trivial fix. > > > > [1] https://wiki.sei.cmu.edu/confluence/display/c/EXP46-C.+Do+not+use+a+bitwise+operator+with+a+Boolean-like+operand > > Good catch! Patch LGTM. > +1. This looks fine to me as well. I'll take care of this early next week unless someone thinks otherwise. -- With Regards, Amit Kapila.
Ranier Vilela <ranier.vf@gmail.com> writes: > At function parallel_vacuum_process_all_indexes there is > a typo with a logical connector. > I think that correct is &&, because both of the operators are > bool types [1]. > As a result, parallel vacuum workers can be incorrectly enabled. Since they're bools, the C spec requires them to promote to integer 0 or 1, therefore the & operator will yield the desired result. So there's not going to be any incorrect behavior. Nonetheless, I agree that && would be better, because it would short-circuit the evaluation of parallel_vacuum_index_is_parallel_safe() when there's no need. regards, tom lane
Em sex., 19 de ago. de 2022 às 10:28, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier.vf@gmail.com> writes:
> At function parallel_vacuum_process_all_indexes there is
> a typo with a logical connector.
> I think that correct is &&, because both of the operators are
> bool types [1].
> As a result, parallel vacuum workers can be incorrectly enabled.
Since they're bools, the C spec requires them to promote to integer
0 or 1, therefore the & operator will yield the desired result.
So there's not going to be any incorrect behavior.
It seems that you are right.
#include <stdio.h>
#ifdef __cplusplus
extern "C" {
#endif
int main()
{
bool op1 = false;
bool op2 = true;
bool band;
bool cand;
band = op1 & op2;
printf("res=%d\n", band);
cand = op1 && op2;
printf("res=%d\n", cand);
}
#ifdef __cplusplus
}
#endif
#ifdef __cplusplus
extern "C" {
#endif
int main()
{
bool op1 = false;
bool op2 = true;
bool band;
bool cand;
band = op1 & op2;
printf("res=%d\n", band);
cand = op1 && op2;
printf("res=%d\n", cand);
}
#ifdef __cplusplus
}
#endif
results:
res=0
res=0
So, my assumption is incorrect.
regards,
Ranier Vilela
On Fri, Aug 19, 2022 at 7:45 PM Ranier Vilela <ranier.vf@gmail.com> wrote: > > Em sex., 19 de ago. de 2022 às 10:28, Tom Lane <tgl@sss.pgh.pa.us> escreveu: >> >> Ranier Vilela <ranier.vf@gmail.com> writes: >> > At function parallel_vacuum_process_all_indexes there is >> > a typo with a logical connector. >> > I think that correct is &&, because both of the operators are >> > bool types [1]. >> > As a result, parallel vacuum workers can be incorrectly enabled. >> >> Since they're bools, the C spec requires them to promote to integer >> 0 or 1, therefore the & operator will yield the desired result. >> So there's not going to be any incorrect behavior. > > > So, my assumption is incorrect. > Right, but as Tom pointed it is still better to change this. However, I am not sure if we should backpatch this to PG15 as this won't lead to any incorrect behavior. -- With Regards, Amit Kapila.
Amit Kapila <amit.kapila16@gmail.com> writes: > Right, but as Tom pointed it is still better to change this. However, > I am not sure if we should backpatch this to PG15 as this won't lead > to any incorrect behavior. If that code only exists in HEAD and v15 then I'd backpatch. It's a very low-risk change and it might avoid merge problems for future backpatches. regards, tom lane
Em sáb., 20 de ago. de 2022 às 01:03, Amit Kapila <amit.kapila16@gmail.com> escreveu:
On Fri, Aug 19, 2022 at 7:45 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Em sex., 19 de ago. de 2022 às 10:28, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
>>
>> Ranier Vilela <ranier.vf@gmail.com> writes:
>> > At function parallel_vacuum_process_all_indexes there is
>> > a typo with a logical connector.
>> > I think that correct is &&, because both of the operators are
>> > bool types [1].
>> > As a result, parallel vacuum workers can be incorrectly enabled.
>>
>> Since they're bools, the C spec requires them to promote to integer
>> 0 or 1, therefore the & operator will yield the desired result.
>> So there's not going to be any incorrect behavior.
>
>
> So, my assumption is incorrect.
>
Right, but as Tom pointed it is still better to change this.
Sorry, I expressed myself badly.
As Tom pointed out, It's not a bug, as I stated in the first post.
But even if it wasn't a small performance improvement, by avoiding the function call.
The correct thing is to use logical connectors (&& ||) with boolean operands.
The correct thing is to use logical connectors (&& ||) with boolean operands.
However,+1 for backpath to PG15, too.
I am not sure if we should backpatch this to PG15 as this won't lead
to any incorrect behavior.
It's certainly a safe change.
regards,
Ranier Vilela
On Sat, Aug 20, 2022 at 10:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amit Kapila <amit.kapila16@gmail.com> writes: > > Right, but as Tom pointed it is still better to change this. However, > > I am not sure if we should backpatch this to PG15 as this won't lead > > to any incorrect behavior. > > If that code only exists in HEAD and v15 then I'd backpatch. > It's a very low-risk change and it might avoid merge problems > for future backpatches. > Okay, done that way. Thanks! -- With Regards, Amit Kapila.
Em seg., 22 de ago. de 2022 às 01:42, Amit Kapila <amit.kapila16@gmail.com> escreveu:
On Sat, Aug 20, 2022 at 10:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > Right, but as Tom pointed it is still better to change this. However,
> > I am not sure if we should backpatch this to PG15 as this won't lead
> > to any incorrect behavior.
>
> If that code only exists in HEAD and v15 then I'd backpatch.
> It's a very low-risk change and it might avoid merge problems
> for future backpatches.
>
Okay, done that way. Thanks!
Thank you.
regards,
Ranier Vilela