connection.TypeMapper.RemoveMapping / AddMapping bug

[speige]

Steps to reproduce

public static class UnitTests
{
    private static string _connectionString = 
        "Server=localhost;Port=5432;Database=myDatabase;User Id=myUserName;Password=myPassword";

    public static void FailsWithConnectionSpecificMappingAndDapper()
    {
        using (NpgsqlConnection connection = new NpgsqlConnection(_connectionString))
        {
            connection.Open();

            connection.TypeMapper.RemoveMapping("text");
            connection.TypeMapper.AddMapping(new NpgsqlTypeMappingBuilder
            {
                PgTypeName = "citext",
                NpgsqlDbType = NpgsqlDbType.Citext,
                DbTypes = new[] { DbType.String },
                ClrTypes = new[] { typeof(string) },
                TypeHandlerFactory = new TextHandlerFactory()
            }.Build());

            if (!connection.Query<bool>(
                "SELECT @p = 'hello'::citext", new { p = "HeLLo" }).First()) // fails here
            {
                throw new Exception("failed"); // never makes it here
            }
        }
    }

    public static void WorksWithGlobalMappingAndDapper()
    {
        NpgsqlConnection.GlobalTypeMapper.RemoveMapping("text");
        NpgsqlConnection.GlobalTypeMapper.AddMapping(new NpgsqlTypeMappingBuilder
        {
            PgTypeName = "citext",
            NpgsqlDbType = NpgsqlDbType.Citext,
            DbTypes = new[] { DbType.String },
            ClrTypes = new[] { typeof(string) },
            TypeHandlerFactory = new TextHandlerFactory()
        }.Build());

        using (NpgsqlConnection connection = new NpgsqlConnection(_connectionString))
        {
            connection.Open();

            if (!connection.Query<bool>(
                "SELECT @p = 'hello'::citext", new { p = "HeLLo" }).First())
            {
                throw new Exception("failed");
            }
        }
    }

    public static void WorksWithConnectionSpecificMappingAndAdoNet()
    {
        using (NpgsqlConnection connection = new NpgsqlConnection(_connectionString))
        {
            connection.Open();

            connection.TypeMapper.RemoveMapping("text");
            connection.TypeMapper.AddMapping(new NpgsqlTypeMappingBuilder
            {
                PgTypeName = "citext",
                NpgsqlDbType = NpgsqlDbType.Citext,
                DbTypes = new[] { DbType.String },
                ClrTypes = new[] { typeof(string) },
                TypeHandlerFactory = new TextHandlerFactory()
            }.Build());

            using (NpgsqlCommand command = new NpgsqlCommand(
                "SELECT @p = 'hello'::citext", connection))
            {
                command.Parameters.AddWithValue("p", "HeLLo");

                if (!(bool)command.ExecuteScalar())
                {
                    throw new Exception("failed");
                }
            }
        }
    }
}

Note: Make sure you run FailsWithConnectionSpecificMappingAndDapper 1st because if WorksWithGlobalMappingAndDapper runs 1st it will mask the bug.

The issue

I'm using the citext mapping that you suggested in response to #1765 (related to #1475).
I think there is a bug, but it's a rare use case so it probably hasn't been noticed.

Your unit test here shows how to map it at the connection-specific level. This works fine with ADO.Net, but fails with Dapper. I've debugged it a bit & believe it's an Npgsql issue, not a Dapper issue. However, NpgsqlConnection.GlobalTypeMapper works with both Ado.Net & Dapper.

From stepping through the code it seems to be related to GlobalTypeMapper.Instance.ToNpgsqlDbType(value) not getting setup correctly when the mapping is changed on a local connection. I haven't debugged any more than that.

On a side note, PostgreSQL can implicitly type-cast between text & citext if needed, so it may not be obvious that you need to remap citext in npgsql because your queries might return the correct results even without the mapping. The reason I even attempted to map citext was because my queries were extremely slow in npgsql but fast in pgAdmin. After lengthy debugging, I realized it was because my table columns were citext, but I wasn't mapping to citext in npgsql. Maybe this could be mentioned at https://www.npgsql.org/doc/performance.html to help other people avoid my mistake?

