Implement `traditional` auto-increment lock mode hold the lock for the duration of the insert iter. #7704

nicktobey · 2024-04-05T22:56:17Z

This is the dolt half of dolthub/go-mysql-server#2439

This adds support for innodb_autoinc_lock_mode=0 ("traditional"). When this system variable is set, the engine will guarantee that every insert statement generates consecutive IDs for AUTO INCREMENT columns.

This PR also allows the user to set innodb_autoinc_lock_mode=1 ("consecutive"), although the behavior is currently identical to "traditional". This is acceptable because both modes make the same guarantees (that each statement gets consecutive IDs), and the observed behavior is the same in almost all cases.

(The "consecutive" contains an additional optimization: if there is a known upper bound on the number of IDs that must be generated for an insert, under "consecutive" mode the engine will just increment the counter by that upper bound and immediately release the lock. In places where not all of those IDs are actually used, the excess are wasted. This PR does not include that optimization. Thus, with this PR, traditional and consecutive behave the same.)

coffeegoddd · 2024-04-05T23:27:31Z

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`fe4a219`	ok	5937457

version	total_tests
`fe4a219`	5937457

correctness_percentage
100.0

coffeegoddd · 2024-04-06T00:45:14Z

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`ab54c2c`	ok	5937457

version	total_tests
`ab54c2c`	5937457

correctness_percentage
100.0

fulghum

Looks good, a few small comments. I skipped over the changes included from #7699 since those have already been reviewed and you're working on some follow ups there.

Looks like a few CI checks are also failing. Some looked transient, but it doesn't look like the new BATS tests are running well in the Lambda executor and probably still need some work.

My biggest comment is that I think the test code would be easier to understand and easier to extend if they used the sql-server golang multi session test framework we have in Dolt already. Testing this in BATS feels more complicated to me.

We have a few tests that use the sql-server multi session test framework in Dolt already (e.g. for testing multi-session behavior with branch deletion). There are definitely some gaps in that part of our testing framework, but it would be really valuable to improve it so we could use it more easily for more functionality like this in the future.

go/libraries/doltcore/sqle/globalstate/auto_increment_tracker.go

fulghum · 2024-04-09T19:40:27Z

integration-tests/bats/auto_increment_lock_modes.bats

+    run dolt sql -q "INSERT INTO test1 (c0) select v from sequence5bit; SELECT * from timestamps; COMMIT;" &
+    run dolt sql -q "INSERT INTO test2 (c0) select v from sequence5bit; SELECT * from timestamps; COMMIT;"


(minor) Doesn't seem like the COMMITs are needed here, right? Since @@autocommit is enabled? Not a big deal, but might be nice remove them just to simplify and not confuse future code readers into thinking there's manual tx management going on.

Is it interesting to test behavior for concurrent transactions? If so, that's another good reason to use the sql server multi session tests in the Go code.

Autocommit is actually disabled when the server is started with the start_sql_server_with_config helper function, so these commits are necessary.

fulghum · 2024-04-09T19:43:36Z

integration-tests/bats/auto_increment_lock_modes.bats

+#!/usr/bin/env bats
+load $BATS_TEST_DIRNAME/helper/common.bash


Did you get a chance to check out the sql server multi session tests in Dolt's Go code? Those tests are somewhat easier to debug, although since they start up a separate sql server process, it's still not as direct as a non-server test. They also have the advantage of having multiple concurrent sessions running and being able to interleave commands from different sessions. They are a little hacky, but improving them so we could use them more would be pretty valuable.

integration-tests/bats/auto_increment_lock_modes.bats

nicktobey · 2024-04-09T20:53:36Z

In regards to your comments about the server multi session tests: I took a look at them, but (if I understood them correctly) their limitation is that while they allow multiple sessions, the test harness is still only executing one statement at a time. For these tests, I need to test multiple sessions that are attempting to execute a statement concurrently.

I'm sure we could improve them to allow for this kind of concurrency, but in the meantime it was easier to just implement the tests this way.

…ERT statements.

…t asserted it can't.

…te.sh

coffeegoddd · 2024-04-10T17:07:53Z

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`e311f89`	ok	5937457

version	total_tests
`e311f89`	5937457

correctness_percentage
100.0

coffeegoddd · 2024-04-10T17:15:08Z

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`6513fd2`	ok	5937457

version	total_tests
`6513fd2`	5937457

correctness_percentage
100.0

coffeegoddd · 2024-04-10T20:04:36Z

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`e4b3856`	ok	5937457

version	total_tests
`e4b3856`	5937457

correctness_percentage
100.0

fulghum · 2024-04-11T00:04:25Z

In regards to your comments about the server multi session tests: I took a look at them, but (if I understood them correctly) their limitation is that while they allow multiple sessions, the test harness is still only executing one statement at a time. For these tests, I need to test multiple sessions that are attempting to execute a statement concurrently.

