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]https://wiki.sei.cmu.edu/confluence/display/c/EXP46-C.+Do+not+use+a+bitwise+operator+with+a+Boolean-like+operand.
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
https://wiki.sei.cmu.edu/confluence/display/c/EXP46-C.+Do+not+use+a+bitwise+operator+with+a+Boolean-like+operand
Attachments:
001-avoid-call-function-typo-shortcut.patchapplication/octet-stream; name=001-avoid-call-function-typo-shortcut.patchDownload
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index 5c6f646eff..f26d796e52 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -612,7 +612,7 @@ parallel_vacuum_process_all_indexes(ParallelVacuumState *pvs, int num_index_scan
Assert(indstats->status == PARALLEL_INDVAC_STATUS_INITIAL);
indstats->status = new_status;
indstats->parallel_workers_can_process =
- (pvs->will_parallel_vacuum[i] &
+ (pvs->will_parallel_vacuum[i] &&
parallel_vacuum_index_is_parallel_safe(pvs->indrels[i],
num_index_scans,
vacuum));
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.
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.
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
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.
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
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