Optional RestartSequenceOperation.StartValue

[dmitry-lipetsk]

MSSQL, PostgreSQL and Firebird support "ALTER SEQUENCE {name} RESTART " without "WITH startValue" section

RestartSequenceOperation.StartValue requires definition of startValue:

efcore/src/EFCore.Relational/Migrations/Operations/RestartSequenceOperation.cs

Lines 27 to 30 in 252ece7

	/// <summary>
	/// The value at which the sequence should re-start, defaulting to 1.
	/// </summary>
	public virtual long StartValue { get; set; } = 1L;

Why do not use Nullable<long> for this property?

[dmihaita]

I intend to contribute a fix for this issue by:

• changing the type of the StartValue property from long to long?
• modify the generators as that it should add " RESTART WITH {StartValue} " only in case StartValue has a value (is not null)
• adapt the affected unit tests.

Should I proceed with the implementation?
Thanks.

[ajcvickers]

@dmihaita Looks good. Go for it!

[roji]

I think there's a conceptual design issue here...

There's no problem with allowing a null start value in RestartSequenceOperation; this represents an operation that occurs at some point in a migration.

However, ISequence itself represents the state of the sequence in the model, at a given point in time; the start value of a sequence is always something - it can never be null. The attempt to work around this with InternalStartValue in #29346 also doesn't work: when I go to the sequence metadata and ask what its start value is, I should get the correct response corresponding to the reality in the database. With #29346 as it is, consider the following:

1. I create a new sequence with start value 5 (migration1)
2. I now change my model and set the start value to null, in order to generate a sequence restart (migration2).
   a. When that migration is executed, my database sequence is back to 5 (as I wanted).
   b. But in my model, _startValue is now null. Reading the StartValue will return DefaultStartValue, which is 1 - that's incorrect.

This can have various problematic consequences; for example, when we implement migration squashing (#2174), if you squash all the migrations together, the original start value of 5 disappears, and you're left with a sequence that has a null starting value (so 1).

At the end of the day, the model cannot contain an operation that needs to occur in the database (such as "reset sequence"); operations can only be a result of comparing two model states (source and destination). So I think the only place where we can allow a sequence restart without a start value is if the user inserts that manually into a scaffolded migration.

@dmihaita let me know if that makes sense - I'll also bring this to a quick team discussion to confirm the above.

[dmihaita]

Thanks for the clarifications, now it is a little clearer (at the least the scope of the ISequence)
I avoided changing IReadOnlySequence.StartValue because this would trigger changes in scaffolding, code generation also.

[roji]

@dmihaita we discussed this, and as written above, we don't think it makes sense to change the model/metadata in any way. In other words, this change should be restricted to RestartSequenceOperation and related APIs, but ISequence and related shouldn't change.

[dmihaita]

Hello @roji, thank you for the update.
Maybe I'm missing something, but as I see it, a RestartSequenceOperation is a direct result of a comparison between two sequences (ISequence). Keeping ISequence as it is, and not changing neither the diff logic, there isn't any way to generate a RestartSequenceOperation (at least for the special case of without "WITH startValue")
I will try to see if I can find another approach. Any hint would be welcomed :)

[roji]

You're right that when the start value is changed, EF generates a RestartSequenceOperation (/cc @bricelam); but that's still an actual model state change (the sequence's start value changed). The same isn't true of "just restart the sequence" - there's no sequence model state change.

I don't think there's anything really to do about this - anything we try to introduce into the model here would again conflate state with operation, and cause bugs. I think it's reasonable to say that users have to manually trigger the restart in the scaffolded migration if they want to do this.

[roji]

@dmihaita is the above clear, and are you planning on updating this PR accordingly?

[dmihaita]

My understanding is that this PR become obsolete, as there is nothing to be done without disrupting other functionalities.
Should I update the documentation, in order to instruct "that users have to manually trigger the restart in the scaffolded migration" ?

[roji]

@dmihaita it's still possible to allow the operation to be used without specifying a start value; EF wouldn't ever generate such a migration operation (as a result of comparing two model states), but users could still specify that operation manually inside migrations. The value here isn't huge, but it's not zero either.

[dmihaita]

In the beginning, sorry for being such a bother.
But I'm missing a point here ... Can you provide a short guidance for a test that will cover how to specify that operation manually?

I got it that this unit test should be removed, as per discussion above:

public override async Task Alter_sequence_restart()
{
    await base.Alter_sequence_restart();
    AssertSql(@"ALTER SEQUENCE [foo] RESTART;");
}

Should I revert the change to SequenceBuilder's method StartsAt also? (I changed the parameter from long to long?).

[roji]

Can you provide a short guidance for a test that will cover how to specify that operation manually?

Since EF shouldn't generate the new RestartSequenceOperation from a diff of two models (as above), the MigrationsTestBase tests aren't suitable for this. We have MigrationsSqlGeneratorTestBase where you can provide a migration operation directly and assert on the generated SQL - that should be suitable for this.

Should I revert the change to SequenceBuilder's method StartsAt also? (I changed the parameter from long to long?)

Yes, I think so. A sequence always has a starting value.

[dmihaita]

I think the scope of this issue was covered with the latest commit.

• Reverted all changes to Sequence and related.
• Adjusted the unit tests (removed the unit test that checked the operation creation through diff, added two unit-tests that cover the query generation based on restart operation definition)

[roji]

A note coming out of implementing this for PG:

DROP SEQUENCE IF EXISTS foo;
CREATE SEQUENCE foo START WITH 10;
SELECT nextval('foo'); -- 10
SELECT nextval('foo'); -- 11
ALTER SEQUENCE foo RESTART WITH 100;
SELECT nextval('foo'); -- 100, actual value changed
SELECT nextval('foo'); -- 101
ALTER SEQUENCE foo RESTART;
SELECT nextval('foo'); -- 10, schema start value did not change
SELECT nextval('foo'); -- 11
ALTER SEQUENCE foo START WITH 100;
SELECT nextval('foo'); -- 12, actual value did not change
ALTER SEQUENCE foo RESTART;
SELECT nextval('foo'); -- 100

In other words, RESTART x changes the current sequence value but does not change its schema "start with" value, whereas STARTS x changes the schema value but not the current sequence.

So for PG, I'll need to do both:

ALTER SEQUENCE foo START WITH x;
ALTER SEQUENCE foo RESTART;

[ajcvickers]

@bricelam What's the breaking change here?

[bricelam]

Hmm, just int to Nullable<int> in a few APIs, but they’re mostly just used by providers or are at least source compatible.