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.gx26...@telsasoft.com -- Justin
