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

Replace NpgsqlTransaction.IsCompleted with NpgsqlConnection.IsTransactionInProgress #2618

Open
roji opened this issue Sep 11, 2019 · 11 comments · May be fixed by #2620

Comments

@roji
Copy link
Member

commented Sep 11, 2019

In #2416 we added transaction recycling for a given connector, since PostgreSQL only supports one transaction at any given time. This change is being reverted for 4.1 in #2617, because it introduced a breaking change around NpgsqlTransaction.IsCompleted (see #2607).

Transaction recycling remains in the dev branch for 5.0, but we need to decide exactly what we want:

  • Reintroduce NpgsqlConnector.IsTransactionInProgress instead, which more closely represents the actual state of things and wouldn't cause any confusion. This would simply expose NpgsqlConnector.TransactionStatus publicly.
  • Reintroduce IsCompleted on NpgsqlTransaction, and require users to dispose transactions before creating new ones. This would provide stronger and safer semantics for IsCompleted, but would be another breaking change (since transaction didn't need to be disposed until now).
  • Not reintroduce IsCompleted. It's an Npgsql-specific property, and it can be argued that it's the user's responsibility to do any tracking. @baronfel any comments here, any specific reason why you need this from Npgsql?
  • Remove transaction recycling for 5.0, which would be a shame for perf.

@roji roji added the enhancement label Sep 11, 2019

@roji roji added this to the 5.0 milestone Sep 11, 2019

@roji roji self-assigned this Sep 11, 2019

@baronfel

This comment has been minimized.

Copy link

commented Sep 11, 2019

I'm the wrong person to ask, I don't have any special knowledge about how marten tracks the underlying NpgsqlTransaction. I just happened to notice the usage as I was trialing 4.1.0!

@jeremydmiller @oskardudycz @mysticmind do you all have more context here for how Marten uses the transaction lifecycle?

@roji

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

Thanks @baronfel.

I think reintroducing something like NpgsqlConnector.IsTransactionInProgress would make sense in 5.0, Marten would have to to make the change to use that.

@oskardudycz

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@roji @baronfel in Marten we're using IsCompleted Transaction property to make sure that we're not Rollbacking or Disposing already Completed transaction. See more in https://github.com/JasperFx/marten/blob/master/src/Marten/Services/TransactionState.cs#L123.

It seems that for this case NpgsqlConnector.IsTransactionInProgress should be perfectly fine.

Btw. @roji do you have some place with written stuff that you're working on 5.0 version (especially potential breaking change would be great to know)? We'd like to prepare in advance to not have big migration issues as we had in 3=>4 migration.

p.s.
@baronfel thank you for giving heads up and great catch!
@roji thank you for reverting the change to keep compatibility 👍

@roji

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

@roji @baronfel in Marten we're using IsCompleted Transaction property to make sure that we're not Rollbacking or Disposing already Completed transaction. See more in https://github.com/JasperFx/marten/blob/master/src/Marten/Services/TransactionState.cs#L123.

It seems that for this case NpgsqlConnector.IsTransactionInProgress should be perfectly fine.

I think it does make sense to have NpgsqlConnector.IsTransactionInProgress, but is there any specific reason you need Npgsql to track the transaction status rather than just doing it yourself? Am wondering also because it's an Npgsql-specific feature, so you simply wouldn't have something like this on, say, SqlClient.

Btw. @roji do you have some place with written stuff that you're working on 5.0 version (especially potential breaking change would be great to know)? We'd like to prepare in advance to not have big migration issues as we had in 3=>4 migration.

The best way to follow this is to look at issues in the 5.0 milestone which have the "breaking change" label. Of course it's also advisable to occasionally run your tests against preview (or even nightly) versions of Npgsql, to make sure nothing slips in by accident.

@oskardudycz

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

I think it does make sense to have NpgsqlConnector.IsTransactionInProgress, but is there any specific reason you need Npgsql to track the transaction status rather than just doing it yourself? Am wondering also because it's an Npgsql-specific feature, so you simply wouldn't have something like this on, say, SqlClient.

As there might be multiple connections using same transaction then imho it's better to use the that information from the source of truth - so Npgsql. Probably we could manage that by ourselves, but if it's possible to keep that then it would be less costly for us. We'll need to add this code plus do some extensive testing if it's working the same way as yours implementation to not break anything in our users codebase.

The best way to follow this is to look at issues in the 5.0 milestone which have the "breaking change" label. Of course it's also advisable to occasionally run your tests against preview (or even nightly) versions of Npgsql, to make sure nothing slips in by accident.

Thanks, I'll start to look into that. Is there an ETA for v5.0?

roji added a commit that referenced this issue Sep 11, 2019
@roji roji referenced a pull request that will close this issue Sep 11, 2019
@roji

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

As there might be multiple connections using same transaction then imho it's better to use the that information from the source of truth - so Npgsql

Do you mean to say you are using distributed transactions (multiple physical connections, one transaction)? If so, I don't think that the current property (NpgsqlTransaction.IsCompleted) tracks that in any way (that's a good point for #2618). Could you please double-check on this point?

Probably we could manage that by ourselves

I'm mainly trying to understand how/why people need this, I really have no particular objection to exposing the information.

@oskardudycz

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

As there might be multiple connections using same transaction then imho it's better to use the that information from the source of truth - so Npgsql

Do you mean to say you are using distributed transactions (multiple physical connections, one transaction)? If so, I don't think that the current property (NpgsqlTransaction.IsCompleted) tracks that in any way (that's a good point for #2618). Could you please double-check on this point?

Probably we could manage that by ourselves

I'm mainly trying to understand how/why people need this, I really have no particular objection to exposing the information.

I probably wasn’t precise enough. I meant that there might be multiple NpgsqConnections sharing the same transaction. If eg. 2 of them fail then to only one Rollback should be made. Being able to get that from the Transaction would be imho easier and less error prone than manually manage that information.

@roji

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

I probably wasn’t precise enough. I meant that there might be multiple NpgsqConnections sharing the same transaction. If eg. 2 of them fail then to only one Rollback should be made. Being able to get that from the Transaction would be imho easier and less error prone than manually manage that information.

I still don't understand - what does it mean for two NpgsqlConnections to share the same transaction, without that being a distributed transaction? An NpgsqlTransaction instance is obtained by calling NpgsqlConnection.BeginTransaction; it represents the transaction started on that connection and is bound to it.

@oskardudycz

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

I still don't understand - what does it mean for two NpgsqlConnections to share the same transaction, without that being a distributed transaction? An NpgsqlTransaction instance is obtained by calling NpgsqlConnection.BeginTransaction; it represents the transaction started on that connection and is bound to it.

@roji you're right, by bad. I should have wrote multiple commands and not multiple connections.

@roji roji changed the title Decide on API contract around transaction recycling Replace NpgsqlTransaction.IsCompleted with NpgsqlConnection.IsTransactionInProgress Sep 15, 2019

@roji

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2019

@oskardudycz thanks for clarifying. Even when multiple commands are associated to the same connection, only one can be active at a given point.

In any case, for 5.0 we can just have NpgsqlConnection.IsTransactionInProgress.

@oskardudycz

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

@roji thank you for clarifications.

Having NpgsqlConnection.IsTransactionInProgress in 5.0 would be great. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.