I don't think the BATS test gives you any better guarantee about the concurrency of the two sessions here – you're spinning up two processes through the shell, so there's no guarantee that the sessions will even be concurrent depending on how the OS ends up scheduling those processes. If you really need two cores to run the same statement at the same time to test this logic, I don't think this BATS test is the way to get that.

coffeegoddd · 2024-04-11T00:11:27Z

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`5ba740f`	ok	5937457

version	total_tests
`5ba740f`	5937457

correctness_percentage
100.0

…ors to ensure no deadlock occurs.

…te.sh

fulghum · 2024-04-11T20:38:08Z

go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go

+			var row1 sql.Row
+			require.NoError(t, err1)
+			row1, err1 = iter1.Next(ctx)
+			_ = row1


(minor) Could you get rid of the var row1 completely by saying: _, err1 = iter1.Next(ctx) like you're doing for iter2?

fulghum · 2024-04-11T20:43:07Z

go/libraries/doltcore/sqle/enginetest/dolt_engine_test.go

+	err = iter2.Close(ctx)
+	require.NoError(t, err)
+
+	dsess.DSessFromSess(ctx.Session).CommitTransaction(ctx, ctx.GetTransaction())


Do we want these to be executed in the same session?

In a real environment they wouldn't be, but I don't think it matters here. The engine tests have a single session baked in, and figuring out how to disentangle that didn't seem worth it for a single test.

coffeegoddd · 2024-04-11T20:57:27Z

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`f62cfb8`	ok	5937457

version	total_tests
`f62cfb8`	5937457

correctness_percentage
100.0

coffeegoddd · 2024-04-11T21:05:56Z

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`0f5cc28`	ok	5937457

version	total_tests
`0f5cc28`	5937457

correctness_percentage
100.0

…gine: the test manages the engine itself.

coffeegoddd · 2024-04-11T21:54:36Z

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`19c89b3`	ok	5937457

version	total_tests
`19c89b3`	5937457

correctness_percentage
100.0

github-actions · 2024-04-11T22:40:01Z

@coffeegoddd DOLT

test_name	detail	row_cnt	sorted	mysql_time	sql_mult	cli_mult
batching	LOAD DATA	10000	1	0.05	1.4
batching	batch sql	10000	1	0.07	1.86
batching	by line sql	10000	1	0.07	1.86
blob	1 blob	200000	1	0.9	3.88	4.38
blob	2 blobs	200000	1	0.88	4.93	5.24
blob	no blob	200000	1	0.89	2.09	2.21
col type	datetime	200000	1	0.83	2.52	2.75
col type	varchar	200000	1	0.67	2.94	3.03
config width	2 cols	200000	1	0.8	2.35	2.19
config width	32 cols	200000	1	1.82	1.81	2.7
config width	8 cols	200000	1	0.99	2.02	2.3
pk type	float	200000	1	0.89	1.89	2
pk type	int	200000	1	0.79	2.16	2.24
pk type	varchar	200000	1	1.57	1.45	1.42
row count	1.6mm	1600000	1	5.54	2.52	2.61
row count	400k	400000	1	1.46	2.34	2.45
row count	800k	800000	1	2.81	2.46	2.57
secondary index	four index	200000	1	3.56	1.28	1.09
secondary index	no secondary	200000	1	0.91	2.02	2.13
secondary index	one index	200000	1	1.14	2.13	2.16
secondary index	two index	200000	1	1.99	1.55	1.5
sorting	shuffled 1mm	1000000	0	5.02	2.5	2.65
sorting	sorted 1mm	1000000	1	4.96	2.53	2.68

github-actions · 2024-04-11T22:48:17Z

@coffeegoddd DOLT

name	detail	mean_mult
dolt_blame_basic	system table	1.25
dolt_blame_commit_filter	system table	3.3
dolt_commit_ancestors_commit_filter	system table	0.84
dolt_commits_commit_filter	system table	0.94
dolt_diff_log_join_from_commit	system table	2.09
dolt_diff_log_join_to_commit	system table	2.02
dolt_diff_table_from_commit_filter	system table	1.13
dolt_diff_table_to_commit_filter	system table	1.1
dolt_diffs_commit_filter	system table	0.95
dolt_history_commit_filter	system table	1.45
dolt_log_commit_filter	system table	0.91

coffeegoddd added the correctness_approved label Apr 5, 2024

nicktobey force-pushed the nicktobey/autoinclock branch from 99db5af to e0d8534 Compare April 6, 2024 00:12

fulghum approved these changes Apr 9, 2024

View reviewed changes

nicktobey added 10 commits April 10, 2024 09:30

Add mutex map implementation.

2d3bee1

Implement per-table auto increment locking.

807c7c9

Conditionally acquire the auto-increment lock for the entirety of INS…

5a940cd

…ERT statements.

Support concurrent auto-increment lock modes.

