From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 11 Sep 2025 06:40:35 GMT
Subject: Re: [PATCH 1/6] bulk-checkin: remove ODB transaction nesting
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:
> 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?

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

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