From: Justin Tobler Date: Tue, 16 Sep 2025 02:55:15 GMT Subject: Re: [PATCH 1/6] bulk-checkin: remove ODB transaction nesting Message-ID: In-Reply-To: On 25/09/15 07:36PM, Taylor Blau wrote: > On Thu, Sep 11, 2025 at 08:40:35AM +0200, Patrick Steinhardt wrote: > > On Tue, Sep 09, 2025 at 02:11:29PM -0500, Justin Tobler wrote: > > > ODB transactions support being nested. Only the outermost > > > {begin,end}_odb_transaction() start and finish a transaction. This is > > > done so that certain object write codepaths that occur internally can be > > > optimized via ODB transactions without having to worry if a transaction > > > has already been started or not. This can make the interface a bit > > > awkward to use, as calling {begin,end}_odb_transaction() does not > > > guarantee that a transaction is actually started or ended. > > > > > > Instead, be more explicit and require callers who use ODB transactions > > > internally to ensure there is not already a pending transaction before > > > beginning or ending a transaction. > > > > I think one bit missing in the commit message is to explain what this > > buys us. Does it for example enable subsequent changes? Or is this > > really only done to have clean ownership semantics for the transaction? > > In addition, it would be useful to hear from the commit message *why* > this is safe to do. Justin's message suggests that nested transactions > are noops, so doing something like: > > begin_odb_transaction(); > begin_odb_transaction(); > write_object(); > end_odb_transaction(); <- object not yet added to the main ODB > end_odb_transaction(); <- now it is > > only results in the object being added to the main ODB when the final > end_odb_transaction() is called. Yes, well said. {begin,end}_odbtransaction() operations on inner transactions are effectively a noop. They simple manage an internal counter to know when a new transaction should be started/finished. > Instead it looks like this patch pushes us towards having callers check > whether or not there is a transaction in progress before starting a new > one. So it seems like this is safe to do only for callers that check > whether or not there is an ongoing transaction before beginning a new > one. Yes, this patch removes the logic that manages the internal nested transaction counter in favor of requiring callers to check if a transaction has already been started or not. > (I think this is what the second paragraph of the quoted part is trying > to say, but I think it may be clearer to say "To preserve the same > semantics, callers MUST ensure there is not [...]"). Yes, you are correct. Apologies for the poor wording. > That's more work for callers, and at first blush feels a little more > error-prone. > > Specifically, if some new piece of code is written that does not first > check whether there is an ongoing transaction, it could result in a > BUG() either at the time it is written, or worse, later on when that > function is called in the context of an outer transaction. > > So I am not sure whether this patch is making things simpler or safer. > Certainly the bulk-checkin API is a little simpler, since we no longer > have to keep track of the nesting level within an odb_transaction. But I > think it pushes more burden onto the callers in a way that I worry could > create the potential for BUG()s later on. I've revisted this patch and I agree there is probably a better/safer way to accoplish this. The ultimate goal of this patch is make it so invoking end_odb_transaction() on a transaction guarantees it is flushed. With this guarantee we would no longer need flush_odb_transaction() and the transaction interface eventually can be simplified to just {begin,end}_odb_transaction(). This also avoids a potential class of errors where a caller _thinks_ they have committed a transaction and that the objects should be visible, but they actually are not because it was a nested transaction. The nice thing about the current implementation though is that nested transactions are automatically treated as noops and the caller doesn't have to check if there is already a pending transaction. This is safer and less error-prone. Thinking about this some more though, we should be able to continue to have {begin,end}_odb_transaction() function as noops when there is already a pending transaction and also be able to drop the internal transaction nesting mechanism at the same time. To do this, instead of erroring out in begin_odb_transaction() when there is already a transaction, we can simply return NULL. If a caller wants to know if a new transaction was actually started, they can just check the return value afterwords. This removes the burden from the caller to explicitly check if there is a pending transaction beforehand. Furthermore, since an ODB only allows a single transaction at a time, it probably makes sense for operations at the ODB layer to guard against this anyway. This change would pair nicely with the change made to end_odb_transaction() in version 2 of this series. In this version, when end_odb_transaction() is provided a NULL transaction, it now functions as a noop as well. If a transaction _is_ provided though, it is guaranteed to be flushed with addresses the main goal on this patch. > I think that takes us back to Patrick's question: what do we gain by > simplifying the internals of the bulk-checkin API, and how does (or > doesn't) that justify the added burden on callers? Looking at the newer > version of this patch in [1], I see that you addressed what we gain, but > I am still curious about how we justify the added cost. Thanks Taylor for the thoughtful feedback. Much appreciated. :) I'll plan on sending a followup version tomorrow that hopefully addresses most of these concerns. -Justin