Prework for read write streamlining #6316
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PgConverterResolvertoPgConcreteTypeInfoProviderandPgResolverTypeInfotoPgProviderTypeInfo. 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.CreateInfoto 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.