Re: What is lurking in the shadows?

On Fri, May 14, 2021 at 01:16:37PM +1200, David Rowley wrote:
> I'm inclined to think that since a bug has already been found due to a
> local variable shadowing a global one that it would be good to review
> these and then consider if it's worth doing any renaming. I think the
> process of looking at each warning individually will allow us to
> determine if; a) there are any bugs, or; b) if it's worth doing any
> renaming.

70116493 is another instance of that, from a not-so-far past..

> I'd say it might be worth aspiring to reduce the warnings from
> building with these flags. If we reduced these down then it might
> allow us to more easily identify cases where there are actual bugs.
> Maybe we can get to a point where we could enable either
> -Wshadow=compatible-local or -Wshadow=local. I doubt we could ever
> get to a stage where -Wshadow=global would work for us. There's also
> some quite old discussion in [1] that you might want to review.

Agreed, not before the 15 branch opens for business for cosmetic
changes. compatible-local did not sound that much interesting to me
on first sight, but the report of Peter tells the contrary: most of
the conflicts come from local problems. I am not sure that you could
enable that safely though as PG_TRY() would complain on that, for
example in ProcessUtilitySlow().

> We also need to take into account that renaming variables here can
> increase the overhead of backpatching fixes. The process of fixing
> those up to make the patch apply to the back branch does increase the
> chances that bugs could make their way into the back branches.
> However, it's probably more likely to end up as a bug if the patch was
> written for the back branch then there's a bigger opportunity for the
> patch author to pick the wrong variable name when converting the patch
> to work with master. In the reverse case, that does not seem as likely
> due to both variables having the same name.

That may be tricky, even if global or local variables are changed,
but I'd like to think that there is room for improvement. Looking at
the report, the global conflicts involve:
- synchronous_commit
- ssl_key_file
- wal_segment_size
- DataDir, with the control data business.

These seem changeable without much holes with potential back-patches.
--
Michael