Re: [PATCH 1/6] bulk-checkin: remove ODB transaction nesting

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 11, 2025, 06:40 UTC
Message-ID: <aMJu4yoO5-Xp52oJ@pks.im>
In-Reply-To: <20250909191134.555689-2-jltobler@gmail.com>

On Tue, Sep 09, 2025 at 02:11:29PM -0500, Justin Tobler wrote:

Show 11 quoted lines

> 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?

Show 14 quoted lines

> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 124c4930676..0da5783090d 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -368,12 +367,11 @@ void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,
>  
>  struct odb_transaction *begin_odb_transaction(struct object_database *odb)
>  {
> -	if (!odb->transaction) {
> -		CALLOC_ARRAY(odb->transaction, 1);
> -		odb->transaction->odb = odb;
> -	}
> +	if (odb->transaction)
> +		BUG("ODB transaction already started");

Okay, good to see we have a `BUG()` here now.

Show 15 quoted lines

> @@ -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.

  - We probably should have an assert that if a pointer is given, then
    `transaction->odb->transaction == transaction`.

The conversions all look correct to me.

Patrick