Re: Define variables in the approprieate scope
Michael Paquier 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
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 signature.asc Description: PGP signature
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 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 +
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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 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 +
Define variables in the approprieate scope
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
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index fddfbf1d8c..14d170823f 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -336,15 +336,11 @@ void
RelationCopyStorage(SMgrRelation src, SMgrRelation dst,
ForkNumber forkNum, char relpersistence)
{
- PGAlignedBlock buf;
- Page page;
bool use_wal;
bool copying_initfork;
BlockNumber nblocks;
BlockNumber blkno;
- page = (Page) buf.data;
-
/*
* The init fork for an unlogged relation in many respects has to be
* treated the same as normal relation, changes need to be WAL logged and
@@ -364,6 +360,9 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst,
for (blkno = 0; blkno < nblocks; blkno++)
{
+ PGAlignedBlock buf;
+ Page page = (Page) buf.data;
+
/* If we got a cancel signal during the copy of the data, quit */
CHECK_FOR_INTERRUPTS();
