[brandur]

River currently opens subtransactions when already in a transaction for
most behavior. This isn't something that's useful in the majority of
situations, but may be very occasionally in cases where a piece of a
transaction fails, but the caller still wants to commit the whole
transaction anyway.

Unfortunately, subtransactions have a real performance impact in
Postgres [1], so it's not free to have them. One of our users confirms
to have observed this IRL.

This patch modifies River's transaction approaches so as to only open a
new one unless not already in one. So Client.Insert still opens a
transaction, but Client.InsertTx won't open a new subtransaction on
top of the tx that was already passed into it.

Though not an API change, this is a behavioral change. I've been having
Codex try to give me the worst case scenario of what might break were we
to change this. So far it hasn't come up with anything convincing (in
every scenario the change improves correctness of the original state),
but it's still worth thinking about whether there might be other real
use cases where people would be broken by this.

For now I've added an env flag RIVER_USE_LEGACY_SUBTRANSACTIONS that
can be used to revert back to the previous behavior. The idea here is
that it'd give anyone broken by this a potential escape hatch, but it
would still let us roll forward with no subtransactions as the preferred
default. The env var would be removed in a future version.

[1] https://planetscale.com/blog/the-dangers-of-postgres-subtransactions

[brandur]

Here's an example from Codex of a potential problem that could result from the change:

---

A concrete example is a Pro user who saves an order but treats an accompanying analytics job as optional:

if err := insertOrder(ctx, tx, order); err != nil {
    return err
}

if _, err := client.InsertTx(ctx, tx, AnalyticsArgs{OrderID: order.ID}, nil); err != nil {
    logger.Error("Couldn't enqueue analytics", "error", err)
    // Intentionally continue: analytics shouldn't block checkout.
}

return tx.Commit(ctx)

Suppose the job insertion hits a PostgreSQL lock_timeout because another transaction holds a conflicting lock.

• Before: Pro rolls back its internal savepoint. The order remains in the outer transaction, and the commit succeeds.
• After: The SQL error leaves the entire transaction aborted. The commit cannot save the order, so an optional analytics failure now causes checkout to fail. PostgreSQL requires rollback to a savepoint—or a full rollback—to recover from that state. PostgreSQL documentation.

[brandur]

Another one:

---

A stronger example is a bulk import that intentionally accepts valid records and reports individual failures.

Suppose a Pro application imports 1,000 documents, calling InsertTx once per document inside a transaction. Its intended result is “999 documents queued; one rejected.”

One document contains a NUL character in its text. Go successfully encodes that as \u0000, but PostgreSQL rejects it in jsonb, so the insertion produces a database error. PostgreSQL documentation.

• Before: Pro rolls back that insertion’s savepoint. The application records the rejected document, continues importing, and commits the other 999 jobs.
• After: That insertion aborts the transaction. Subsequent insertions fail, and none of the jobs can be committed. Retrying the same input repeats the failure.

Here, retaining successful work is explicitly the desired behavior. There’s no order/email consistency concern: each document is independent, and one malformed input now prevents every valid document from being processed.

The application would need its own per-document savepoints, or separate transactions, to restore that behavior.

That’s a legitimate compatibility concern, although it’s still a hypothetical usage pattern, not an observed customer dependency. I’d still favor caller-owned savepoints for this case: the application knows it wants partial success, whereas most callers want the entire transaction rolled back when an insertion fails.

[brandur]

I don't find either of these examples all that convincing in that it's definitely describing a real thing, but it'd be unusual to still want to commit the transaction in case of either failure, and in case you did want to commit it, an explicit savepoint would definitely be preferable to River deciding for you.

Want to be careful on this one because although the obvious answer would be to put this behind a configuration flag, it seems to me that the overwhelmingly more "correct" behavior would be the opposite of what River does right now. So it'd be nice to work our way to a point where we could get back to that.

[bgentry]

I was a bit confused until I realized this was mainly a sqlite-specific PR (its title doesn't say so, nor does the changelog entry). Aside from making that clearer, I think this is fine to merge. I assume you've also verified the other drivers don't need such changes?

[brandur]

Ah, it actually does apply to all databases. The key piece is in db_util.go here:

// executor preserves transaction identity when a method is invoked through an
// embedded Executor, so transaction helpers can reuse it without a savepoint.
func (e *Executor) executor() riverdriver.Executor {
	if e.execTx != nil {
		return e.execTx
	}
	return e
}

SQLite's driver needed a little extra code too, but this change fixes all DBs.