On 25/09/11 08:40AM, 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?
This change does help with the subsequent patches which drop
flush_odb_transaction() by removing reasons for this explicit operation
to exist. Now it is guaranteed that calling end_odb_transaction() also
writes the changes. The change is largely just to clarify ownership
sematics for the transaction though. I'll try to clarify this in the
commit message in the next version.
> > @@ -389,14 +387,6 @@ void flush_odb_transaction(struct odb_transaction *transaction)
> >
> > void end_odb_transaction(struct odb_transaction *transaction)
> > {
> > - if (!transaction || transaction->nesting == 0)
> > - BUG("Unbalanced ODB transaction nesting");
> > -
> > - transaction->nesting -= 1;
> > -
> > - if (transaction->nesting)
> > - return;
> > -
> > flush_odb_transaction(transaction);
> > transaction->odb->transaction = NULL;
> > free(transaction);
>
> But I wonder whether we also want to make this function here a bit more
> robust:
>
> - I think we should handle `NULL` pointers here and convert them into
> a no-op. This allows us to skip the `if (transaction)` checks in
> many places.
This sounds reasonable. Will do.
> - We probably should have an assert that if a pointer is given, then
> `transaction->odb->transaction == transaction`.
Good idea. I'll also add this in the next version.
-Justin
