Reset connection when closing within TransactionScope#6424
Reset connection when closing within TransactionScope#6424roji wants to merge 1 commit intonpgsql:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request attempts to fix issue #3735 by resetting pooled connections when they are closed within a TransactionScope. The goal is to ensure that when a connection is closed and then reopened within the same TransactionScope, temporary tables, sequences, and other session state are reset, even though the same physical connection is reused.
Changes:
- Added a new
ResetWithinEnlistedTransaction()method to reset connector state when closing within an enlisted transaction - Updated
NpgsqlConnection.CloseAsync()to call the new reset method before adding the connector to the pending enlisted list - Added documentation comments explaining the new reset behavior
- Added a test case to verify that temp tables are properly discarded when reusing connections within a TransactionScope
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/Npgsql.Tests/SystemTransactionTests.cs | Added test to verify temp tables are reset when reusing connections in TransactionScope |
| src/Npgsql/NpgsqlConnection.cs | Updated connection close logic to reset connector before adding to enlisted pending list |
| src/Npgsql/Internal/NpgsqlConnector.cs | Added ResetWithinEnlistedTransaction() method and updated GenerateResetMessage() documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
roji
force-pushed
the
TransactionScopeReset
branch
from
689ce47 to
93dbcbe
Compare
|
|
||
| /// <summary> | ||
| /// Called when a pooled connection with an enlisted System.Transactions transaction is closed. | ||
| /// Since we're inside a transaction block, we cannot send DISCARD ALL, DISCARD TEMP or DISCARD SEQUENCES; |
There was a problem hiding this comment.
This comment says DISCARD TEMP and DISCRD SEQUENCES cannot be used, but I can see they are being added inside GenerateResetMessage() which is called below.
This seems to contradict the comment, or am I missing something?
There was a problem hiding this comment.
The comment is wrong (AI-generated), I'll fix - DISCARD TEMP and DISCARD SEQUENCES seem to work fine within transactions (but DISCARD ALL is indeed disallowed).
There was a problem hiding this comment.
I imagine that's to make sure that in case they add a new command in the future which can't run in transactions, that way they'll not break anyone.
| } | ||
|
|
||
| [MemberNotNull(nameof(_resetWithoutDeallocateMessage))] | ||
| void GenerateResetMessage() |
There was a problem hiding this comment.
@roji do you think it might make sense to move _resetWithoutDeallocateMessage to the data source/database info, rather than cache per each connector? Not in this pr of course.
| internal void ResetWithinEnlistedTransaction() | ||
| { | ||
| // Our buffer may contain unsent prepended messages, so clear it out. | ||
| WriteBuffer.Clear(); |
There was a problem hiding this comment.
Should we start user action here to prevent messing up concurrent keepalive?
3 participants
Fixes #3735