a630cf6

Add bats tests for innodb_autoinc_lock_mode

49a3356

innodb_autoinc_lock_mode now supports other values. Remove test tha…

efec700

…t asserted it can't.

Bump GMS version.

4a154ad

Respond to PR feedback.

622f975

Merge branch 'main' into nicktobey/autoinclock

19772ad

[ga-bump-dep] Bump dependency in Dolt by nicktobey

e311f89

nicktobey force-pushed the nicktobey/autoinclock branch from fc77d06 to e311f89 Compare April 10, 2024 16:36

[ga-format-pr] Run go/utils/repofmt/format_repo.sh and go/Godeps/upda…

6513fd2

…te.sh

Prevent potential deadlock in MutexMap implementation.

e4b3856

Merge branch 'main' into nicktobey/autoinclock

5ba740f

nicktobey and others added 3 commits April 11, 2024 13:17

Replace flaky bats test with go test that manually interleaves iterat…

2eb8c75

…ors to ensure no deadlock occurs.

Reduce size of input for bats test to avoid slowdowns on CI.

f62cfb8

[ga-format-pr] Run go/utils/repofmt/format_repo.sh and go/Godeps/upda…

0f5cc28

…te.sh

fulghum reviewed Apr 11, 2024

View reviewed changes

Skip auto_increment_lock_modes.bats when running tests wiht remote-en…

19c89b3

…gine: the test manages the engine itself.

nicktobey merged commit 5d5847f into main Apr 11, 2024
20 checks passed

nicktobey deleted the nicktobey/autoinclock branch April 11, 2024 21:47

BrewTestBot mentioned this pull request Apr 12, 2024

dolt 1.35.9 Homebrew/homebrew-core#168691

Merged

BrewTestBot mentioned this pull request Apr 19, 2024

dolt 1.35.10 Homebrew/homebrew-core#169481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `traditional` auto-increment lock mode hold the lock for the duration of the insert iter. #7704

Implement `traditional` auto-increment lock mode hold the lock for the duration of the insert iter. #7704

nicktobey commented Apr 5, 2024

coffeegoddd commented Apr 5, 2024

coffeegoddd commented Apr 6, 2024

fulghum left a comment •

edited

Loading

fulghum Apr 9, 2024

nicktobey Apr 9, 2024

fulghum Apr 9, 2024

nicktobey commented Apr 9, 2024

coffeegoddd commented Apr 10, 2024

coffeegoddd commented Apr 10, 2024

coffeegoddd commented Apr 10, 2024

fulghum commented Apr 11, 2024

coffeegoddd commented Apr 11, 2024

fulghum Apr 11, 2024

fulghum Apr 11, 2024

nicktobey Apr 11, 2024

coffeegoddd commented Apr 11, 2024

coffeegoddd commented Apr 11, 2024

coffeegoddd commented Apr 11, 2024

github-actions bot commented Apr 11, 2024

github-actions bot commented Apr 11, 2024

		run dolt sql -q "INSERT INTO test1 (c0) select v from sequence5bit; SELECT * from timestamps; COMMIT;" &
		run dolt sql -q "INSERT INTO test2 (c0) select v from sequence5bit; SELECT * from timestamps; COMMIT;"

		#!/usr/bin/env bats
		load $BATS_TEST_DIRNAME/helper/common.bash

Implement traditional auto-increment lock mode hold the lock for the duration of the insert iter. #7704

Implement traditional auto-increment lock mode hold the lock for the duration of the insert iter. #7704

Conversation

nicktobey commented Apr 5, 2024

coffeegoddd commented Apr 5, 2024

coffeegoddd commented Apr 6, 2024

fulghum left a comment • edited Loading

Choose a reason for hiding this comment

fulghum Apr 9, 2024

Choose a reason for hiding this comment

nicktobey Apr 9, 2024

Choose a reason for hiding this comment

fulghum Apr 9, 2024

Choose a reason for hiding this comment

nicktobey commented Apr 9, 2024

coffeegoddd commented Apr 10, 2024

coffeegoddd commented Apr 10, 2024

coffeegoddd commented Apr 10, 2024

fulghum commented Apr 11, 2024

coffeegoddd commented Apr 11, 2024

fulghum Apr 11, 2024

Choose a reason for hiding this comment

fulghum Apr 11, 2024

Choose a reason for hiding this comment

nicktobey Apr 11, 2024

Choose a reason for hiding this comment

coffeegoddd commented Apr 11, 2024

coffeegoddd commented Apr 11, 2024

coffeegoddd commented Apr 11, 2024

github-actions bot commented Apr 11, 2024

github-actions bot commented Apr 11, 2024

Implement `traditional` auto-increment lock mode hold the lock for the duration of the insert iter. #7704

Implement `traditional` auto-increment lock mode hold the lock for the duration of the insert iter. #7704

fulghum left a comment •

edited

Loading