Horton

Thread

Commits

  1. Use logical operator && instead of & in vacuumparallel.c.

  1. Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2022-08-19T12:04:57Z

    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

[1]
https://wiki.sei.cmu.edu/confluence/display/c/EXP46-C.+Do+not+use+a+bitwise+operator+with+a+Boolean-like+operand

  2. Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-08-19T12:09:37Z

    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/

  3. Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

    Amit Kapila <amit.kapila16@gmail.com> — 2022-08-19T12:13:46Z

    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.

  4. Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-19T13:28:19Z

    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

  5. Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2022-08-19T14:15:33Z

    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

results:
res=0
res=0

So, my assumption is incorrect.

regards,
Ranier Vilela

  6. Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

    Amit Kapila <amit.kapila16@gmail.com> — 2022-08-20T04:02:57Z

    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.

  7. Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-20T04:34:41Z

    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

  8. Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2022-08-20T14:07:59Z

    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.



> However,
> I am not sure if we should backpatch this to PG15 as this won't lead
> to any incorrect behavior.
>
+1 for backpath to PG15, too.
It's certainly a safe change.

regards,
Ranier Vilela

  9. Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

    Amit Kapila <amit.kapila16@gmail.com> — 2022-08-22T04:41:55Z

    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.

  10. Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2022-08-22T11:19:22Z

    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