Skip to content

Rewrite user type mappings #4970

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vonzshik wants to merge 2 commits into from
Closed

Rewrite user type mappings #4970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ffc5c7
Rewrite user type mappings
vonzshik Mar 4, 2023
e130e18
Remove few outdated interfaces
vonzshik Mar 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Npgsql/Internal/TypeHandling/TypeMappingInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

namespace Npgsql.Internal.TypeHandling;

public class TypeMappingInfo
public sealed class TypeMappingInfo
{
public TypeMappingInfo(NpgsqlDbType? npgsqlDbType, string? dataTypeName, Type clrType)
=> (NpgsqlDbType, DataTypeName, ClrTypes) = (npgsqlDbType, dataTypeName, new[] { clrType });
Expand Down
20 changes: 1 addition & 19 deletions src/Npgsql/Internal/TypeMapping/TypeMapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ internal NpgsqlDatabaseInfo DatabaseInfo
readonly ConcurrentDictionary<Type, NpgsqlTypeHandler> _handlersByClrType = new();
readonly ConcurrentDictionary<string, NpgsqlTypeHandler> _handlersByDataTypeName = new();

readonly Dictionary<uint, TypeMappingInfo> _userTypeMappings = new();
readonly INpgsqlNameTranslator _defaultNameTranslator;

readonly ILogger _commandLogger;
Expand All @@ -63,28 +62,14 @@ internal TypeMapper(NpgsqlConnector connector, INpgsqlNameTranslator defaultName

internal void Initialize(
NpgsqlDatabaseInfo databaseInfo,
List<TypeHandlerResolverFactory> resolverFactories,
Dictionary<string, IUserTypeMapping> userTypeMappings)
List<TypeHandlerResolverFactory> resolverFactories)
{
_databaseInfo = databaseInfo;

var resolvers = new TypeHandlerResolver[resolverFactories.Count];
for (var i = 0; i < resolverFactories.Count; i++)
resolvers[i] = resolverFactories[i].Create(this, Connector);
_resolvers = resolvers;

foreach (var userTypeMapping in userTypeMappings.Values)
{
if (DatabaseInfo.TryGetPostgresTypeByName(userTypeMapping.PgTypeName, out var pgType))
{
_handlersByOID[pgType.OID] =
_handlersByDataTypeName[pgType.FullName] =
_handlersByDataTypeName[pgType.Name] =
_handlersByClrType[userTypeMapping.ClrType] = userTypeMapping.CreateHandler(pgType, Connector);

_userTypeMappings[pgType.OID] = new(npgsqlDbType: null, pgType.Name, userTypeMapping.ClrType);
}
}
}

#region Type handler lookup
Expand Down Expand Up @@ -495,9 +480,6 @@ internal bool TryGetMapping(PostgresType pgType, [NotNullWhen(true)] out TypeMap
}

break;

case PostgresEnumType or PostgresCompositeType:
return _userTypeMappings.TryGetValue(pgType.OID, out mapping);
}

mapping = null;
Expand Down
7 changes: 1 addition & 6 deletions src/Npgsql/Internal/TypeMapping/UserCompositeTypeMappings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,11 @@

namespace Npgsql.Internal.TypeMapping;

public interface IUserCompositeTypeMapping : IUserTypeMapping
sealed class UserCompositeTypeMapping<T> : IUserTypeMapping
{
INpgsqlNameTranslator NameTranslator { get; }
}