Exception Message:

{Npgsql.NpgsqlException (0x80004005): The NpgsqlDbType 'Text' isn't present in your database. You may need to install an extension or upgrade to a newer version.

Stack Trace:

   at Npgsql.TypeMapping.ConnectorTypeMapper.GetByNpgsqlDbType(NpgsqlDbType npgsqlDbType) in C:\projects\npgsql\src\Npgsql\TypeMapping\ConnectorTypeMapper.cs:line 106
   at Npgsql.NpgsqlParameter.ResolveHandler(ConnectorTypeMapper typeMapper) in C:\projects\npgsql\src\Npgsql\NpgsqlParameter.cs:line 523
   at Npgsql.NpgsqlCommand.ValidateParameters() in C:\projects\npgsql\src\Npgsql\NpgsqlCommand.cs:line 796
   at Npgsql.NpgsqlCommand.ExecuteDbDataReader(CommandBehavior behavior, Boolean async, CancellationToken cancellationToken) in C:\projects\npgsql\src\Npgsql\NpgsqlCommand.cs:line 1141
   at Npgsql.NpgsqlCommand.ExecuteDbDataReader(CommandBehavior behavior) in C:\projects\npgsql\src\Npgsql\NpgsqlCommand.cs:line 1130
   at System.Data.Common.DbCommand.System.Data.IDbCommand.ExecuteReader(CommandBehavior behavior)
   at Dapper.SqlMapper.ExecuteReaderWithFlagsFallback(IDbCommand cmd, Boolean wasClosed, CommandBehavior behavior) in C:\projects\dapper\Dapper\SqlMapper.cs:line 1053
   at Dapper.SqlMapper.QueryImpl[T](IDbConnection cnn, CommandDefinition command, Type effectiveType)+MoveNext() in C:\projects\dapper\Dapper\SqlMapper.cs:line 1081
   at System.Collections.Generic.List`1.AddEnumerable(IEnumerable`1 enumerable)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at Dapper.SqlMapper.Query[T](IDbConnection cnn, String sql, Object param, IDbTransaction transaction, Boolean buffered, Nullable`1 commandTimeout, Nullable`1 commandType) in C:\projects\dapper\Dapper\SqlMapper.cs:line 723

Further technical details

Npgsql version: 4.0.3
Dapper: 1.50.5
PostgreSQL version: PostgreSQL 10.1, compiled by Visual C++ build 1800, 64-bit
Operating system: Windows 10 Pro 64-bot

[austindrenski]

From stepping through the code it seems to be related to GlobalTypeMapper.Instance.ToNpgsqlDbType(value) not getting setup correctly when the mapping is changed on a local connection. I haven't debugged any more than that.

Are you expecting changes in the connection's local type mapper to be reflected in the global type mapper instance?

The reason I even attempted to map citext was because my queries were extremely slow in npgsql but fast in pgAdmin.

This is interesting. Could you elaborate on what exactly you're seeing here?

[speige]

Sorry, I should have been more clear.

I expect that no exception will be thrown.
The following query returns false, because the values are case-sensitive.

SELECT 'HeLLo'::text = 'hello'::citext

Without adjusting the mappings, npgsql's default mappings will convert a c# string to a postgresql ::text parameter. I want to ensure that by changing the mappings, npgsql is sending a ::citext parameter to my query and not a ::text parameter.

