From 1a12b85ea996614540e7d878eca50098a03e6e9f Mon Sep 17 00:00:00 2001 From: Nathan Gabrielson Date: Mon, 19 May 2025 15:24:21 -0700 Subject: [PATCH 1/7] Add session variable for locking warnings --- engine.go | 11 ++++++++++- sql/variables/system_variables.go | 8 ++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/engine.go b/engine.go index 9b7faf4f7a..a994262b81 100644 --- a/engine.go +++ b/engine.go @@ -268,7 +268,7 @@ func clearWarnings(ctx *sql.Context, node sql.Node) { case *plan.Offset, *plan.Limit: // `show warning limit x offset y` is valid, so we need to recurse clearWarnings(ctx, n.Children()[0]) - case plan.ShowWarnings: + case plan.ShowWarnings, *plan.Set: // ShowWarnings should not clear the warnings, but should still reset the warning count. ctx.ClearWarningCount() default: @@ -410,6 +410,15 @@ func (e *Engine) QueryWithBindings(ctx *sql.Context, query string, parsed sqlpar return nil, nil, nil, err } + shouldLock, err := ctx.GetSessionVariable(ctx, "lock_warnings") + if err != nil { + return nil, nil, nil, err + } + if shouldLock == true { + ctx.LockWarnings() + defer ctx.UnlockWarnings() + } + // planbuilding can produce warnings, so we need to preserve them numPrevWarnings := len(ctx.Session.Warnings()) bound, qFlags, err := e.bindQuery(ctx, query, parsed, bindings, binder, qFlags) diff --git a/sql/variables/system_variables.go b/sql/variables/system_variables.go index e515ba6134..41194f5f25 100644 --- a/sql/variables/system_variables.go +++ b/sql/variables/system_variables.go @@ -1253,6 +1253,14 @@ var systemVars = map[string]sql.SystemVariable{ Type: types.NewSystemIntType("lock_wait_timeout", 1, 31536000, false), Default: int64(31536000), }, + "lock_warnings": &sql.MysqlSystemVariable{ + Name: "lock_warnings", + Scope: sql.GetMysqlScope(sql.SystemVariableScope_Both), + Dynamic: true, + SetVarHintApplies: false, + Type: types.NewSystemBoolType("lock_warnings"), + Default: int8(0), + }, "log_bin": &sql.MysqlSystemVariable{ Name: "log_bin", Scope: sql.GetMysqlScope(sql.SystemVariableScope_Persist), From 5d1ea2dce043031ac13b18ed60f3509c0e31a407 Mon Sep 17 00:00:00 2001 From: Nathan Gabrielson Date: Mon, 19 May 2025 16:07:30 -0700 Subject: [PATCH 2/7] Quick fix to if statement logic --- engine.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine.go b/engine.go index a994262b81..1635c531cf 100644 --- a/engine.go +++ b/engine.go @@ -414,7 +414,7 @@ func (e *Engine) QueryWithBindings(ctx *sql.Context, query string, parsed sqlpar if err != nil { return nil, nil, nil, err } - if shouldLock == true { + if shouldLock.(int8) == 1 { ctx.LockWarnings() defer ctx.UnlockWarnings() } From aa19071db712ee7bf901ea4d217bc766966eec27 Mon Sep 17 00:00:00 2001 From: Nathan Gabrielson Date: Tue, 20 May 2025 09:52:30 -0700 Subject: [PATCH 3/7] Tests for @@lock_warnings --- enginetest/queries/variable_queries.go | 30 ++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/enginetest/queries/variable_queries.go b/enginetest/queries/variable_queries.go index 056771d00b..9bfc4a7415 100644 --- a/enginetest/queries/variable_queries.go +++ b/enginetest/queries/variable_queries.go @@ -575,6 +575,36 @@ var VariableQueries = []ScriptTest{ }, }, }, + { + Name: "locked warnings stay after query", + SetUpScript: []string{ + "set @@lock_warnings = 1", + "select 1/0,1/0", + "select 1/1", + }, + Assertions: []ScriptTestAssertion{ + { + Query: "show warnings", + Expected: []sql.Row{ + {"Warning", 1365, "Division by 0"}, + {"Warning", 1365, "Division by 0"}}, + }, + }, + }, + { + Name: "unlocked warnings clear after query", + SetUpScript: []string{ + "set @@lock_warnings = 0", + "select 1/0,1/0", + "select 1/1", + }, + Assertions: []ScriptTestAssertion{ + { + Query: "show warnings", + Expected: []sql.Row{}, + }, + }, + }, //TODO: do not override tables with user-var-like names...but why would you do this?? //{ // Name: "user var table name no conflict", From 93f69b8613fe5df1360067da0fad5ee3d630faeb Mon Sep 17 00:00:00 2001 From: Nathan Gabrielson Date: Tue, 20 May 2025 12:01:28 -0700 Subject: [PATCH 4/7] Fix requested changes --- engine.go | 12 ++--------- enginetest/queries/variable_queries.go | 30 ++++++++++++++++++++++++++ sql/variables/system_variables.go | 11 +++++++++- 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/engine.go b/engine.go index 1635c531cf..bd8c239394 100644 --- a/engine.go +++ b/engine.go @@ -268,7 +268,8 @@ func clearWarnings(ctx *sql.Context, node sql.Node) { case *plan.Offset, *plan.Limit: // `show warning limit x offset y` is valid, so we need to recurse clearWarnings(ctx, n.Children()[0]) - case plan.ShowWarnings, *plan.Set: + case *plan.Set: + case plan.ShowWarnings: // ShowWarnings should not clear the warnings, but should still reset the warning count. ctx.ClearWarningCount() default: @@ -410,15 +411,6 @@ func (e *Engine) QueryWithBindings(ctx *sql.Context, query string, parsed sqlpar return nil, nil, nil, err } - shouldLock, err := ctx.GetSessionVariable(ctx, "lock_warnings") - if err != nil { - return nil, nil, nil, err - } - if shouldLock.(int8) == 1 { - ctx.LockWarnings() - defer ctx.UnlockWarnings() - } - // planbuilding can produce warnings, so we need to preserve them numPrevWarnings := len(ctx.Session.Warnings()) bound, qFlags, err := e.bindQuery(ctx, query, parsed, bindings, binder, qFlags) diff --git a/enginetest/queries/variable_queries.go b/enginetest/queries/variable_queries.go index 9bfc4a7415..ce18c9ccc2 100644 --- a/enginetest/queries/variable_queries.go +++ b/enginetest/queries/variable_queries.go @@ -589,6 +589,20 @@ var VariableQueries = []ScriptTest{ {"Warning", 1365, "Division by 0"}, {"Warning", 1365, "Division by 0"}}, }, + { + Query: "select 1/0", + Expected: []sql.Row{ + {"NULL"}, + }, + }, + { + Query: "show warnings", + Expected: []sql.Row{ + {"Warning", 1365, "Division by 0"}, + {"Warning", 1365, "Division by 0"}, + {"Warning", 1365, "Division by 0"}, + }, + }, }, }, { @@ -605,6 +619,22 @@ var VariableQueries = []ScriptTest{ }, }, }, + { + Name: "warnings persist after locking between queries", + SetUpScript: []string{ + "select 1/0", + "set @@lock_warnings = 1", + "select 1/1", + }, + Assertions: []ScriptTestAssertion{ + { + Query: "show warnings", + Expected: []sql.Row{ + {"Warning", 1365, "Division by 0"}, + }, + }, + }, + }, //TODO: do not override tables with user-var-like names...but why would you do this?? //{ // Name: "user var table name no conflict", diff --git a/sql/variables/system_variables.go b/sql/variables/system_variables.go index 41194f5f25..188d3bc5ec 100644 --- a/sql/variables/system_variables.go +++ b/sql/variables/system_variables.go @@ -1255,11 +1255,20 @@ var systemVars = map[string]sql.SystemVariable{ }, "lock_warnings": &sql.MysqlSystemVariable{ Name: "lock_warnings", - Scope: sql.GetMysqlScope(sql.SystemVariableScope_Both), + Scope: sql.GetMysqlScope(sql.SystemVariableScope_Session), Dynamic: true, SetVarHintApplies: false, Type: types.NewSystemBoolType("lock_warnings"), Default: int8(0), + NotifyChanged: func(ctx *sql.Context, _ sql.SystemVariableScope, value sql.SystemVarValue) error { + switch value.Val.(int8) { + case 0: + ctx.UnlockWarnings() + case 1: + ctx.LockWarnings() + } + return nil + }, }, "log_bin": &sql.MysqlSystemVariable{ Name: "log_bin", From 49e8c3639194aed54c4bcac38f23ad216ab2a5e4 Mon Sep 17 00:00:00 2001 From: Nathan Gabrielson Date: Tue, 20 May 2025 12:42:54 -0700 Subject: [PATCH 5/7] Fix test typo --- enginetest/queries/variable_queries.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enginetest/queries/variable_queries.go b/enginetest/queries/variable_queries.go index ce18c9ccc2..1ccfdaed18 100644 --- a/enginetest/queries/variable_queries.go +++ b/enginetest/queries/variable_queries.go @@ -592,7 +592,7 @@ var VariableQueries = []ScriptTest{ { Query: "select 1/0", Expected: []sql.Row{ - {"NULL"}, + {interface{}(nil)}, }, }, { From 4e4cb457bc033296ceb1d67b27f398fb91dc2256 Mon Sep 17 00:00:00 2001 From: Nathan Gabrielson Date: Tue, 20 May 2025 13:25:24 -0700 Subject: [PATCH 6/7] Comments to explain clearWarnings --- engine.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/engine.go b/engine.go index bd8c239394..714a0adb48 100644 --- a/engine.go +++ b/engine.go @@ -269,6 +269,8 @@ func clearWarnings(ctx *sql.Context, node sql.Node) { // `show warning limit x offset y` is valid, so we need to recurse clearWarnings(ctx, n.Children()[0]) case *plan.Set: + // We want to maintain warnings when setting the warnings_lock variable. + // Set statements also can't produce warnings, so we don't care about clearing them. case plan.ShowWarnings: // ShowWarnings should not clear the warnings, but should still reset the warning count. ctx.ClearWarningCount() From 421019f08e6d07a938afec370ab398d4ea0af9cd Mon Sep 17 00:00:00 2001 From: Nathan Gabrielson Date: Tue, 20 May 2025 14:41:54 -0700 Subject: [PATCH 7/7] use skipresults when appropriate --- enginetest/queries/variable_queries.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/enginetest/queries/variable_queries.go b/enginetest/queries/variable_queries.go index 1ccfdaed18..173be4222a 100644 --- a/enginetest/queries/variable_queries.go +++ b/enginetest/queries/variable_queries.go @@ -590,10 +590,8 @@ var VariableQueries = []ScriptTest{ {"Warning", 1365, "Division by 0"}}, }, { - Query: "select 1/0", - Expected: []sql.Row{ - {interface{}(nil)}, - }, + Query: "select 1/0", + SkipResultsCheck: true, }, { Query: "show warnings",