Online DDL: migration with same context and SQL now considered duplicate and silently marked as complete

[shlomi-noach]

Description

As reminder, a migration has both a UUID (unique to the migration) and context. The context is implicitly set to be unique in a VTGate session, or is (optionally) provided via vtctl ApplySchema -request_context.

Up till now the migration context was purely declarative and had no logical role. One could submit it, one could list it, one could SHOW VITESS_MIGRATIONS LIKE 'the-context' but that's it.

This PR offers a form of implicit idempotency based on the context, adding actual logic to the use of context.

If two migrations are submitted with same context and same SQL/DDL, then they are considered to be duplicates. When the Online DDL executor schedules a migration it checks:

• if the migration has an explicit context, and
• there is a previous migration with exact same context and SQL, and the migration is complete
• then we implicitly mark the scheduled migration as complete and we do not actually run it.

the idea is that the user is requested to provide a unique context for migrations. Take the following vtctl command:

$ vtctl ApplySchema -request_context 1111-2222-3333 -sql "alter table t 1add column i int not null default 0; alter table t2 drop column c"

It's the user's choice to provide the context 1111-2222-3333 and it's the users responsibility to make it unique.

In the above example the user submits two queries. It is possible for vtctl to crash midway, leaving the user in confusion: were both migrations submitted? Was any of them submitted? Was just one of them submitted?

The user could invoke SHOW VITESS_MIGRATIONS LIKE '1111-2222-3333' and compare the result set with the list of migrations the user provided via -sql. But we feel we can automate that better in Vitess.

As of this PR, if the user again submits same ApplySchema with same context and same SQL:

$ vtctl ApplySchema -request_context 1111-2222-3333 -sql "alter table t 1add column i int not null default 0; alter table t2 drop column c"

Then Vitess identifies that either or both of these migrations are duplicates. If any of these duplicates is already marked as complete, then the "new" migrations can be skipped.
Vitess does generate new UUIDs for the new migrations, but, in case of duplication, they will not execute, and very quickly be marked as complete.
Of course, if the "old" duplicate was failed, then the new migration is actually executed.

Related Issue(s)

• Tracking: Online DDL: tracking issue #6926
• Similar: Online DDL: support for semi-idempotent migration via -combine-duplicate-ddl flag #8209

Checklist

• Should this PR be backported?
• Tests were added or are not required
• Documentation was added or is not required

cc @mscoutermarsh @deepthi @mavenraven

[mavenraven]

@shlomi-noach Awesome, thank you so much! I think this will cover everything.

edit: Re: Vitess does generate new UUIDs for the new migrations, but, in case of duplication, they will not execute, and very quickly be marked as complete. How feasible would it be for not make new migrations at all?

[shlomi-noach]

How feasible would it be for not make new migrations at all?

This is doable This is quite complicated. Consider that vtclt ApplySchema could provide 5 statements, and only get back 2 UUIDs. That's quite confusing; which 2 UUID are those?

One might think we can instead return the UUIDs of the existing migrations. However, UUIDs are generated way before the migration is sent to the tablets: consider how there could be multiple shards for a single migration; all shards must use the same migration UUID.

[shlomi-noach]

s/This is doable/This is quite complicated/g

[mavenraven]

How feasible would it be for not make new migrations at all?

This is doable This is quite complicated. Consider that vtclt ApplySchema could provide 5 statements, and only get back 2 UUIDs. That's quite confusing; which 2 UUID are those?

One might think we can instead return the UUIDs of the existing migrations. However, UUIDs are generated way before the migration is sent to the tablets: consider how there could be multiple shards for a single migration; all shards must use the same migration UUID.

Ok, actually let me rephrase. When we display the progress of the migrations of a given context, it would be ideal if we only showed the "real" ones, otherwise it'll be super confusing for a user. How would we filter out the extra ones? If there's a solution to this, then I'm happy.

[shlomi-noach]

When we display the progress of the migrations of a given context, it would be ideal if we only showed the "real" ones, otherwise it'll be super confusing for a user.

Got it. Right, in the current suggested method the new migration will have a binary "not yet started" then suddenly "I''m all done" status/progress, and not show the actual progress of the running predecessor duplicate. Let me think about this some more.

[shlomi-noach]

After some internal discussions about how this might be used, it looks like we're on the right path. This PR is good to be reviewed.

[mavenraven]

Suggested change

	// ### Teh following tests are not strictly 'declarative' but are best served under this endtoend test
	// ### The following tests are not strictly 'declarative' but are best served under this endtoend test

[deepthi]

LGTM

[shlomi-noach]

I've temporarily converted to draft again just because we had another internal discussion about the use cases. I think the PR is good as it is, and worth merging irrespective, but I just want to think a bit more to see we haven't missed anything.

[shlomi-noach]

(conflict resolution delayed a bit due to new linter's errors)

[shlomi-noach]

OK I'm happy with the PR as-is. It does not solve the entire discussion around idempotency but it's good basis. Merging.

[vkovacik]

@shlomi-noach
Do you plan to add support for parameter request_context provided via vtgate session variable?
Vitess already supports executing online DDL via vtgate using @@ddl_strategy='online' but this PR does not allow to pass @@request_context session variable.

Thanks for this PR, very useful.

[shlomi-noach]

@vkovacik I wasn't planning to do so. Right now if you run an online DDL from vtgate, the value request_context is your session identifier (ie, different session - different context).

I don't have a solid idea/suggestion on how to do it in vtgate, I'm open to ideas. Should we introduce a new @@request_context variable and fall back to the session ID if empty?

[vkovacik]

I would suggest that all ddl related vtgate session variables share the same prefix. I think @@ddl_request_context would make sense to avoid ambiguity.

…

On Sat., Jan. 22, 2022, 8:58 p.m. Shlomi Noach ***@***.***> wrote: @vkovacik <https://github.com/vkovacik> I wasn't planning to do so. Right now if you run an online DDL from vtgate, the value request_context is your session identifier (ie, different session - different context). I don't have a solid idea/suggestion on how to do it in vtgate, I'm open to ideas. Should we introduce a new @@request_context variable and fall back to the session ID if empty? — Reply to this email directly, view it on GitHub <#9107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AP262OOUAOKXUYFTRINZR6TUXODGRANCNFSM5G4VC7YQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>