In my code UnitTests.FailsWithConnectionSpecificMappingAndDapper will throw an exception (The NpgsqlDbType 'Text' isn't present in your database. You may need to install an extension or upgrade to a newer version.). The exception is related to the change in mapping not fully working, so npgsql doesn't know how to execute the query.
I believe it's a bug in npgsql. In https://github.com/npgsql/npgsql/blob/dev/src/Npgsql/NpgsqlParameter.cs the DbType { set; } method has this line of code:

_npgsqlDbType = GlobalTypeMapper.Instance.ToNpgsqlDbType(value);

Essentially Dapper is converting my anonymous object's p property to a Parameter of type DbType.String, but npgsql is converting that to NpgsqlDbType.Text, despite the fact that I have a mapping from DbType.String=>NpgsqlDbType.Citext. This is because NpgsqlParameter is using global mappings, not connection-specific mappings.

I think it's a minor bug with a workaround: do a global mapping instead of connection-specific.

[speige]

Elaborating on the performance issue. It's not technically an npgsql issue, however it's a "gotcha" a programmer might make without realizing it.

In pgAdmin, if you execute this:

--prep work
create table my_users_table ( userid citext )
create index userid_index on my_users_table (userid)
insert into my_users_table (userid) select md5(random()::text) from generate_Series(1,1000000) -- will take a few min
select * from my_users_table order by random() limit 1 -- use this value for following queries

--monitor the ms when running these queries as well as the execution plans
SELECT * from my_users_table where userid = 'VALUE_FROM_ABOVE'::citext -- index only scan (fast)
SELECT * from my_users_table where userid = 'VALUE_FROM_ABOVE'::text -- same as above
SELECT * from my_users_table where userid = 'VALUE_FROM_ABOVE' -- seq scan (slow)

You'll notice that if the column & parameter don't match, postgres is choosing to type-cast every value in the table to a ::text rather than type-casting the parameter to a ::citext. This requires a full table scan which is much slower than an index lookup.
If the data & where clause happen to be the same-case, the programmer will still get the results they expect & not realize there is significant slowness due to poor execution plan.

For this reason, when doing a WHERE clause on a citext column, it's important to modify the mappings in npgsql to ensure you're sending a citext parameter, not a text parameter. If you're creating NpgsqlParameters directly in code this should be obvious. However, most people use shorthand code that auto-generates the NpgsqlParameter behind the scenes (Parameters.AddWithValue or Dapper), so they don't think about whether the underlying parameter was creating as text or citext.

[austindrenski]

Interesting...

NpgsqlCommand.ExecuteDbDataReader calls NpgsqlCommand.ValidateParameters which in turn calls NpgsqlParameter.Bind to re-bind the parameters against the command/connection-specific type mapper.

It may be a bug to use GlobalTypeMapper.Instance in the setter, but it may also be by-design for parameters that are not yet associated with a connection.

I don't have much time to look into this today, but it would be helpful if you could take a look at how Dapper is interacting with these snippets. For example, does Dapper not call ValidateParameters?

npgsql/src/Npgsql/NpgsqlCommand.cs

Lines 1132 to 1140 in c362e1c

	async ValueTask<DbDataReader> ExecuteDbDataReader(CommandBehavior behavior, bool async, CancellationToken cancellationToken)
	{
	var connector = CheckReadyAndGetConnector();
	connector.StartUserAction(this);
	try
	{
	using (cancellationToken.Register(cmd => ((NpgsqlCommand)cmd).Cancel(), this))
	{
	ValidateParameters();

npgsql/src/Npgsql/NpgsqlParameter.cs

Lines 332 to 360 in c362e1c

	public sealed override DbType DbType
	{
	get
	{
	if (_dbType.HasValue) {
	return _dbType.Value;
	}
	if (_value != null) { // Infer from value
	return GlobalTypeMapper.Instance.ToDbType(_value.GetType());
	}
	return DbType.Object;
	}
	set
	{
	Handler = null;
	if (value == DbType.Object)
	{
	_dbType = null;
	_npgsqlDbType = null;
	}
	else
	{
	_dbType = value;
	_npgsqlDbType = GlobalTypeMapper.Instance.ToNpgsqlDbType(value);
	}
	}
	}

npgsql/src/Npgsql/NpgsqlCommand.cs

Lines 791 to 802 in c362e1c

	void ValidateParameters()
	{
	for (var i = 0; i < Parameters.Count; i++)
	{
	var p = Parameters[i];
	if (!p.IsInputDirection)
	continue;
	p.Bind(Connection.Connector.TypeMapper);
	p.LengthCache?.Clear();
	p.ValidateAndGetLength();
	}
	}

npgsql/src/Npgsql/NpgsqlParameter.cs

Lines 534 to 539 in c362e1c

	internal void Bind(ConnectorTypeMapper typeMapper)
	{
	ResolveHandler(typeMapper);
	Debug.Assert(Handler != null);
	FormatCode = Handler.PreferTextWrite ? FormatCode.Text : FormatCode.Binary;
	}

npgsql/src/Npgsql/NpgsqlParameter.cs

Lines 517 to 532 in c362e1c

	internal virtual void ResolveHandler(ConnectorTypeMapper typeMapper)
	{
	if (Handler != null)
	return;
	if (_npgsqlDbType.HasValue)
	Handler = typeMapper.GetByNpgsqlDbType(_npgsqlDbType.Value);
	else if (_dataTypeName != null)
	Handler = typeMapper.GetByDataTypeName(_dataTypeName);
	else if (_dbType.HasValue)
	Handler = typeMapper.GetByDbType(_dbType.Value);
	else if (_value != null)
	Handler = typeMapper.GetByClrType(_value.GetType());
	else
	throw new InvalidOperationException($"Parameter '{ParameterName}' must have its value set");
	}

[roji]

First, mapping string to citext as shown above is probably a bad idea - unless your entire application/database is always case-insensitive, you will have trouble dealing with non-insensitive strings, as this affects all strings written/read. So this should be discouraged except for some pretty specific rare cases...

You may also be interested in DapperLib/Dapper#433 and specifically DapperLib/Dapper#471 for setting string->citext mapping at the Dapper level (instead of at the ADO.NET level).

Regardless, the analysis seems to be correct: when converting between DbType and NpgsqlDbType on NpgsqlParameter, the global type mapper is used. As @austindrenski wrote, this is because the ADO.NET API (unfortunately) allows you to have detached DbParameters, which haven't been added to any to any specific command, and therefore which aren't associated to any specific connection. We are still expected to do various translations on these parameters (e.g. infer an NpgsqlDbType from a set Value), so the only option we have is to use the global type mapper. However, as you pointed out when a command is actually executed, parameters are bound using the command's connection's type mapper.

Looking at this again, I'm unsure on why we eagerly do NpgsqlDbType/DbType conversions when one of them is set, rather than lazily. In other words, when DbType is set, NpgsqlDbType could simply be set to null, and NpgsqlDbType's getter would do the DbType->NpgsqlDbType lookup if required. This would solve this issue, and also be slightly better for perf as useless translations are avoided.

When doing the translation, we could optionally also check whether the parameter is associated to a command which itself is associated with a connection, and if so, use that connection's type mapper instead of the global type mapper. But assuming we make the translations lazy as described above, this doesn't really seem to be needed.

Makes sense?

[speige]

@roji Thanks. Those links are helpful.

I agree that doing lazy conversion from DbType to NpgsqlDbType instead of eager is a good solution for this issue. I also think making the conversion always lazy, rather than only if parameter doesn't have a command/connection, is better. However, I'll leave both of those decision up to you.

Thanks for your help. I have a workaround for now. The changes you proposed should make things a little better for posterity.

What do you think about making a note on the performance webpage? I think a lot of users will only check their execution plans via pgAdmin & not realize that malformed parameters due to using Dapper/NpgSql incorrectly can drastically affect the execution plan.

[roji]

I agree that doing lazy conversion from DbType to NpgsqlDbType instead of eager is a good solution for this issue. I also think making the conversion always lazy, rather than only if parameter doesn't have a command/connection, is better. However, I'll leave both of those decision up to you.

Yeah, that's exactly what I had in mind - the conversion should always be lazy. I don't think it's really worth it to make the conversion look at whether a connection is associated or not - we can probably leave it to always use the global type mapper, at least until someone complains.

What do you think about making a note on the performance webpage? I think a lot of users will only check their execution plans via pgAdmin & not realize that malformed parameters due to using Dapper/NpgSql incorrectly can drastically affect the execution plan.

We can, but it would be an extremely specific note - so specific that someone would probably have to suspect a perf issue around citext/text in order to go find it... So I'm a bit skeptical. Take a look at the current perf page, and if you want to submit a PR for it we'll definitely consider it.