Dapper icon indicating copy to clipboard operation
Dapper copied to clipboard

SqlMapper Query param empty list cause table scan in mysql

Open GREsau opened this issue 3 years ago • 2 comments

Previously reported in #1379, but the issue was closed by the original reporter for some reason, despite the actual problem still being present in Dapper.

connection.Query(
 "Select xxx from test WHERE messageid IN @messageids",
 new { messageids = new List() }
);

Generates the SQL Select xxx from test WHERE messageid IN (SELECT NULL WHERE 1 = 0), which causes a table scan in mysql due to poor query optimization.

@jayrowe's excellent investigation in the original issue shows that the table scan can be avoided by adding any FROM clause to the inner select, e.g. Select xxx from test WHERE messageid IN (SELECT NULL FROM (SELECT NULL) n WHERE 1 = 0)

Would you be open to a PR implementing this change?

GREsau avatar Aug 19 '22 10:08 GREsau

@GREsau - Technically this is a MySql optimizer bug, but I agree - if Dapper could generate something more reasonable it could make the troubles go away. I'd venture a guess there is a subset of the population that doesn't realize this is happening because their tables are small enough that they don't notice. A change in Dapper would be easier and safer than anything an outsider can do.

I needed this resolved in production ASAP and I already have an implementation of DBConnection that I use to wrap the MySqlConnection for instrumentation and other purposes. I modified that to do the fixup for me whenever CommandText is set.

I've extracted just the bits that should be necessary for this here; no guarantees that it actually compiles, but it should be pretty close. The regex is not correct but is "correct enough" for my purposes given the types of sql we're using. In particular you'd need to watch out if you're building dynamic sql (hopefully you're not) to be sure the regex doesn't snag the problematic text if it appears within a string or something like that. If you're using positional parameters... well, not my problem.

If you've got a factory somewhere that generates connections for you, just change it to wrap your connection in one of these:

public class DapperFixupDbConnection : DbConnection
{
    internal readonly DbConnection _connection;

    public DapperFixupDbConnection(DbConnection connection)
    {
        _connection = connection ?? throw new ArgumentNullException();
        _connection.StateChange += (o, e) => OnStateChange(e);
    }

    public override string ConnectionString
    {
        get => _connection.ConnectionString;
        set => _connection.ConnectionString = value;
    }

    public override string Database => _connection.Database;

    public override string DataSource => _connection.DataSource;

    public override string ServerVersion => _connection.ServerVersion;

    public override ConnectionState State => _connection.State;

    public override void ChangeDatabase(string databaseName) => _connection.ChangeDatabase(databaseName);

    public override void Close() => _connection.Close();

    public override void Open() => _connection.Open();

    public override Task OpenAsync(CancellationToken cancellationToken) => _connection.OpenAsync(cancellationToken);

    protected override DbTransaction BeginDbTransaction(IsolationLevel isolationLevel) => _connection.BeginTransaction(isolationLevel);

    protected override DbCommand CreateDbCommand() => new DapperFixupDbCommand(this, _connection.CreateCommand());

    public override int ConnectionTimeout => _connection.ConnectionTimeout;

    protected override void Dispose(bool disposing)
    {
       base.Dispose(disposing);

        if (disposing)
        {
            _connection.Dispose();
        }
    }

    public override void EnlistTransaction(System.Transactions.Transaction transaction) => _connection.EnlistTransaction(transaction);

    public override DataTable GetSchema() => _connection.GetSchema();

    public override DataTable GetSchema(string collectionName) => _connection.GetSchema(collectionName);

    public override DataTable GetSchema(string collectionName, string[] restrictionValues) => _connection.GetSchema(collectionName, restrictionValues);

    public override ISite Site
    {
        get => _connection.Site;
        set => _connection.Site = value;
    }
}

internal class DapperFixupDbCommand : DbCommand
{
    private DbConnection _connection;
    private readonly DbCommand _command;

    public DapperFixupDbCommand(DbConnection connection, DbCommand command)
    {
        _connection = connection;
        _command = command;
    }

    public override string CommandText
    {
        get => _command.CommandText;
        set => _command.CommandText = FixupDapperSql(value) ?? "";
    }
    public override int CommandTimeout
    {
        get => _command.CommandTimeout;
        set => _command.CommandTimeout = value;
    }
    public override CommandType CommandType
    {
        get => _command.CommandType;
        set => _command.CommandType = value;
    }
    public override bool DesignTimeVisible
    {
        get => _command.DesignTimeVisible;
        set => _command.DesignTimeVisible = value;
    }
    public override UpdateRowSource UpdatedRowSource
    {
        get => _command.UpdatedRowSource;
        set => _command.UpdatedRowSource = value;
    }
    protected override DbConnection DbConnection
    {
        get => _connection;
        set
        {
            if (value is null)
            {
                _command.Connection = null;
                _connection = null;

                return;
            }

            if (value is not DapperFixupDbConnection connection)
            {
                throw new ArgumentException("Connection must be a DapperFixupDbConnection instance");
            }

            _command.Connection = connection._connection;
            _connection = connection;
        }
    }

    protected override DbParameterCollection DbParameterCollection => _command.Parameters;

    protected override DbTransaction DbTransaction
    {
        get => _command.Transaction;
        set => _command.Transaction = value;
    }

    public override void Cancel() => _command.Cancel();

    public override int ExecuteNonQuery() => _command.ExecuteNonQuery();

    public override object ExecuteScalar() => _command.ExecuteScalar();

    public override void Prepare() => _command.Prepare();

    protected override DbParameter CreateDbParameter() => _command.CreateParameter();

    protected override DbDataReader ExecuteDbDataReader(CommandBehavior behavior) => _command.ExecuteDbDataReader(behavior);

    protected override void Dispose(bool disposing)
    {
        base.Dispose(disposing);

        if (disposing)
        {
            _command.Dispose();
        }
    }

    protected override Task<DbDataReader> ExecuteDbDataReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken) => _command.ExecuteDbDataReaderAsync(behavior, cancellationToken);

    public override Task<int> ExecuteNonQueryAsync(CancellationToken cancellationToken) => _command.ExecuteNonQueryAsync(cancellationToken);

    public override Task<object> ExecuteScalarAsync(CancellationToken cancellationToken) => _command.ExecuteScalarAsync(cancellationToken);

    private static readonly Regex _emptyListSql = new(
        @"\(SELECT @\w+ WHERE 1 = 0\)",
        RegexOptions.Compiled);

    // this is sketchy and probably a bit dangerous
    private static string FixupDapperSql(string sql)
    {
        if (sql == null)
        {
            return null;
        }

        return _emptyListSql.Replace(
            sql,
            match => match.Value.Replace("WHERE 1 = 0", "FROM (SELECT NULL) n WHERE 1 = 0"));
    }
}

No warranties neither express nor implied, use at your own risk, blah blah.

jayrowe avatar Aug 19 '22 18:08 jayrowe

Any plan to add a workaround until mysql fixes their query optimizer?

Also removing a where condition "Select xxx from test WHERE messageid IN (SELECT NULL )" will also yield an optimal query plan in mysql

avireddy02 avatar Jun 08 '23 20:06 avireddy02