From 65e48a88011a96fe20bb014ca91e8d3ca8fd2064 Mon Sep 17 00:00:00 2001 From: Nick Tobey Date: Thu, 4 Apr 2024 13:55:46 -0700 Subject: [PATCH 1/3] For AutoIncrement lock modes other than "interleaved", when inserting into a table with an unspecified auto-increment column, acquire a lock when the iterator is created, and release the lock when the iterator is closed. For the in-memory db, this is a no-op because it doesn't have a lock on generating AutoIncrement values. But Dolt has its own AutoIncrement Tracker, which will supply the lock. --- memory/table_editor.go | 5 +++++ sql/fulltext/multi_editor.go | 5 +++++ sql/plan/foreign_key_handler.go | 5 +++++ sql/rowexec/dml.go | 22 ++++++++++++++++++++++ sql/rowexec/insert.go | 4 ++++ sql/session.go | 3 ++- sql/tables.go | 12 ++++++++++++ 7 files changed, 55 insertions(+), 1 deletion(-) diff --git a/memory/table_editor.go b/memory/table_editor.go index 81492b4fc3..6267ddfb8b 100644 --- a/memory/table_editor.go +++ b/memory/table_editor.go @@ -277,6 +277,11 @@ func (t *tableEditor) SetAutoIncrementValue(ctx *sql.Context, val uint64) error return nil } +func (t *tableEditor) AcquireAutoIncrementLock(ctx *sql.Context) (func(), error) { + // TODO: Add concurrency tests for AutoIncrement locking modes. + return func() {}, nil +} + func (t *tableEditor) PreciseMatch() bool { return true } diff --git a/sql/fulltext/multi_editor.go b/sql/fulltext/multi_editor.go index 33e33e5370..444317d7ae 100644 --- a/sql/fulltext/multi_editor.go +++ b/sql/fulltext/multi_editor.go @@ -140,6 +140,11 @@ func (editor MultiTableEditor) SetAutoIncrementValue(ctx *sql.Context, u uint64) return editor.primary.(sql.AutoIncrementSetter).SetAutoIncrementValue(ctx, u) } +func (editor MultiTableEditor) AcquireAutoIncrementLock(ctx *sql.Context) (func(), error) { + // TODO: Add concurrency tests for AutoIncrement locking modes. + return func() {}, nil +} + // Close implements the interface sql.TableEditor. func (editor MultiTableEditor) Close(ctx *sql.Context) error { var err error diff --git a/sql/plan/foreign_key_handler.go b/sql/plan/foreign_key_handler.go index 92ceb3edc2..5aa2a2c3db 100644 --- a/sql/plan/foreign_key_handler.go +++ b/sql/plan/foreign_key_handler.go @@ -30,6 +30,10 @@ type ForeignKeyHandler struct { AllUpdaters []sql.ForeignKeyEditor } +func (n *ForeignKeyHandler) Underlying() sql.Table { + return n.Table +} + var _ sql.Node = (*ForeignKeyHandler)(nil) var _ sql.CollationCoercible = (*ForeignKeyHandler)(nil) var _ sql.Table = (*ForeignKeyHandler)(nil) @@ -41,6 +45,7 @@ var _ sql.TableEditor = (*ForeignKeyHandler)(nil) var _ sql.RowInserter = (*ForeignKeyHandler)(nil) var _ sql.RowUpdater = (*ForeignKeyHandler)(nil) var _ sql.RowDeleter = (*ForeignKeyHandler)(nil) +var _ sql.TableWrapper = (*ForeignKeyHandler)(nil) // Resolved implements the interface sql.Node. func (n *ForeignKeyHandler) Resolved() bool { diff --git a/sql/rowexec/dml.go b/sql/rowexec/dml.go index 58101ca9bd..94dda5c308 100644 --- a/sql/rowexec/dml.go +++ b/sql/rowexec/dml.go @@ -54,7 +54,28 @@ func (b *BaseBuilder) buildInsertInto(ctx *sql.Context, ii *plan.InsertInto, row return nil, err } + var unlocker func() insertExpressions := getInsertExpressions(ii.Source) + if ii.HasUnspecifiedAutoInc { + _, i, _ := sql.SystemVariables.GetGlobal("innodb_autoinc_lock_mode") + lockMode := i.(int64) + if err != nil { + return nil, err + } + // Lock modes "traditional" (0) and "consecutive" (1) require that a single lock is held for the entire iteration. + // Lock mode "interleaved" (2) will acquire the lock only when inserting into the table. + if lockMode != 2 { + autoIncrementable, ok := sql.GetUnderlyingTable(insertable).(sql.AutoIncrementTable) + if !ok { + panic("Auto increment expression on non-AutoIncrement table. This should not be possible.") + } + + unlocker, err = autoIncrementable.AutoIncrementSetter(ctx).AcquireAutoIncrementLock(ctx) + if err != nil { + return nil, err + } + } + } insertIter := &insertIter{ schema: dstSchema, tableNode: ii.Destination, @@ -63,6 +84,7 @@ func (b *BaseBuilder) buildInsertInto(ctx *sql.Context, ii *plan.InsertInto, row updater: updater, rowSource: rowIter, hasAutoAutoIncValue: ii.HasUnspecifiedAutoInc, + unlocker: unlocker, updateExprs: ii.OnDupExprs, insertExprs: insertExpressions, checks: ii.Checks(), diff --git a/sql/rowexec/insert.go b/sql/rowexec/insert.go index f2ad586f1d..3b20d0fa27 100644 --- a/sql/rowexec/insert.go +++ b/sql/rowexec/insert.go @@ -37,6 +37,7 @@ type insertIter struct { rowSource sql.RowIter lastInsertIdUpdated bool hasAutoAutoIncValue bool + unlocker func() ctx *sql.Context insertExprs []sql.Expression updateExprs []sql.Expression @@ -257,6 +258,9 @@ func (i *insertIter) resolveValues(ctx *sql.Context, insertRow sql.Row) error { func (i *insertIter) Close(ctx *sql.Context) error { if !i.closed { i.closed = true + if i.unlocker != nil { + i.unlocker() + } var rsErr, iErr, rErr, uErr error if i.rowSource != nil { rsErr = i.rowSource.Close(ctx) diff --git a/sql/session.go b/sql/session.go index 462d1767b1..deced16fdc 100644 --- a/sql/session.go +++ b/sql/session.go @@ -73,6 +73,7 @@ type Session interface { // SetUserVariable sets the given user variable to the value given for this session, or creates it for this session. SetUserVariable(ctx *Context, varName string, value interface{}, typ Type) error // GetSessionVariable returns this session's value of the system variable with the given name. + // To access global scope, use sql.SystemVariables.GetGlobal instead. GetSessionVariable(ctx *Context, sysVarName string) (interface{}, error) // GetUserVariable returns this session's value of the user variable with the given name, along with its most // appropriate type. @@ -83,7 +84,7 @@ type Session interface { // To access global scope, use sql.StatusVariables instead. GetStatusVariable(ctx *Context, statVarName string) (interface{}, error) // SetStatusVariable sets the value of the status variable with session scope with the given name. - // To access global scope, use sql.StatusVariables instead. + // To access global scope, use sql.StatusVariables.GetGlobal instead. SetStatusVariable(ctx *Context, statVarName string, val interface{}) error // GetAllStatusVariables returns a map of all status variables with session scope and their values. // To access global scope, use sql.StatusVariables instead. diff --git a/sql/tables.go b/sql/tables.go index 18fc8ad9ba..dcc72e1cb8 100644 --- a/sql/tables.go +++ b/sql/tables.go @@ -67,6 +67,13 @@ type TableWrapper interface { Underlying() Table } +func GetUnderlyingTable(t Table) Table { + if tw, ok := t.(TableWrapper); ok { + return GetUnderlyingTable(tw.Underlying()) + } + return t +} + // MutableTableWrapper is a TableWrapper that can change its underlying table. type MutableTableWrapper interface { TableWrapper @@ -332,6 +339,11 @@ type AutoIncrementTable interface { type AutoIncrementSetter interface { // SetAutoIncrementValue sets a new AUTO_INCREMENT value. SetAutoIncrementValue(*Context, uint64) error + + // AcquireAutoIncrementLock acquires (if necessary) an exclusive lock on generating auto-increment values for the underlying table. + // This is called when @@innodb_autoinc_lock_mode is set to 0 (traditional) or 1 (consecutive), in order to guarentee that insert + // operations get a consecutive range of generated ids. The function returns a callback to release the lock. + AcquireAutoIncrementLock(ctx *Context) (func(), error) // Closer finalizes the set operation, persisting the result. Closer } From 325ec529c504d8d9c54490bc50c6a27e6dd0eaa1 Mon Sep 17 00:00:00 2001 From: Nick Tobey Date: Fri, 5 Apr 2024 15:57:32 -0700 Subject: [PATCH 2/3] Allow new options for `innodb_autoinc_lock_mode`. --- sql/variables/system_variables.go | 2 +- sql/variables/system_variables_test.go | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/sql/variables/system_variables.go b/sql/variables/system_variables.go index a71d066ade..8a0875468e 100644 --- a/sql/variables/system_variables.go +++ b/sql/variables/system_variables.go @@ -1030,7 +1030,7 @@ var systemVars = map[string]sql.SystemVariable{ Dynamic: false, SetVarHintApplies: false, // TODO: lower bound should be 0: https://github.com/dolthub/dolt/issues/7634 - Type: types.NewSystemIntType("innodb_autoinc_lock_mode", 2, 2, false), + Type: types.NewSystemIntType("innodb_autoinc_lock_mode", 0, 2, false), Default: int64(2), }, // Row locking is currently not supported. This variable is provided for 3p tools, and we always return the diff --git a/sql/variables/system_variables_test.go b/sql/variables/system_variables_test.go index efd896ab69..a77ddd1bcc 100644 --- a/sql/variables/system_variables_test.go +++ b/sql/variables/system_variables_test.go @@ -61,17 +61,20 @@ func TestInitSystemVars(t *testing.T) { { varName: "innodb_autoinc_lock_mode", varVal: 0, - err: sql.ErrInvalidSystemVariableValue, }, { varName: "innodb_autoinc_lock_mode", varVal: 1, - err: sql.ErrInvalidSystemVariableValue, }, { varName: "innodb_autoinc_lock_mode", varVal: 2, }, + { + varName: "innodb_autoinc_lock_mode", + varVal: 3, + err: sql.ErrInvalidSystemVariableValue, + }, } for _, test := range tests { From 6b7f01b5447cd38378e605b940afe0cfcab266ba Mon Sep 17 00:00:00 2001 From: Nick Tobey Date: Tue, 9 Apr 2024 13:43:37 -0700 Subject: [PATCH 3/3] Respond to PR feedback. --- sql/rowexec/dml.go | 9 +++++---- sql/variables/system_variables.go | 5 ++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/sql/rowexec/dml.go b/sql/rowexec/dml.go index 94dda5c308..d2b199b2ce 100644 --- a/sql/rowexec/dml.go +++ b/sql/rowexec/dml.go @@ -15,6 +15,7 @@ package rowexec import ( + "errors" "fmt" "sync" @@ -58,16 +59,16 @@ func (b *BaseBuilder) buildInsertInto(ctx *sql.Context, ii *plan.InsertInto, row insertExpressions := getInsertExpressions(ii.Source) if ii.HasUnspecifiedAutoInc { _, i, _ := sql.SystemVariables.GetGlobal("innodb_autoinc_lock_mode") - lockMode := i.(int64) - if err != nil { - return nil, err + lockMode, ok := i.(int64) + if !ok { + return nil, errors.New(fmt.Sprintf("unexpected type for innodb_autoinc_lock_mode, expected int64, got %T", i)) } // Lock modes "traditional" (0) and "consecutive" (1) require that a single lock is held for the entire iteration. // Lock mode "interleaved" (2) will acquire the lock only when inserting into the table. if lockMode != 2 { autoIncrementable, ok := sql.GetUnderlyingTable(insertable).(sql.AutoIncrementTable) if !ok { - panic("Auto increment expression on non-AutoIncrement table. This should not be possible.") + return nil, errors.New("auto increment expression on non-AutoIncrement table. This should not be possible") } unlocker, err = autoIncrementable.AutoIncrementSetter(ctx).AcquireAutoIncrementLock(ctx) diff --git a/sql/variables/system_variables.go b/sql/variables/system_variables.go index a059e1b9af..47cdecda55 100644 --- a/sql/variables/system_variables.go +++ b/sql/variables/system_variables.go @@ -1029,9 +1029,8 @@ var systemVars = map[string]sql.SystemVariable{ Scope: sql.GetMysqlScope(sql.SystemVariableScope_Global), Dynamic: false, SetVarHintApplies: false, - // TODO: lower bound should be 0: https://github.com/dolthub/dolt/issues/7634 - Type: types.NewSystemIntType("innodb_autoinc_lock_mode", 0, 2, false), - Default: int64(2), + Type: types.NewSystemIntType("innodb_autoinc_lock_mode", 0, 2, false), + Default: int64(2), }, // Row locking is currently not supported. This variable is provided for 3p tools, and we always return the // Lowest value allowed by MySQL, which is 1. If you attempt to set this value to anything other than 1, errors ensue.