Em sáb., 13 de abr. de 2024 às 04:16, Heikki Linnakangas <hlinn...@iki.fi> escreveu:
> On 11/04/2024 16:37, Ranier Vilela wrote: > > Em qui., 11 de abr. de 2024 às 09:54, Heikki Linnakangas > > <hlinn...@iki.fi <mailto:hlinn...@iki.fi>> escreveu: > > > > On 11/04/2024 15:03, Ranier Vilela wrote: > > > Em qua., 10 de abr. de 2024 às 18:28, Heikki Linnakangas > > > <hlinn...@iki.fi <mailto:hlinn...@iki.fi> <mailto:hlinn...@iki.fi > > <mailto:hlinn...@iki.fi>>> escreveu: > > > > > > On 10/04/2024 21:07, Ranier Vilela wrote: > > > > Hi, > > > > > > > > Per Coverity. > > > > > > > > The function ReorderBufferTXNByXid, > > > > can return NULL when the parameter *create* is false. > > > > > > > > In the functions ReorderBufferSetBaseSnapshot > > > > and ReorderBufferXidHasBaseSnapshot, > > > > the second call to ReorderBufferTXNByXid, > > > > pass false to *create* argument. > > > > > > > > In the function ReorderBufferSetBaseSnapshot, > > > > fixed passing true as argument to always return > > > > a valid ReorderBufferTXN pointer. > > > > > > > > In the function ReorderBufferXidHasBaseSnapshot, > > > > fixed by checking if the pointer is NULL. > > > > > > If it's a "known subxid", the top-level XID should already > > have its > > > ReorderBufferTXN entry, so ReorderBufferTXN() should never > > return NULL. > > > > > > There are several conditions to not return NULL, > > > I think trusting never is insecure. > > > > Well, you could make it an elog(ERROR, ..) instead. But the point is > > that it should not happen, and if it does for some reason, that's > very > > suprpising and there is a bug somewhere. In that case, we should > *not* > > just blindly create it and proceed as if everything was OK. > > > > Thanks for the clarification. > > I will then suggest improving robustness, > > but avoiding hiding any possible errors that may occur. > > I don't much like adding extra runtime checks for "can't happen" > scenarios either. Assertions would be more clear, but in this case the > code would just segfault trying to dereference the NULL pointer, which > is fine for a "can't happen" scenario. > 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. > Looking closer, when we identify an XID as a subtransaction, we: > - assign toplevel_xid, > - set RBTXN_IS_SUBXACT, and > - assign toptxn pointer. > > ISTM the 'toplevel_xid' and RBTXN_IS_SUBXACT are redundant. > 'toplevel_xid' is only used in those two calls that do > ReorderBufferTXNByXid(rb, txn->toplevel_xid,...), and you could replace > those by following the 'toptxn' pointer directly. And RBTXN_IS_SUBXACT > is redundant with (toptxn != NULL). So here's a patch to remove > 'toplevel_xid' and RBTXN_IS_SUBXACT altogether. > + if (rbtxn_is_subtxn(txn)) + txn = rbtxn_get_toptxn(txn); 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. best regards, Ranier Vilela
v1-0001-Remove-redundant-field-and-flag-from-ReorderBufferTX.patch
Description: Binary data
