From: Justin Pryzby <pryzby@telsasoft.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: pgsql-hackers@postgresql.org, Tomas Vondra <tomas.vondra@postgresql.org>
Cc: Peter Smith <smithpb2250@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Subject: Re: shadow variables - pg15 edition
Date: Wed, 24 Aug 2022 09:00:27 -0500
Message-ID: <20220824140027.GN2342@telsasoft.com> (raw)
In-Reply-To: <CAApHDvpL3ZpRhSaLf7wqSAbF42RtnJ_7puyiXR=2RdkoZ9RsjQ@mail.gmail.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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, dgrowleyml@gmail.com, tomas.vondra@postgresql.org, smithpb2250@gmail.com, alvherre@alvh.no-ip.org
Subject: Re: shadow variables - pg15 edition
In-Reply-To: <20220824140027.GN2342@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox