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.
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
API review changes #33412
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you not able to get rid of forAlter
by refactoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I.e. moving this overload to SqlServer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 1
to 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or did you have something else in mind @AndriySvyryd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 1
to 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.GetOriginalOrCurrentValue
withHasOriginalValue
MigrationsSqlGenerator.SequenceOptions
overload withforAlter
. Move logic to other methods otherwise.IReadOnlySequence.IsCached
.CacheSize
of0
or1
, indicates no caching, all other non-null
values indicate caching