Define variables in the approprieate scope

Started by Antonin Houskaover 6 years ago6 messageshackers
Beta feature

Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.

won't retrysuccessCI history

You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:

docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t41981
psql -h localhost -U postgres

Built from patchset v1 (message #1), July 28, 2026 at 03:03 AM.

Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:

git clone --branch t41981_1 https://github.com/hackorum-dev/postgres.git

In a checkout you already have, add the fork once:

git remote add hackorum https://github.com/hackorum-dev/postgres.git

then, for this patchset and every later one:

git fetch hackorum t41981_1 && git checkout t41981_1

Patchset v1 (message #1) is on t41981_1

Jump to latest
#1Antonin Houska
ah@cybertec.at

I've noticed that two variables in RelationCopyStorage() are defined in a
scope higher than necessary. Please see the patch.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

Attachments:

t41981_1
variable_scope.patchtext/x-diffDownload+3-4
#2Bruce Momjian
bruce@momjian.us
In reply to: Antonin Houska (#1)
Re: Define variables in the approprieate scope

On Tue, Feb 25, 2020 at 09:35:52AM +0100, Antonin Houska wrote:

I've noticed that two variables in RelationCopyStorage() are defined in a
scope higher than necessary. Please see the patch.

It seems cleaner to me to allocate the variables once before the loop
starts, rather than for each loop iteration.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +
#3Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Bruce Momjian (#2)
Re: Define variables in the approprieate scope

On 2020-Mar-18, Bruce Momjian wrote:

On Tue, Feb 25, 2020 at 09:35:52AM +0100, Antonin Houska wrote:

I've noticed that two variables in RelationCopyStorage() are defined in a
scope higher than necessary. Please see the patch.

It seems cleaner to me to allocate the variables once before the loop
starts, rather than for each loop iteration.

If we're talking about personal preference, my own is what Antonin
shows. However, since disagreement has been expressed, I think we
should only change it if the generated code turns out better.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#4Bruce Momjian
bruce@momjian.us
In reply to: Alvaro Herrera (#3)
Re: Define variables in the approprieate scope

On Mon, Mar 23, 2020 at 01:00:24PM -0300, Alvaro Herrera wrote:

On 2020-Mar-18, Bruce Momjian wrote:

On Tue, Feb 25, 2020 at 09:35:52AM +0100, Antonin Houska wrote:

I've noticed that two variables in RelationCopyStorage() are defined in a
scope higher than necessary. Please see the patch.

It seems cleaner to me to allocate the variables once before the loop
starts, rather than for each loop iteration.

If we're talking about personal preference, my own is what Antonin
shows. However, since disagreement has been expressed, I think we
should only change it if the generated code turns out better.

I am fine with either usage, frankly. I was just pointing out what
might be the benefit of the current coding.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +
#5Michael Paquier
michael@paquier.xyz
In reply to: Bruce Momjian (#4)
Re: Define variables in the approprieate scope

On Mon, Mar 23, 2020 at 08:50:55PM -0400, Bruce Momjian wrote:

On Mon, Mar 23, 2020 at 01:00:24PM -0300, Alvaro Herrera wrote:

If we're talking about personal preference, my own is what Antonin
shows. However, since disagreement has been expressed, I think we
should only change it if the generated code turns out better.

I am fine with either usage, frankly. I was just pointing out what
might be the benefit of the current coding.

Personal opinion here. I tend to prefer putting variable declarations
into the inner portions because it makes it easier to reason about the
code, though I agree that this concept does not need to be applied all
the time.
--
Michael

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Michael Paquier (#5)
Re: Define variables in the approprieate scope

Michael Paquier <michael@paquier.xyz> writes:

On Mon, Mar 23, 2020 at 08:50:55PM -0400, Bruce Momjian wrote:

I am fine with either usage, frankly. I was just pointing out what
might be the benefit of the current coding.

Personal opinion here. I tend to prefer putting variable declarations
into the inner portions because it makes it easier to reason about the
code, though I agree that this concept does not need to be applied all
the time.

My vote is to not make this sort of change until there's another
reason to touch the code in question. All changes create hazards for
back-patching, and I don't think this change is worth it on its own.
But if there are going to be diffs in the immediate vicinity anyway,
then sure.

(I'm feeling a bit sensitized to this, perhaps, because of recent
unpleasant experience with back-patching b4570d33a. That didn't touch
very much code, and the functions in question seemed like fairly stagnant
backwaters of the code base, so it should not have been painful to
back-patch ... but it was, because of assorted often-cosmetic changes
in said code.)

regards, tom lane