On Sat, Apr 13, 2024 at 10:40:35AM -0300, Ranier Vilela wrote:
> This sounds a little confusing to me.
> Is the project policy to *tolerate* dereferencing NULL pointers?
> If this is the case, no problem, using Assert would serve the purpose of
> protecting against these errors well.
In most cases, it does not matter to have an assertion for a code path
that's going to crash a few lines later. The result is the same: the
code will crash and inform about the failure.
> rbtxn_get_toptxn already calls rbtxn_is_subtx,
> which adds a little unnecessary overhead.
> I made a v1 of your patch, modifying this, please ignore it if you don't
> agree.
c55040ccd017 has been added in v14~, so this is material for 18~ at
this stage. If you want to move on with these changes, I'd suggest to
add a CF entry.
FWIW, I think that I agree with the point made upthread by Heikki
about the fact that these extra ReorderBufferTXNByXid() are redundant.
In these two code paths, the ReorderBufferTXN entry of top transaction
ID should exist, so removing toplevel_xid makes sense.
It may be worth exploring if more simplifications around
ReorderBufferTXNByXid() are possible, actually.
--
Michael