jonwagner / Insight.Database

Fast, lightweight .NET micro-ORM
Other
861 stars 145 forks source link

Cache TVP probe results #354

Closed nzbart closed 6 years ago

nzbart commented 6 years ago

We use table-valued parameters with dynamic SQL, and I've noticed that, for every call to the database, we're incurring an additional three calls to locate the table valued parameter because it follows the naming convention Type + Table.

Since these structures will very rarely change or be dropped, is it possible to cache the results of these probes so that only the first query actually needs to hit the database?

I see the following three SQL queries executed, with a round-trip to the database for each one:

exec sp_executesql N'SELECT COUNT(*) FROM sys.table_types WHERE NAME = @name',N'@name nvarchar(6)',@name=N'values'
exec sp_executesql N'SELECT COUNT(*) FROM sys.table_types WHERE NAME = @name',N'@name nvarchar(9)',@name=N'SimpleInt'
exec sp_executesql N'SELECT COUNT(*) FROM sys.table_types WHERE NAME = @name',N'@name nvarchar(14)',@name=N'SimpleIntTable'

The values would be cached either in SqlInsightDbProvider.SetupTableValuedParameter or SqlInsightDbProvider.GetTableParameterTypeName.

jonwagner commented 6 years ago

The results of that should automatically be cached in the stored procedure parameter cache. It shouldn't have a large impact unless you have a lot of stored procedures. Or am I missing something?

nzbart commented 6 years ago

We almost exclusively use dynamic SQL and occasionaly use stored procedures.

I've written a test that runs the same query twice to demonstrate that the existence of the TVP is checked on every query.

using System;
using System.Data;
using System.Data.SqlClient;
using System.Linq;
using FluentAssertions;
using Insight.Database;
using Insight.Database.Providers;
using NUnit.Framework;

public class Tests
{
    [Test]
    public void Given_a_query_that_has_been_run_previously_When_run_again_Then_the_table_valued_parameter_metadata_should_not_be_queried_more_than_once()
    {
        InsightDbProvider.RegisterProvider(new SqlInsightDbProvider());

        using (var db = new SqlConnection("server=localhost;trusted_connection=true;app=InsightTest"))
        {
            db.Open();

            CreateNewDatabase(db);

            try
            {
                var sql = "select count(*) from @values";
                var values = Enumerable.Range(1, 4).Select(v => new SimpleInt(v)).ToArray();

                void RunQuery() => db.SingleSql<int>(sql, new { values }).Should().Be(values.Length);

                //Run the query twice
                RunQuery();
                RunQuery();
            }
            finally
            {
                DropDatabase(db);
            }
        }
    }

    static void CreateNewDatabase(IDbConnection db)
    {
        var dbName = "Test_" + Guid.NewGuid().ToString("N");
        db.ExecuteSql($"create database {dbName}");
        db.ExecuteSql($"use {dbName}; create type dbo.SimpleIntTable as table (Value int primary key)");
    }

    void DropDatabase(IDbConnection db)
    {
        db.ExecuteSql($"use master; drop database {db.Database}");
    }
}

public class SimpleInt
{
    public int Value { get; }

    public SimpleInt(int value)
    {
        Value = value;
    }
}

Unfortunately, I couldn't figure out how to capture the TVP probes in my test because the SqlCommand is used directly, so I can't wrap it with my logging command. I've captured the actual SQL queries using SQL Server Extended Events:

image

Would it be possible to avoid subsequent probes for the SimpleInt table type after we've done so once?

jonwagner commented 6 years ago

Ah, I totally forgot that you can use TVPs with dynamic SQL. :)

We can definitely cache these. I'm planning on taking a day next week to work on some open issues.

nzbart commented 6 years ago

That'd be excellent, thanks!

andymac4182 commented 6 years ago

@jonwagner If you are busy and can point me in the right direction I can look at helping with this.

jonwagner commented 6 years ago

It’s next on my non-billable list for this week and the last item before I build 6.2. I think it might just need a quick cache map in the sql provider.

On Mar 5, 2018, at 11:38 PM, Andrew McClenaghan notifications@github.com<mailto:notifications@github.com> wrote:

@jonwagnerhttps://github.com/jonwagner If you are busy and can point me in the right direction I can look at helping with this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/jonwagner/Insight.Database/issues/354#issuecomment-370660140, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABk3dITdfqQFzdvVfhSU6HbPJgZnTvN5ks5tbhLKgaJpZM4SLS8r.

jonwagner commented 6 years ago

I'm on this today...

jonwagner commented 6 years ago

this is definitely a performance bug and not just an edge case

jonwagner commented 6 years ago

@nzbart There is a fix for this in master. Can you run some tests to make sure that it handles all of the cases you are seeing? If so, I can kick off an official build.

nzbart commented 6 years ago

@jonwagner I've run a few tests against it, and it appears to be working well. Thanks for getting onto this so quickly!

nzbart commented 6 years ago

Sorry, I didn't intend to close the issue - just clicked the wrong button.

jonwagner commented 6 years ago

Perfect. I'm going to test out the new oracle .net core driver and then i'll do an official build.

jonwagner commented 6 years ago

6.2 is in nuget