API review changes #33412
API review changes #33412
Conversation
- Replace `IUpdateEntry.GetOriginalOrCurrentValue` with `HasOriginalValue` - Review whether we really need `MigrationsSqlGenerator.SequenceOptions` overload with `forAlter`. Move logic to other methods otherwise. - Remove `IReadOnlySequence.IsCached`. `CacheSize` of `0` or `1`, indicates no caching, all other non-`null` values indicate caching
| { | ||
| builder | ||
| .Append(" NO CACHE"); | ||
| } | ||
| } | ||
| else if (forAlter) |
There was a problem hiding this comment.
Were you not able to get rid of forAlter by refactoring?
There was a problem hiding this comment.
I.e. moving this overload to SqlServer
There was a problem hiding this comment.
Not if we want the behavior for Alter to be different from the behavior when creating. That is, don't specify anything if nothing is set when creating, but do when altering because we don't know what we might be changing it from.
There was a problem hiding this comment.
I'm not 100% sure what we said we should do, but here's a quick database comparison:
- In PostgreSQL things are pretty simple: CREATE SEQUENCE allows specifying CACHE, but if you do, you must specify a number (how many to cache). Otherwise, it's like a non-caching sequence, which is the same as
CACHE 1) (docs). ALTER SEQUENCE is similar: if you specify CACHE, you must specify by how much. If you don't specify CACHE, it isn't change (so remains whatever it was). - MySQL doesn't support sequences, but MariaDB does (docs); the default is 1000 there, and disabling caching is done via
NOCACHE(notNO CACHE). - Oracle (docs) has NOCACHE, and CACHE requires at least 2 as argument (so one cannot use
CACHE 1to disable caching as in PG). Seems to be the same for ALTER SEQUENCE (docs). - SQLite doesn't support caching.
So the implementation currently seems to be a bit SQL Server-specific, but there's not going to be a thing that works across all databases. I'd make sure it's at least easy for providers to specify the behavior (may be splitting the caching fragment to its own method, so providers don't have to override the entire SequenceOptions?).
BTW I'm noticing that once any sequence option changed, all sequence options are set. It seems like it would be better to compare each options against the old sequence definition (I think we have that?), to avoid e.g. setting the cache size (and everything else) when only the MaxValue changed.
There was a problem hiding this comment.
Or did you have something else in mind @AndriySvyryd?
There was a problem hiding this comment.
We can keep the signature, but move the cache-related implementation to SqlServer. My assumption is that the behavior of explicitly setting other sequence options is only needed for SqlServer when changing the caching options, we might need to add some more alter tests and see whether the approach Shay suggested works.
| .Append(" CACHE ") | ||
| .Append(intTypeMapping.GenerateSqlLiteral(operation.CacheSize.Value)); | ||
| var cacheSize = operation.CacheSize; | ||
| if (cacheSize != 1 && cacheSize != 0) |
There was a problem hiding this comment.
| if (cacheSize != 1 && cacheSize != 0) | |
| if (cacheSize is not 0 and not 1) |
| { | ||
| builder | ||
| .Append(" NO CACHE"); | ||
| } | ||
| } | ||
| else if (forAlter) |
There was a problem hiding this comment.
I'm not 100% sure what we said we should do, but here's a quick database comparison:
- In PostgreSQL things are pretty simple: CREATE SEQUENCE allows specifying CACHE, but if you do, you must specify a number (how many to cache). Otherwise, it's like a non-caching sequence, which is the same as
CACHE 1) (docs). ALTER SEQUENCE is similar: if you specify CACHE, you must specify by how much. If you don't specify CACHE, it isn't change (so remains whatever it was). - MySQL doesn't support sequences, but MariaDB does (docs); the default is 1000 there, and disabling caching is done via
NOCACHE(notNO CACHE). - Oracle (docs) has NOCACHE, and CACHE requires at least 2 as argument (so one cannot use
CACHE 1to disable caching as in PG). Seems to be the same for ALTER SEQUENCE (docs). - SQLite doesn't support caching.
So the implementation currently seems to be a bit SQL Server-specific, but there's not going to be a thing that works across all databases. I'd make sure it's at least easy for providers to specify the behavior (may be splitting the caching fragment to its own method, so providers don't have to override the entire SequenceOptions?).
BTW I'm noticing that once any sequence option changed, all sequence options are set. It seems like it would be better to compare each options against the old sequence definition (I think we have that?), to avoid e.g. setting the cache size (and everything else) when only the MaxValue changed.
| { | ||
| builder | ||
| .Append(" NO CACHE"); | ||
| } | ||
| } | ||
| else if (forAlter) |
There was a problem hiding this comment.
Or did you have something else in mind @AndriySvyryd?
|
I think we're spending way too much time on this. I'll give it one more day to get to some agreement on the behavior we want, otherwise I'll back this change out and we can put it back on the backlog. |
Part of #33220
IUpdateEntry.GetOriginalOrCurrentValuewithHasOriginalValueMigrationsSqlGenerator.SequenceOptionsoverload withforAlter. Move logic to other methods otherwise.IReadOnlySequence.IsCached.CacheSizeof0or1, indicates no caching, all other non-nullvalues indicate caching