Remove redundant assignment in CreateWorkExprContext

Started by Chao Li8 months ago6 messageshackers
Jump to latest
#1Chao Li
li.evan.chao@gmail.com

Hi,

While discussing [1]/messages/by-id/CAA4eK1+SEus_6vQay9TF_r4ow+E-Q7LYNLfsD78HaOsLSgppxQ@mail.gmail.com, I was reading execUtils.c, then I noticed this
redundant local variable assignment in CreateWorkExprContext(). The
attached patch fixed that.

[1]: /messages/by-id/CAA4eK1+SEus_6vQay9TF_r4ow+E-Q7LYNLfsD78HaOsLSgppxQ@mail.gmail.com
/messages/by-id/CAA4eK1+SEus_6vQay9TF_r4ow+E-Q7LYNLfsD78HaOsLSgppxQ@mail.gmail.com

Best regards,

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Attachments:

v1-0001-Remove-redundant-assignemnt-in-CreateWorkExprCont.patchtext/plain; charset=UTF-8; name=v1-0001-Remove-redundant-assignemnt-in-CreateWorkExprCont.patchDownload+1-4
#2Andreas Karlsson
andreas.karlsson@percona.com
In reply to: Chao Li (#1)
Re: Remove redundant assignment in CreateWorkExprContext

On 8/21/25 5:47 AM, Chao Li wrote:

While discussing [1], I was reading execUtils.c, then I noticed this
redundant local variable assignment in CreateWorkExprContext(). The
attached patch fixed that.

Nice spotted but I think your patch reduces readability. How about this?

Andreas

Attachments:

v2-0001-Remove-redundant-assignemnt-in-CreateWorkExprCont.patchtext/x-patch; charset=UTF-8; name=v2-0001-Remove-redundant-assignemnt-in-CreateWorkExprCont.patchDownload+1-2
#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andreas Karlsson (#2)
Re: Remove redundant assignment in CreateWorkExprContext

Andreas Karlsson <andreas@proxel.se> writes:

On 8/21/25 5:47 AM, Chao Li wrote:

While discussing [1], I was reading execUtils.c, then I noticed this
redundant local variable assignment in CreateWorkExprContext(). The
attached patch fixed that.

Nice spotted but I think your patch reduces readability. How about this?

Looking at the git history, CreateWorkExprContext was introduced in
50a38f651, and at that time it did some nontrivial calculations
to adjust that initial value of maxBlockSize. Later, cc721c459
simplified matters but forgot to remove the now-useless
initialization. So +1, unless Jeff has some reason to keep it
like this?

regards, tom lane

#4Chao Li
li.evan.chao@gmail.com
In reply to: Andreas Karlsson (#2)
Re: Remove redundant assignment in CreateWorkExprContext

On Jan 14, 2026, at 15:13, Andreas Karlsson <andreas@proxel.se> wrote:

On 8/21/25 5:47 AM, Chao Li wrote:

While discussing [1], I was reading execUtils.c, then I noticed this redundant local variable assignment in CreateWorkExprContext(). The attached patch fixed that.

Nice spotted but I think your patch reduces readability. How about this?

Andreas
<v2-0001-Remove-redundant-assignemnt-in-CreateWorkExprCont.patch>

Hi Andraes,

Thank you very much for reviewing and bumping this patch. I’m fine with your tuning; I don’t have a strong preference, so I’m happy to leave the final decision to the committers.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

#5Jeff Davis
pgsql@j-davis.com
In reply to: Tom Lane (#3)
Re: Remove redundant assignment in CreateWorkExprContext

On Wed, 2026-01-14 at 02:33 -0500, Tom Lane wrote:

Looking at the git history, CreateWorkExprContext was introduced in
50a38f651, and at that time it did some nontrivial calculations
to adjust that initial value of maxBlockSize.  Later, cc721c459
simplified matters but forgot to remove the now-useless
initialization.  So +1, unless Jeff has some reason to keep it
like this?

Right. Committed, thank you.

Regards,
Jeff Davis

#6Bertrand Drouvot
bertranddrouvot.pg@gmail.com
In reply to: Jeff Davis (#5)
Re: Remove redundant assignment in CreateWorkExprContext

Hi,

On Wed, Jan 14, 2026 at 12:03:01PM -0800, Jeff Davis wrote:

On Wed, 2026-01-14 at 02:33 -0500, Tom Lane wrote:

Looking at the git history, CreateWorkExprContext was introduced in
50a38f651, and at that time it did some nontrivial calculations
to adjust that initial value of maxBlockSize.  Later, cc721c459
simplified matters but forgot to remove the now-useless
initialization.  So +1, unless Jeff has some reason to keep it
like this?

Right. Committed, thank you.

FWIW, this thread gave me the idea to check for such cases in the entire code
tree. I did it with the help of [1]https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/useless_assignment.cocci, and that would give:

"
85 files changed, 116 insertions(+), 116 deletions(-)
"

This is quite large, so if we think that's worth the time and energy we could
update a subset of files at a time per month.

That would:

- keep changes consistent within each file
- ease the review(s)
- avoid "large" rebases for patches waiting in the commitfest

Thoughts?

[1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/useless_assignment.cocci

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com