Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recycle NpgsqlTransaction #2416

Closed
roji opened this issue Apr 3, 2019 · 5 comments

Comments

Projects
None yet
2 participants
@roji
Copy link
Member

commented Apr 3, 2019

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 dotnet/corefx#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.

@roji roji added the performance label Apr 3, 2019

@roji roji added this to the 4.1 milestone Apr 3, 2019

@roji roji self-assigned this Apr 4, 2019

@roji roji added the breakage label Apr 4, 2019

@roji

This comment has been minimized.

Copy link
Member Author

commented 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

This comment has been minimized.

Copy link
Contributor

commented 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

This comment has been minimized.

Copy link
Member Author

commented 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

This comment has been minimized.

Copy link
Contributor

commented 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

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

Thanks @chriswebb, I'll go ahead with the implementation then.

roji added a commit to roji/Npgsql that referenced this issue Apr 10, 2019

Recycle NpgsqlTransaction
Since PostgreSQL only supports on transaction on a given connection,
we reycle the NpgsqlTransaction object.

Closes npgsql#2416

roji added a commit to roji/Npgsql that referenced this issue Apr 10, 2019

Recycle NpgsqlTransaction
Since PostgreSQL only supports on transaction on a given connection,
we reycle the NpgsqlTransaction object.

Closes npgsql#2416

roji added a commit to roji/Npgsql that referenced this issue May 15, 2019

Recycle NpgsqlTransaction
Since PostgreSQL only supports on transaction on a given connection,
we reycle the NpgsqlTransaction object.

Closes npgsql#2416

@roji roji closed this in #2427 May 15, 2019

roji added a commit that referenced this issue May 15, 2019

Recycle NpgsqlTransaction
Since PostgreSQL only supports on transaction on a given connection,
we reycle the NpgsqlTransaction object.

Closes #2416
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.