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
