Skip to content

Conversation

@NinoFloris
Copy link
Member

@NinoFloris NinoFloris commented Nov 16, 2025

This has been a change I've been meaning to make ever since we released 8.0, as it was too late in the cycle to rework it and think it all through at the time.

We have a bunch of accidental complexity - additional types and state, and duplicated logic to work on these things that must not be forgotten or things go wrong in rare cases - due to the fact we need to support both concrete converter and converter resolver based PgTypeInfos today. Today the public PgConverterResolution, the internal PgConverterInfo and even to some extent the internal ColumnInfo exist because we need to handle that split all the way at the actual read and write call sites. That means we must handle this split correctly in all the following places: NpgsqlParameter, NpgsqlBinaryImporter/NpgsqlBinaryExporter, NpgsqlDataReader, NpgsqlNestedDataReader, ReplicationValue, CompositeConverter, and ObjectConverter.

Unsurprisingly it would be much better if all of this could live in one place, and I recently got inspired to finally tackle this after reviewing #6005. The feature that the linked PR intends to enable deals with all the same complexity of value dependent writing to provide general DataTable to pg (table) composite support which they would like to release as a plugin.

To improve the status quo this PR starts by making PgTypeInfo abstract and adds a new derived type to the hierarchy: PgConcreteTypeInfo, this will be the authoritative info source that gets used during reads and writes. We also rename PgConverterResolver to PgConcreteTypeInfoProvider and PgResolverTypeInfo to PgProviderTypeInfo. These renames align the types with what they operate on and gets rid of the overloaded resolver/resolution moniker, which will now just relate to the IPgTypeInfoResolver concept.

Shifting the PgTypeInfo hierarchy into this direction allows us to merge the concrete/resolver split back into one source after field and value binding. For most cases nothing really changes on the mapping authoring side except the type identity: mappings that don't rely on a resolver will now return PgConcreteTypeInfo. As most mappings rely on mapping.CreateInfo to create their PgTypeInfo we are able to change this in a binary compatible way. And if the PgTypeInfo constructor does happen to be used today a small change to new up PgConcreteTypeInfo instead is sufficient. The PgResolverTypeInfo and PgConverterResolver changes are more breaking, though also easily adapted to. Regardless, no users of this api surface have been found after a thorough github search; and it's unlikely private implementations exist either. Additionally all of this api surface is marked experimental, and while I don't advocate for doing impactful breaking changes just because we can, it is intended to give us that kind of wiggle room.

Finally, where mappings do use the PgProviderTypeInfo functionality (like bitstring fields or datetime values) the provider implementation will become responsible for returning *cached* PgConcreteTypeInfo instances. The unwillingness to demand this caching was the original sin that lead to this split, and why the PgConverterResolution return type was a struct. At the time it was unclear to me whether we could always cache these results (which would otherwise lead to an unavoidable allocation on every bind) as the providers may act on ever changing values. In practice all implementations today end up caching their produced converter instances already. And at this point I'm also convinced that any useful implementation classifies fields and values into a few well known categories for which it is never a problem to cache the required variations; even if the number of variations must now factor in the number of PgTypeIds supported, as these are stored in the info instance.

The total work is split across PRs to make reviewing more practical. This PR mostly contains the nominal and type level restructuring, while the follow up PR will use these changes to actually deliver the simplification of the call sites. A final PR should provide the public surface that we can recommend to plugin authors interested in working with POCO or dynamic (e.g. datatable) conversions that interact correctly with nested field and value binding.

@NinoFloris NinoFloris force-pushed the prework-read-write-streamlining branch from 0fb90c0 to 7982f05 Compare November 16, 2025 23:29
@NinoFloris
Copy link
Member Author

Rebased onto main

@NinoFloris NinoFloris added this to the 11.0.0 milestone Nov 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant