{{ message }}
Repository navigation
Recycle NpgsqlTransaction #2416
Description
PostgreSQL doesn't support more than one transaction on a given connection at any given time. This means that each connector can have a single NpgsqlTransaction instance, and return that every time NpgsqlConnection.BeginTransaction() is invoked.
When we end up implementing BeginTransactionAsync() (see https://github.com/dotnet/corefx/issues/35012), we can also keep a single cached Task<NpgsqlTransaction> on the connector, since beginning a transaction doesn't actually do anything and therefore always returns synchronously.
Activity
roji commentedon Apr 4, 2019
Note that this is a slightly user-visible change, in that a user holding a reference to an NpgsqlTransaction will suddenly see the instance "revived" when
BeginTransaction()is called again (this is similar to how we recycle NpgsqlDataReader instances).More importantly, this would make the
IsCompleted()API largely meaningless and somewhat dangerous, as a completed NpgsqlTransaction object would become "uncompleted" when a new (and unrelated) transaction is started on the same connection. As a result, I think we should remove that API entirely.This would make it the user's responsibility to track the transaction state, and not call rollback on it twice (doing so would trigger an exception). If an intermediate layer is causing the rollback (as in #985), the user would need to be aware of that and refrain from calling rollback again. While this would make life a bit more difficult for users, it seems to make sense for the perf gain. Note that this is an Npgsql-specific API, not ADO.NET (introduced in #985).
@YohDeadfall @austindrenski what do you think? @chriswebb your opinion would also be appreciated here, as you originally made the request.
chriswebb commentedon Apr 5, 2019
Since the underlying behavior of transactions is changing, it does make sense to remove this property from the API as we can no longer assume the NpgsqlTransaction object belongs to the same transaction after we release it.
I think it is reasonable to require the framework to either remove all special handling in the framework side so the transaction isn't released and reused early or as you say keep track of this in some other way to prevent the double rollback problem. For that either a structure could be used, or just removing the NpgsqlTransaction object.
roji commentedon Apr 5, 2019
Thanks for your comment @chriswebb. I'm especially interested if there was some specific reason that let you to open #985, i.e. that tracking the transaction state yourself wasn't possible (just want to make sure I'm not overlooking some important scenario).
chriswebb commentedon Apr 9, 2019
Sorry for the delay. From what I recall about the issue, we did have to fix it on our side since we couldn't wait on the release of the patch and we prefer to use official builds for production, but we did migrate our solution to Npgsql's once it was released. The reasoning behind bringing it to Npgsql at all was due to it having the ultimate knowledge of its own state and the feeling that other people could benefit from making that Connnector == null call more transparent (since connector was private on the transaction object if i recall correctly). We obviously cannot rely on this property any more and will need to re-examine our old solution.
roji commentedon Apr 10, 2019
Thanks @chriswebb, I'll go ahead with the implementation then.
Recycle NpgsqlTransaction
Recycle NpgsqlTransaction