NpgsqlCommand.Dispose() should execute "DEALLOCATE <name>" for a prepared command #158

Closed
udoliess opened this Issue Feb 6, 2014 · 3 comments

Projects

None yet

3 participants

@udoliess
Contributor
udoliess commented Feb 6, 2014

NpgsqlCommand supports Prepare() and I think it should execute "DEALLOCATE " in its Dispose() method.
Currently there is no NpgsqlCommand.Dispose() method at all.
Preparing of many commands will pump up the server process memory and closing the connection afterwards takes very long!
Below is an example program...
(It was tested with Win7/64bit, PostgreSQL 9.2.6, .NET 4.0, VS2012, Npgsql/master current commit 6407cc3.)
In my case disposing of connection takes 14s. But with DEALLOCATE work around it takes only 2ms.

using Npgsql;
using System;
using System.Diagnostics;
class Program
{
    static void Main(string[] args)
    {
        var sw = Stopwatch.StartNew();
        using (var con = new NpgsqlConnection("Server=localhost;User ID=npgsql_tests;Password=npgsql_tests;Database=npgsql_tests;syncnotification=false"))
        {
            con.Open();
            for (int i = 0; i < 30000; i++)
            {
                using (var cmd = con.CreateCommand())
                {
                    cmd.CommandText = "SELECT 0";
                    cmd.Prepare();
                    cmd.ExecuteScalar();
                }
                // activating this DEALLOCATE command works around missing deallocation of prepared command
                /*
                using (var cmd = con.CreateCommand())
                {
                    cmd.CommandText = "DEALLOCATE ALL";
                    cmd.ExecuteNonQuery();
                }
                */
            }
            Console.WriteLine(sw.Elapsed);
            sw.Restart();
        }
        Console.WriteLine(sw.Elapsed);
        Console.ReadLine();
    }
}
@franciscojunior
Member

I think you have a good point here.

I think this issue didn't appear before because we are always asking users to use the pattern of open the connection, use it and then close it. This way, it isn't normal to accumulate too much prepared statements.

Another point is that we discouraged the use of prepared statements because they had poor performance. Now that Glen added a lot of optimizations, I think more and more prepared statements will be used and this issue may start to be noticed.

Thank you very much, Udo, for bringing our attention to this. We will work on it for the 2.2 release. I'll add a milestone to indicate that.

@franciscojunior franciscojunior added this to the 2.2 milestone Feb 6, 2014
@roji
Member
roji commented Jul 27, 2014

Guys, this is a pretty important item, I'll take a look at it, hopefully it will make it into 2.2.

@roji roji self-assigned this Jul 27, 2014
@roji roji pushed a commit to roji/Npgsql that referenced this issue Jul 27, 2014
Shay Rojansky Unit test for #158 (prepared stmt leak) 83ebbce
@roji
Member
roji commented Jul 27, 2014

Implementation note to self: we can't do any sort of database interaction from with Dispose() (i.e. DEALLOCATE) since when executed from the finalizer (can be whenver) this causes contention on the database connection with whatever query is actually being executed.

Can't lock either, since any sort of blocking within a finalizer is a big no-no.

So need to push the DEALLOCATE to a queue to be processed at the earliest convenience. It would be nice to be able to check, from Dispose(), whether the connection is in use (TryLock) and have a fast-path, but there's no such lock currently in Npgsql. This mechanism would also be relevant for #270.

@roji roji added a commit to roji/Npgsql that referenced this issue Jul 27, 2014
@roji roji Implemented Dispose() for NpgsqlConnection
Disposes of the active reader if one exists, and
if the command is prepared, deallocates the backend
prepared statement.

Fixes #158
a8643f1
@roji roji added a commit to roji/Npgsql that referenced this issue Jul 27, 2014
@roji roji Unit test for #158 (prepared stmt leak) b7c4b8a
@roji roji added a commit to roji/Npgsql that referenced this issue Jul 27, 2014
@roji roji Implemented Dispose() for NpgsqlConnection
Disposes of the active reader if one exists, and
if the command is prepared, deallocates the backend
prepared statement.

Fixes #158
1040d1c
@roji roji added a commit that closed this issue Jul 28, 2014
@roji roji + Shay Rojansky Deallocate prepared statement on Dispose()
Note that this only happens in explicit Dispose()/
using, and not when Dispose() is called from a
finalizer (implementation for that is complicated).

Regardless, all prepared statements are automatically
deallocated when a connection is returned to the
connection pool (DISCARD ALL).

Fixes #158
61ddb96
@roji roji closed this in 61ddb96 Jul 28, 2014
@roji roji added a commit that referenced this issue Jul 28, 2014
@roji roji Unit test for #158 (prepared stmt leak) c429ad1
@roji roji added a commit that referenced this issue Jul 28, 2014
@roji roji Deallocate prepared statement on Dispose()
Note that this only happens in explicit Dispose()/
using, and not when Dispose() is called from a
finalizer (implementation for that is complicated).

Regardless, all prepared statements are automatically
deallocated when a connection is returned to the
connection pool (DISCARD ALL).

Fixes #158
46e2fe7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment