Add TPC support to update pipeline #27737
Conversation
Switch the update pipeline to use the relational model for command ordering Add provider value comparer Part of #3170
src/EFCore.Relational/Infrastructure/EntityFrameworkRelationalServicesBuilder.cs
Show resolved
Hide resolved
src/EFCore.Relational/Metadata/Internal/TableBaseIdentityComparer.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Metadata/Internal/TableBaseIdentityComparer.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Metadata/Internal/TableBaseIdentityComparer.cs
Outdated
Show resolved
Hide resolved
| object? originalValue = null; | ||
| object? currentValue = null; | ||
| RelationalTypeMapping? typeMapping = null; | ||
| foreach (var entry in command.Entries) |
There was a problem hiding this comment.
Any possible concerns about perf compared to the previous code?
There was a problem hiding this comment.
Should be faster in most cases. PKs and FKs with value converters could be slower as values are converted twice now.
Also simple key values are getting boxed, filed #27756
Perf improvements
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| public virtual object CreateValueIndex(IReadOnlyModificationCommand command, bool fromOriginalValues = false) | ||
| => new ValueIndex<TKey>( |
There was a problem hiding this comment.
This is a heap allocation (is this what #27756 will optimize?)
There was a problem hiding this comment.
No, this is by design. The dictionary where these keys are stored contains values from different constraints, so a value type cannot be used without boxing. In the future we could decide to use a dictionary per constraint, then we could use the generic CreateKeyValue instead. Not filing an issue for this as this was already the case before this PR, benchmarks will show whether this should be changed.
|
|
||
| predecessorCommands.Add(command); | ||
| } | ||
| if (!predecessorsMap.TryGetValue(principalKeyValue, out var predecessorCommands)) |
There was a problem hiding this comment.
I'm probably missing something, but the principal key value is recorded in the predecessor map without any reference to its table; doesn't that mean we're possibly mixing values from different tables when e.g. we happen to insert the same new PK value into two different tables (same for unique value edges)?
I had one thought in this area... Rather than tracking edges between each individual command, for foreign keys it's probably enough to know that all commands which e.g. insert into a dependent table depend on all commands which insert into a principal table. Such a "group dependency" would free us from having to track or even know individual key values, and could simplify and optimize this process. The same approach may be applicable to other edge types (unique constraint, same table).
I realize this is a pretty different approach to the topological sort problem, any thoughts on whether this is worth thinking about?
There was a problem hiding this comment.
I'm probably missing something, but the principal key value is recorded in the predecessor map without any reference to its table; doesn't that mean we're possibly mixing values from different tables when e.g. we happen to insert the same new PK value into two different tables (same for unique value edges)?
As implied in previous comment ValueIndex<> is used as the key here and it contains the EqualityComparer corresponding to the constraint it was created for.
Rather than tracking edges between each individual command, for foreign keys it's probably enough to know that all commands which e.g. insert into a dependent table depend on all commands which insert into a principal table.
Unfortunately most real-life models seem to have FK cycles between tables, so this approach wouldn't work in the general case. However, we could implement it as an optimization for simple models
| /// any release. You should only use it directly in your code with extreme caution and knowing that | ||
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| public sealed class ValueIndex<TKey> |
There was a problem hiding this comment.
Equals and GetHashCode implementations don't follow the record pattern
| /// <summary> | ||
| /// A <see cref="ValueComparer" /> for the provider CLR type values. | ||
| /// </summary> | ||
| public virtual ValueComparer ProviderComparer |
There was a problem hiding this comment.
Also, it may be better to have the provider value comparer on ValueConverter rather than directly on RelationalTypeMapping (and could also help non-relational scenarios).
There was a problem hiding this comment.
This is here to make it easier for providers to use a specific value comparer independently of the value converter, see #27762
For non-relational scenarios we could lift this to CoreTypeMapping.
Yes, there could be a case with table sharing where the PKs use value comparers with different ordering, but this should be rare and it was already the case before this PR. Filed #27761 |
Part of #3170