Connections not reset properly if connection is opened/closed within a transactionscope.

[magnusanderholm]

Steps to reproduce

Before running the test You will have to create the db test_db and role test_db_role manually.

[TestCase]
public void ConnectionNotReset()
{
    var tO = new TransactionOptions();
    tO.IsolationLevel = IsolationLevel.Serializable;
    var connectionString = "Server=127.0.0.1;Port=5432;Userid=postgres;Password=postgres;Pooling=true;MinPoolSize=0;MaxPoolSize=20;CommandTimeout=180;Database=test_db;Max Auto Prepare=100;";
    for (var i = 0; i < 2; ++i)
    {
        using (var txn = new TransactionScope(TransactionScopeOption.Required, tO, TransactionScopeAsyncFlowOption.Enabled))
        {
            using (var con = new NpgsqlConnection(connectionString))
            {
                con.Open();
                using (var cmd = con.CreateCommand())
                {
                    cmd.CommandText = @"
                        DO $$ 
                        BEGIN
                        IF (current_user <> 'postgres') THEN
                            RAISE EXCEPTION 'reset_violation. User %', current_user USING HINT = 'connection not reset';
                        END IF;
                        SET ROLE test_db_role;
                        END
                        $$";
                    cmd.ExecuteNonQuery();
                }
                con.Close();
            }
            txn.Complete();
        };
    }
}

The issue

I'm not sure if it really is a bug or not but here it goes :).
It looks like the connection from the connection pool is not reset correctly if NpgsqlConnection.Open/Close is called within a TransactionScope. In the test code above the first execution of the query works fine. The seconds time we (probably) get the same connection from the connection pool back but it is not being reset properly (no DISCARD ALL). On the second call the SET ROLE command is still in effect (current_user = 'test_db_role') which I assume can have serious consequences in some cases (for example sql cmd executing as a role with higher privileges than intended?). This is of course not limited to just the SET ROLE command. Temp tables etc are also not discarded.
I kind of understand why since its not allowed to issue a DISCARD ALL within a transaction. However from what I've seen in the wild its quite common to create a transactionscope and then open/close connections inside it to do DB operations.

Npgsql.PostgresException
  HResult=0x80004005
  Message=P0001: reset_violation. User bim_dev_psad_local_read
  Source=Npgsql
  StackTrace:
   at Npgsql.Internal.NpgsqlConnector.<<ReadMessage>g__ReadMessageLong|211_0>d.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Npgsql.NpgsqlDataReader.<NextResult>d__47.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at Npgsql.NpgsqlDataReader.NextResult()
   at Npgsql.NpgsqlCommand.<ExecuteReader>d__116.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Npgsql.NpgsqlCommand.<ExecuteReader>d__116.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Npgsql.NpgsqlCommand.<ExecuteNonQuery>d__104.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at Npgsql.NpgsqlCommand.ExecuteNonQuery()
   at PS.BIM.Db.Tests.DbTransactionUnitTests.ConnectionNotReset() in F:\git\ps-bim\bim\PS.BIM.Tests\Db\DbTransactionUnitTests.cs:line 69

  This exception was originally thrown at this call stack:
    Npgsql.Internal.NpgsqlConnector.ReadMessage.__ReadMessageLong|211_0(Npgsql.Internal.NpgsqlConnector, bool, Npgsql.Internal.DataRowLoadingMode, bool, bool)
    System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
    System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(System.Threading.Tasks.Task)
    System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(System.Threading.Tasks.Task)
    Npgsql.NpgsqlDataReader.NextResult(bool, bool, System.Threading.CancellationToken)
    System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
    System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(System.Threading.Tasks.Task)
    System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(System.Threading.Tasks.Task)
    System.Runtime.CompilerServices.TaskAwaiter<TResult>.GetResult()
    Npgsql.NpgsqlDataReader.NextResult()
    ...
    [Call Stack Truncated]

Further technical details

Npgsql version: 6.0.4
PostgreSQL version: 14
Operating system: Windows Server 2012 R2
Runtime: NET 6.

[roji]

As you wrote, it isn't possible to execute DISCARD ALL within a transaction (PG returns an error).

To give more context, opening multiple connections within the same TransactionScope is typically the trigger to start a distributed transaction (currently unsupported in .NET Core/5+, see dotnet/runtime#715). However, there's a special optimization where connections are done to the same database, and not at the same time - as in your code. In that case, since the connection being closed is still enlisted in an uncommitted TransactionScope, it is returned to a special place in the pool, and if another connection is opened to the same database within that TransactionScope, that connection is handed out again. This prevents escalation to a distributed transaction, which is a much more costly mechanism.

Technically, we could maybe issue the various reset commands that DISCARD ALL performs (see docs). However, the current transaction may be relying on e.g. temporary tables, and resetting those could interfere with the proper functioning of the transaction.

So I'm simply not sure what else we could do here - but am open to suggestions.

[Mistagrooves]

Could the .Reset() be called when returning the connection back after the Transaction has been committed in the VolatileResourceManager.cs:Dispose()? My initial test seemed to pass for the simple non-distributed test case.

Seems like a lot of work to do in Dispose() function though.

            if (_connector.TryRemovePendingEnlistedConnector(_transaction))
            {
                _connector.Reset(false).GetAwaiter().GetResult();
                _connector.Return();
            }

[magnusanderholm]

I'm no export :) so the the only input I can give is that the code should probably catch, log and eat any exceptions that could arise due to the Reset() command being issued. Dispose() is not supposed to throw based on my understanding, can potentially hide the real exception.

However if this can't be solved then perhaps the docs should be updated to reflect the issue? (https://www.npgsql.org/doc/basic-usage.html#systemtransactions-and-distributed-transactions).
Using transaction scopes like in the sample below does work. However it does feel like it kind of goes against how a transctionscope is supposed to be used though no?

var tO = new TransactionOptions();
tO.IsolationLevel = IsolationLevel.Serializable;
var connectionString = "Server=127.0.0.1;Port=5432;Userid=postgres;Password=postgres;Pooling=true;MinPoolSize=0;MaxPoolSize=20;CommandTimeout=180;Database=test_db;Max Auto Prepare=100;";
for (var i = 0; i < 2; ++i)
{
    var txn = new TransactionScope(TransactionScopeOption.Required, tO, TransactionScopeAsyncFlowOption.Enabled);
    using (var con = new NpgsqlConnection(connectionString))
    using(txn) // Make sure transaction scope is disposed before connection so connection can be reset properly
    {
        con.Open();
        using (var cmd = con.CreateCommand())
        {
            cmd.CommandText = @"
                DO $$ 
                BEGIN
                IF (current_user <> 'postgres') THEN
                    RAISE EXCEPTION 'reset_violation. User %', current_user USING HINT = 'connection not reset';
                END IF;
                SET ROLE test_db_role;
                END
                $$";
            cmd.ExecuteNonQuery();
        }
        txn.Complete();
    }                
};