Date: Wed, 24 Aug 2022 09:00:27 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: pgsql-hackers@postgresql.org,
	Tomas Vondra <tomas.vondra@postgresql.org>,
	Peter Smith <smithpb2250@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>
Subject: Re: shadow variables - pg15 edition
Message-ID: <20220824140027.GN2342@telsasoft.com>
References: <20220818232141.GQ26426@telsasoft.com>
 <CAApHDvo3Jibd=+pykPUAyfFYW_1_P-jJt2ehhNNWF8ie-QeFPw@mail.gmail.com>
 <20220819042816.GU26426@telsasoft.com>
 <CAApHDvrwLGBP+Yw9vriayyf=XR4uPWP5jr6cQhP9au_kaDUhbA@mail.gmail.com>
 <20220823011659.GF2342@telsasoft.com>
 <CAApHDvovE675zPvqiY6Y9pV1X53jSGjyhkf75yc8WGy7qAGdEA@mail.gmail.com>
 <20220823021412.GG2342@telsasoft.com>
 <CAApHDvoSgT93Da5D=ZWWhsE_exGyjK6sGWt5LpFJHsnaXVELzw@mail.gmail.com>
 <20220824023944.GM2342@telsasoft.com>
 <CAApHDvpL3ZpRhSaLf7wqSAbF42RtnJ_7puyiXR=2RdkoZ9RsjQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAApHDvpL3ZpRhSaLf7wqSAbF42RtnJ_7puyiXR=2RdkoZ9RsjQ@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220824140027.GN2342%40telsasoft.com>
Precedence: bulk

On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote:
> I was hoping we'd already caught all of the #1s in 421892a19, but I
> caught a few of those in some of your other patches. One you'd done
> another way and some you'd done the rescope but just put it in the
> wrong patch.  The others had not been done yet. I just pushed
> f959bf9a5 to fix those ones.

This fixed pg_get_statisticsobj_worker() but not pg_get_indexdef_worker() nor
pg_get_partkeydef_worker().

(Also, I'd mentioned that my fixes for those deliberately re-used the
outer-scope vars, which isn't what you did, and it's why I didn't include them
with the patch for inner-scope).

> I really think #2s should be done last. I'm not as comfortable with
> the renaming and we might want to discuss tactics on that. We could
> either opt to rename the shadowed or shadowing variable, or both.  If
> we rename the shadowing variable, then pending patches or forward
> patches could use the wrong variable.  If we rename the shadowed
> variable then it's not impossible that backpatching could go wrong
> where the new code intends to reference the outer variable using the
> newly named variable, but when that's backpatched it uses the variable
> with the same name in the inner scope.  Renaming both would make the
> problem more obvious.  I'm not sure which is best.  The answer may
> depend on how many lines the variable is in scope for. If it's just
> for a few lines then the hunk context would conflict and the committer
> would likely notice the issue when resolving the conflict.

Yes, the hope is to limit the change to variables that are only used a couple
times within a few lines.  It's also possible that these will break patches in
development, but that's normal for any change at all.

> I'll study #7 a bit more. My eyes glazed over a bit from doing all
> that analysis, so I might be mistaken about that being a bug.

I reported this last week.
https://www.postgresql.org/message-id/20220819211824.GX26426@telsasoft.com

-- 
Justin