WIP: add optional flag to enable multi shard autocommit by default #17635

demmer · 2025-01-27T22:35:16Z

Description

Note this is a RFC Draft PR -- soliciting feedback on the idea / proposed implementation.

Adds a new vtgate flag --default-multi-shard-autocommit which, as the name implies, opts the query engine into using multi-shard autocommit semantics by default, even if the plan does not contain the query directive MULTI_SHARD_AUTOCOMMIT.

The aim of this change is to help protect Slack or other large scale Vitess adopters that use autocommit from inadvertently expensive extra round trips on scatter DMLs.

As a general rule, Slack avoids scatter DMLs, but on occasion when they are needed, prefer the semantics of the MULTI_SHARD_AUTOCOMMIT=1 query directive (in which each shard executes its own autocommit DML) as opposed to the default behavior (in which each shard runs the DML in a transaction and vtgate issues a second round trip to commit).

We generally apply this as a directive in the application code, but that doesn't prevent mistakes where developers can inadvertently forget to apply the directive.

This PR adds an option to change the default behavior so that this is opted in by default. It should not affect any semantics on explicit transactions, nor should it affect any deployments that don't enable the feature.

Related Issue(s)

None.

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

Adds a new vtgate flag --default-multi-shard-autocommit which, as the name implies, opts the query engine into using multi-shard autocommit semantics by default, even if the plan does not contain the query directive MULTI_SHARD_AUTOCOMMIT. Signed-off-by: Michael Demmer <[email protected]>

vitess-bot · 2025-01-27T22:35:19Z

codecov · 2025-01-27T22:58:43Z

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 67.69%. Comparing base (de33a39) to head (34774af).
Report is 229 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vtgate/vtgate.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17635      +/-   ##
==========================================
+ Coverage   67.68%   67.69%   +0.01%     
==========================================
  Files        1586     1586              
  Lines      255647   255654       +7     
==========================================
+ Hits       173034   173075      +41     
+ Misses      82613    82579      -34

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

harshit-gangal · 2025-01-28T10:11:01Z

How about we use transaction_mode for this? Currently it can be single, multi, twopc.

demmer · 2025-01-28T15:17:36Z

Great question -- I considered that as well and if that's what we all think would be a better way to do this, I could try it out and see how it goes. That said, I opted for a separate option for a couple reasons: 1. Conceptually, a new "MULTI_AUTOCOMMIT_ALLOWED" transaction mode would be basically the same as MULTI just with this one tweak. So, the change seemed more like the `--no-scatter` option which was added a while back in that it affects a particular behavior of the vtgate rather than defining a whole new "mode". 2. Pragmatically, doing a new transaction mode would be a more involved patch to plumb down into all the right places and would involve a lot more boilerplate changes to end up in the 2-3 places that we actually have to change the executor.

github-actions · 2025-02-28T01:33:31Z

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

Push additional commits to the associated branch.
Remove the stale label.
Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

demmer · 2025-03-05T23:25:14Z

@harshit-gangal @deepthi This PR sat kinda idle. Is this something that we feel ok about moving forward?

I'm happy to work on the test conflicts if so.

harshit-gangal · 2025-03-17T05:40:33Z

go/vt/vtgate/engine/dml.go


 func (dml *DML) execMultiShard(ctx context.Context, primitive Primitive, vcursor VCursor, rss []*srvtopo.ResolvedShard, queries []*querypb.BoundQuery) (*sqltypes.Result, error) {
-	autocommit := (len(rss) == 1 || dml.MultiShardAutocommit) && vcursor.AutocommitApproval()
+	autocommit := (len(rss) == 1 || vcursor.DefaultMultiShardAutocommit() || dml.MultiShardAutocommit) && vcursor.AutocommitApproval()


dml.MultiShardAutocommit should be authoritative over the default, so if we want to override the default let's say true with a false we have to consider that.

We would need to store like a enum or a pointer bool in the plan to know if this is set or not.

harshit-gangal · 2025-03-17T05:47:57Z

go/vt/vtgate/executor.go

+
+		// DefaultMultiShardAutocommit will opt into autocommit semantics even for multi shard DMLs
+		DefaultMultiShardAutocommit bool


nit: This is not used by executor, it is only passed to vcusor. We can set on the vcursor config directly.

harshit-gangal · 2025-03-17T05:49:40Z

@harshit-gangal @deepthi This PR sat kinda idle. Is this something that we feel ok about moving forward?

I'm happy to work on the test conflicts if so.

@demmer Sorry for the delay. I have reviewed the code now.

github-actions · 2025-05-04T01:34:21Z

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

Push additional commits to the associated branch.
Remove the stale label.
Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

github-actions · 2025-05-12T01:35:15Z

This PR was closed because it has been stale for 7 days with no activity.

github-actions bot added this to the v22.0.0 milestone Jan 27, 2025

github-actions bot added Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. and removed Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. labels Feb 28, 2025

harshit-gangal reviewed Mar 17, 2025

View reviewed changes

frouioui modified the milestones: v22.0.0, v23.0.0 Apr 1, 2025

github-actions bot added the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label May 4, 2025

github-actions bot closed this May 12, 2025

tanjinx mentioned this pull request Aug 5, 2025

add optional flag to enable multi shard autocommit by default slackhq/vitess#681

Merged

5 tasks

systay mentioned this pull request Aug 29, 2025

tooling: changelog automatic reporting #18600

Merged


		// DefaultMultiShardAutocommit will opt into autocommit semantics even for multi shard DMLs
		DefaultMultiShardAutocommit bool

WIP: add optional flag to enable multi shard autocommit by default #17635

WIP: add optional flag to enable multi shard autocommit by default #17635

Uh oh!

Conversation

demmer commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Checklist

Deployment Notes

Uh oh!

vitess-bot bot commented Jan 27, 2025

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

Uh oh!

codecov bot commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

harshit-gangal commented Jan 28, 2025

Uh oh!

demmer commented Jan 28, 2025 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 28, 2025

Uh oh!

demmer commented Mar 5, 2025

Uh oh!

harshit-gangal Mar 17, 2025

Choose a reason for hiding this comment

Uh oh!

harshit-gangal Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harshit-gangal Mar 17, 2025

Choose a reason for hiding this comment

Uh oh!

harshit-gangal commented Mar 17, 2025

Uh oh!

github-actions bot commented May 4, 2025

Uh oh!

github-actions bot commented May 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

demmer commented Jan 27, 2025 •

edited

Loading

codecov bot commented Jan 27, 2025 •

edited

Loading

demmer commented Jan 28, 2025 via email •

edited

Loading

harshit-gangal Mar 17, 2025 •

edited

Loading