On Thu, Jan 27, 2022 at 7:33 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
> Why not, at least, just add "Assert(result.page != NULL);" after the
> "Assert(!result.found);" in FreePageManagerPutInternal()?
> The following code block in FreePageBtreeSearch() - which lacks those
> initializations - should never be invoked in this case, and the added
> Assert will make this more obvious.
>
> if (btp == NULL)
> {
> result->page = NULL;
> result->found = false;
> return;
> }
This patch is now in its fourth CommitFest, which is really a pretty
high number for a patch that has no demonstrated benefit. I'm marking
it rejected.
If you or someone else wants to submit a carefully-considered patch to
add meaningful assertions to this file in places where it would
clarify the intent of the code, please feel free to do that. But the
patch as presented doesn't do that. It simply initializes some
structure members to arbitrary values that probably won't produce
sensible results instead of leaving them uninitialized which probably
won't lead to sensible results either. It's been argued that this
could prevent future bugs, but I find that dubious. This code isn't
likely to be heavily modified in the future - it's a low-level
subsystem that has thus far shown no evidence of needing major
surgery. If surgery does happen in the future, I would argue that this
change could easily *mask* bugs, because if somebody tries to apply
valgrind to this code the useless initializations will just cover up
what valgrind would otherwise detect as an access to uninitialized
memory.
Please let's move on. There are almost 300 patches in this CommitFest
and many of them add nifty features or fix demonstrable bugs. This
does neither.
--
Robert Haas
EDB: http://www.enterprisedb.com