sealed class UserCompositeTypeMapping<T> : IUserCompositeTypeMapping
{
public string PgTypeName { get; }
public Type ClrType => typeof(T);
public INpgsqlNameTranslator NameTranslator { get; }

public UserCompositeTypeMapping(string pgTypeName, INpgsqlNameTranslator nameTranslator)
=> (PgTypeName, NameTranslator) = (pgTypeName, nameTranslator);
Expand Down
10 changes: 2 additions & 8 deletions src/Npgsql/Internal/TypeMapping/UserEnumTypeMappings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,18 @@

namespace Npgsql.Internal.TypeMapping;

public interface IUserEnumTypeMapping : IUserTypeMapping
{
INpgsqlNameTranslator NameTranslator { get; }
}

sealed class UserEnumTypeMapping<TEnum> : IUserEnumTypeMapping
sealed class UserEnumTypeMapping<TEnum> : IUserTypeMapping
where TEnum : struct, Enum
{
public string PgTypeName { get; }
public Type ClrType => typeof(TEnum);
public INpgsqlNameTranslator NameTranslator { get; }

readonly Dictionary<TEnum, string> _enumToLabel = new();
readonly Dictionary<string, TEnum> _labelToEnum = new();

public UserEnumTypeMapping(string pgTypeName, INpgsqlNameTranslator nameTranslator)
{
(PgTypeName, NameTranslator) = (pgTypeName, nameTranslator);
PgTypeName = pgTypeName;

foreach (var field in typeof(TEnum).GetFields(BindingFlags.Static | BindingFlags.Public))
{
Expand Down
4 changes: 1 addition & 3 deletions src/Npgsql/NpgsqlDataSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ public abstract class NpgsqlDataSource : DbDataSource
internal NpgsqlLoggingConfiguration LoggingConfiguration { get; }

readonly List<TypeHandlerResolverFactory> _resolverFactories;
readonly Dictionary<string, IUserTypeMapping> _userTypeMappings;
readonly INpgsqlNameTranslator _defaultNameTranslator;

internal TypeMapper TypeMapper { get; private set; } = null!; // Initialized at bootstrapping
Expand Down Expand Up @@ -95,7 +94,6 @@ internal NpgsqlDataSource(
_periodicPasswordSuccessRefreshInterval,
_periodicPasswordFailureRefreshInterval,
_resolverFactories,
_userTypeMappings,
_defaultNameTranslator,
ConnectionInitializer,
ConnectionInitializerAsync,
Expand Down Expand Up @@ -236,7 +234,7 @@ internal async Task Bootstrap(

DatabaseInfo = databaseInfo;
connector.DatabaseInfo = databaseInfo;
typeMapper.Initialize(databaseInfo, _resolverFactories, _userTypeMappings);
typeMapper.Initialize(databaseInfo, _resolverFactories);
TypeMapper = typeMapper;

_isBootstrapped = true;
Expand Down
1 change: 0 additions & 1 deletion src/Npgsql/NpgsqlDataSourceConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ sealed record NpgsqlDataSourceConfiguration(
TimeSpan PeriodicPasswordSuccessRefreshInterval,
TimeSpan PeriodicPasswordFailureRefreshInterval,
List<TypeHandlerResolverFactory> ResolverFactories,
Dictionary<string, IUserTypeMapping> UserTypeMappings,
INpgsqlNameTranslator DefaultNameTranslator,
Action<NpgsqlConnection>? ConnectionInitializer,
Func<NpgsqlConnection, Task>? ConnectionInitializerAsync,
Expand Down
37 changes: 27 additions & 10 deletions src/Npgsql/NpgsqlSlimDataSourceBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ public sealed class NpgsqlSlimDataSourceBuilder : INpgsqlTypeMapper
TimeSpan _periodicPasswordSuccessRefreshInterval, _periodicPasswordFailureRefreshInterval;

readonly List<TypeHandlerResolverFactory> _resolverFactories = new();
readonly Dictionary<string, IUserTypeMapping> _userTypeMappings = new();

/// <inheritdoc />
public INpgsqlNameTranslator DefaultNameTranslator { get; set; } = GlobalTypeMapper.Instance.DefaultNameTranslator;
Expand Down Expand Up @@ -276,6 +275,27 @@ internal void AddDefaultTypeResolverFactory(TypeHandlerResolverFactory resolverF
throw new Exception("No built-in resolver factory found");
}

void AddUserMappingResolver(IUserTypeMapping userMapping)
{
RemoveUserMappingResolver(userMapping.PgTypeName);
var factory = new UserMappedTypeHandlerResolverFactory(userMapping);
Copy link
Member

@roji roji Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I thought of the resolvers list as always being quite small, e.g. how many plugins could there possibly be.. I'm not sure if we have places where perf would be problematic if this list starts growing; IIRC we should only actually access the resolvers for NpgsqlDbType/Type that we haven't seen before (we cache). Maybe on connection startup (when the cache is empty) this could in theory become problematic, if the user has e.g. 200 enums/composites or something.

To be on the safe side, we could have a single resolver handle them all; though if you're convinced this doesn't matter I'm OK with it.

Copy link
Contributor Author

@vonzshik vonzshik Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe on connection startup (when the cache is empty) this could in theory become problematic, if the user has e.g. 200 enums/composites or something.

You do have a point here. A few of my thoughts below:

  1. With 200+ enums/composites a cold start will be slow no matter what we do (as it implies a big codebase).
  2. One way to go around this is to implement resolver sorting. Since enums/composites don't interfere with BuiltInResolver (and the same thing can be said about all of the resolvers we implement in Implement NpgsqlDataSource #4495) we can put BuiltInResolver first and enums/composites last (+ resolvers from Implement NpgsqlDataSource #4495).
  3. While we do cache the results of the resolvers, there is an exception to that rule: resolving by value. Because of DateTime's kind we have to return either timestamp or timestamptz.

In light of the above I propose:

  1. We do implement a single resolver for custom types.
  2. We consider sorting resolvers (putting BuiltInResolver first, most of out internal resolvers after, and anything a user adds before BuiltInResolver).

What do you say?

Copy link
Contributor Author

@vonzshik vonzshik Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do implement a single resolver for custom types.

Though that's probably going to complicate things for the global type mapper...

Copy link
Member

@roji roji Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NinoFloris what do you think of the above? We've discussed simplying our type handler concepts by bringing ideas across from Slon; if we do that, it may not be worth worrying too much about the current situation...

Copy link
Member

@NinoFloris NinoFloris Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, Slon's model also relies on resolvers for this work so in that respect nothing changes. If there are a huge number of (user) resolvers to check before ending up at the builtin set it would equally slow things down until the cache is properly filled.

I would probably have something like the following order:

  • Builtin
  • User mappings (just one resolver)
  • (Additive) plugins (however many resolvers as there are plugins)

In Slon the only time you need another resolver to be first is for overriding the given converter for some clr type or overriding info (default clr type for a pg type, text writing preferred etc).

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

_resolverFactories.Insert(0, factory);
}

bool RemoveUserMappingResolver(string pgName)
{
for (var i = 0; i < _resolverFactories.Count; i++)
{
if (_resolverFactories[i] is UserMappedTypeHandlerResolverFactory { } resolverFactory && resolverFactory.Mapping.PgTypeName == pgName)
{
_resolverFactories.RemoveAt(i);
return true;
}
}

return false;
}

/// <inheritdoc />
public INpgsqlTypeMapper MapEnum<TEnum>(string? pgName = null, INpgsqlNameTranslator? nameTranslator = null)
where TEnum : struct, Enum
Expand All @@ -286,7 +306,8 @@ public INpgsqlTypeMapper MapEnum<TEnum>(string? pgName = null, INpgsqlNameTransl
nameTranslator ??= DefaultNameTranslator;
pgName ??= GetPgName(typeof(TEnum), nameTranslator);

_userTypeMappings[pgName] = new UserEnumTypeMapping<TEnum>(pgName, nameTranslator);
var userMapping = new UserEnumTypeMapping<TEnum>(pgName, nameTranslator);
AddUserMappingResolver(userMapping);
return this;
}

Expand All @@ -300,7 +321,7 @@ public bool UnmapEnum<TEnum>(string? pgName = null, INpgsqlNameTranslator? nameT
nameTranslator ??= DefaultNameTranslator;
pgName ??= GetPgName(typeof(TEnum), nameTranslator);

return _userTypeMappings.Remove(pgName);
return RemoveUserMappingResolver(pgName);
}

/// <inheritdoc />
Expand All @@ -313,7 +334,8 @@ public INpgsqlTypeMapper MapComposite<T>(string? pgName = null, INpgsqlNameTrans
nameTranslator ??= DefaultNameTranslator;
pgName ??= GetPgName(typeof(T), nameTranslator);

_userTypeMappings[pgName] = new UserCompositeTypeMapping<T>(pgName, nameTranslator);
var userMapping = new UserCompositeTypeMapping<T>(pgName, nameTranslator);
AddUserMappingResolver(userMapping);
return this;
}

Expand Down Expand Up @@ -343,7 +365,7 @@ public bool UnmapComposite(Type clrType, string? pgName = null, INpgsqlNameTrans
nameTranslator ??= DefaultNameTranslator;
pgName ??= GetPgName(clrType, nameTranslator);

return _userTypeMappings.Remove(pgName);
return RemoveUserMappingResolver(pgName);
}

void INpgsqlTypeMapper.Reset()
Expand All @@ -358,10 +380,6 @@ void ResetTypeMappings()
_resolverFactories.Clear();
foreach (var resolverFactory in globalMapper.ResolverFactories)
_resolverFactories.Add(resolverFactory);

_userTypeMappings.Clear();
foreach (var kv in globalMapper.UserTypeMappings)
_userTypeMappings[kv.Key] = kv.Value;
}
finally
{
Expand Down Expand Up @@ -462,7 +480,6 @@ _loggerFactory is null
_periodicPasswordSuccessRefreshInterval,
_periodicPasswordFailureRefreshInterval,
_resolverFactories,
_userTypeMappings,
DefaultNameTranslator,
_syncConnectionInitializer,
_asyncConnectionInitializer,
Expand Down
48 changes: 27 additions & 21 deletions src/Npgsql/TypeMapping/GlobalTypeMapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ sealed class GlobalTypeMapper : INpgsqlTypeMapper
public INpgsqlNameTranslator DefaultNameTranslator { get; set; } = new NpgsqlSnakeCaseNameTranslator();

internal List<TypeHandlerResolverFactory> ResolverFactories { get; } = new();
public ConcurrentDictionary<string, IUserTypeMapping> UserTypeMappings { get; } = new();

readonly ConcurrentDictionary<Type, TypeMappingInfo> _mappingsByClrType = new();

Expand All @@ -48,8 +47,7 @@ public INpgsqlTypeMapper MapEnum<TEnum>(string? pgName = null, INpgsqlNameTransl
Lock.EnterWriteLock();
try
{
UserTypeMappings[pgName] = new UserEnumTypeMapping<TEnum>(pgName, nameTranslator);
RecordChange();
AddUserMappingResolver(new UserEnumTypeMapping<TEnum>(pgName, nameTranslator));
return this;
}
finally
Expand All @@ -70,13 +68,7 @@ public bool UnmapEnum<TEnum>(string? pgName = null, INpgsqlNameTranslator? nameT
Lock.EnterWriteLock();
try
{
if (UserTypeMappings.TryRemove(pgName, out _))
{
RecordChange();
return true;
}

return false;
return RemoveUserMappingResolver(pgName);
}
finally
{
Expand All @@ -96,8 +88,7 @@ public INpgsqlTypeMapper MapComposite<T>(string? pgName = null, INpgsqlNameTrans
Lock.EnterWriteLock();
try
{
UserTypeMappings[pgName] = new UserCompositeTypeMapping<T>(pgName, nameTranslator);
RecordChange();
AddUserMappingResolver(new UserCompositeTypeMapping<T>(pgName, nameTranslator));
return this;
}
finally
Expand Down Expand Up @@ -132,13 +123,7 @@ public bool UnmapComposite(Type clrType, string? pgName = null, INpgsqlNameTrans
Lock.EnterWriteLock();
try
{
if (UserTypeMappings.TryRemove(pgName, out _))
{
RecordChange();
return true;
}

return false;
return RemoveUserMappingResolver(pgName);
}
finally
{
Expand Down Expand Up @@ -174,6 +159,29 @@ public void AddTypeResolverFactory(TypeHandlerResolverFactory resolverFactory)
}
}

void AddUserMappingResolver(IUserTypeMapping userMapping)
{
RemoveUserMappingResolver(userMapping.PgTypeName);
var factory = new UserMappedTypeHandlerResolverFactory(userMapping);
ResolverFactories.Insert(0, factory);
RecordChange();
}

bool RemoveUserMappingResolver(string pgName)
{
for (var i = 0; i < ResolverFactories.Count; i++)
{
if (ResolverFactories[i] is UserMappedTypeHandlerResolverFactory { } resolverFactory && resolverFactory.Mapping.PgTypeName == pgName)
{
ResolverFactories.RemoveAt(i);
RecordChange();
return true;
}
}

return false;
}

public void Reset()
{
Lock.EnterWriteLock();
Expand All @@ -182,8 +190,6 @@ public void Reset()
ResolverFactories.Clear();
ResolverFactories.Add(new BuiltInTypeHandlerResolverFactory());

UserTypeMappings.Clear();

RecordChange();
}
finally
Expand Down
62 changes: 62 additions & 0 deletions src/Npgsql/TypeMapping/UserMappedTypeHandlerResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using Npgsql.Internal;
using Npgsql.Internal.TypeHandling;
using Npgsql.Internal.TypeMapping;
using Npgsql.PostgresTypes;

namespace Npgsql.TypeMapping;

sealed class UserMappedTypeHandlerResolver : TypeHandlerResolver
{
readonly NpgsqlConnector _connector;
readonly IUserTypeMapping _mapping;
readonly uint? _typeOID;
readonly TypeMappingInfo? _typeMapping;

NpgsqlTypeHandler? _handler;

public UserMappedTypeHandlerResolver(NpgsqlConnector connector, IUserTypeMapping mapping)
{
_connector = connector;
_mapping = mapping;

if (connector.DatabaseInfo.TryGetPostgresTypeByName(mapping.PgTypeName, out var pgType))
{
_typeOID = pgType.OID;
_typeMapping = new(npgsqlDbType: null, pgType.Name, mapping.ClrType);
}
}

public override NpgsqlTypeHandler? ResolveByDataTypeName(string typeName)
{
if (_connector.DatabaseInfo.TryGetPostgresTypeByName(_mapping.PgTypeName, out var pgType)
&& (pgType.FullName == typeName || pgType.Name == typeName))
return GetHandler(pgType);

return null;
}

public override NpgsqlTypeHandler? ResolveByClrType(Type type)
{
if (_connector.DatabaseInfo.TryGetPostgresTypeByName(_mapping.PgTypeName, out var pgType) && _mapping.ClrType == type)
return GetHandler(pgType);

return null;
}

NpgsqlTypeHandler GetHandler(PostgresType pgType) => _handler ??= _mapping.CreateHandler(pgType, _connector);

public override NpgsqlTypeHandler? ResolveByPostgresType(PostgresType type)
=> ResolveByDataTypeName(type.FullName);

public override TypeMappingInfo? GetMappingByPostgresType(PostgresType type)
{
if (_typeOID != type.OID)
return null;
Debug.Assert(_typeMapping is not null);
return _typeMapping;
}
}
Loading