From 92e53fd1225fe74f490c3f33d686215eccda77e8 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 6 Dec 2023 13:50:55 +0530 Subject: [PATCH 1/9] add foreign key as part of set statement when reserved connection is needed Signed-off-by: Harshit Gangal --- go/test/endtoend/vtgate/foreignkey/fk_test.go | 28 +++++++++++++++++++ go/vt/sysvars/sysvars.go | 7 ++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/go/test/endtoend/vtgate/foreignkey/fk_test.go b/go/test/endtoend/vtgate/foreignkey/fk_test.go index 101cd313e21..c3f1d9dc29c 100644 --- a/go/test/endtoend/vtgate/foreignkey/fk_test.go +++ b/go/test/endtoend/vtgate/foreignkey/fk_test.go @@ -1207,3 +1207,31 @@ func TestReplaceWithFK(t *testing.T) { utils.AssertMatches(t, conn, `select * from u_t1`, `[[INT64(1) INT64(1)] [INT64(2) INT64(2)]]`) utils.AssertMatches(t, conn, `select * from u_t2`, `[[INT64(1) NULL] [INT64(2) NULL]]`) } + +// TestDDLFk tests that table is created with fk constraint when foreign_key_checks is off. +func TestDDLFk(t *testing.T) { + mcmp, closer := start(t) + defer closer() + + utils.Exec(t, mcmp.VtConn, `use uks`) + + createTableDDLTemp1 := ` +create table temp1(id bigint auto_increment primary key, col varchar(20) not null, +foreign key (col) references temp2(col)) +` + mcmp.Exec(`set foreign_key_checks = off`) + // should be able to create `temp1` table without a `temp2` + mcmp.Exec(createTableDDLTemp1) + + createTableDDLTemp2 := ` +create table temp2(id bigint auto_increment primary key, col varchar(20) not null, key (col)) +` + // now create `temp2` + mcmp.Exec(createTableDDLTemp2) + + // inserting some data with fk constraints on. + mcmp.Exec(`set foreign_key_checks = on`) + mcmp.Exec(`insert into temp2(col) values('a'), ('b'), ('c') `) + mcmp.Exec(`insert into temp1(col) values('a') `) + mcmp.ExecAllowAndCompareError(`insert into temp1(col) values('d') `) +} diff --git a/go/vt/sysvars/sysvars.go b/go/vt/sysvars/sysvars.go index 945a60ae965..44f8586bcc6 100644 --- a/go/vt/sysvars/sysvars.go +++ b/go/vt/sysvars/sysvars.go @@ -40,6 +40,8 @@ type SystemVariable struct { SupportSetVar bool + IncludeInSet bool + Case StorageCase } @@ -71,7 +73,7 @@ var ( TxReadOnly = SystemVariable{Name: "tx_read_only", IsBoolean: true, Default: off} Workload = SystemVariable{Name: "workload", IdentifierAsString: true} QueryTimeout = SystemVariable{Name: "query_timeout"} - ForeignKeyChecks = SystemVariable{Name: "foreign_key_checks", IsBoolean: true, SupportSetVar: true} + ForeignKeyChecks = SystemVariable{Name: "foreign_key_checks", IsBoolean: true, SupportSetVar: true, IncludeInSet: true} // Online DDL DDLStrategy = SystemVariable{Name: "ddl_strategy", IdentifierAsString: true} @@ -295,6 +297,9 @@ func IsVitessAware(sysv string) bool { vitessAwareInit.Do(func() { vitessAwareVariableNames = make(map[string]struct{}, len(VitessAware)) for _, v := range VitessAware { + if v.IncludeInSet { + continue + } vitessAwareVariableNames[v.Name] = struct{}{} } }) From fe88546d65cf2b582ff0f0c70ad8ae56d466cc48 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 6 Dec 2023 14:25:11 +0530 Subject: [PATCH 2/9] fix: add fk to set satement at part of prequeries Signed-off-by: Harshit Gangal --- go/vt/sysvars/sysvars.go | 7 +------ go/vt/vtgate/safe_session.go | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/go/vt/sysvars/sysvars.go b/go/vt/sysvars/sysvars.go index 44f8586bcc6..945a60ae965 100644 --- a/go/vt/sysvars/sysvars.go +++ b/go/vt/sysvars/sysvars.go @@ -40,8 +40,6 @@ type SystemVariable struct { SupportSetVar bool - IncludeInSet bool - Case StorageCase } @@ -73,7 +71,7 @@ var ( TxReadOnly = SystemVariable{Name: "tx_read_only", IsBoolean: true, Default: off} Workload = SystemVariable{Name: "workload", IdentifierAsString: true} QueryTimeout = SystemVariable{Name: "query_timeout"} - ForeignKeyChecks = SystemVariable{Name: "foreign_key_checks", IsBoolean: true, SupportSetVar: true, IncludeInSet: true} + ForeignKeyChecks = SystemVariable{Name: "foreign_key_checks", IsBoolean: true, SupportSetVar: true} // Online DDL DDLStrategy = SystemVariable{Name: "ddl_strategy", IdentifierAsString: true} @@ -297,9 +295,6 @@ func IsVitessAware(sysv string) bool { vitessAwareInit.Do(func() { vitessAwareVariableNames = make(map[string]struct{}, len(VitessAware)) for _, v := range VitessAware { - if v.IncludeInSet { - continue - } vitessAwareVariableNames[v.Name] = struct{}{} } }) diff --git a/go/vt/vtgate/safe_session.go b/go/vt/vtgate/safe_session.go index 60d99ab1952..b9498779457 100644 --- a/go/vt/vtgate/safe_session.go +++ b/go/vt/vtgate/safe_session.go @@ -571,6 +571,20 @@ func (session *SafeSession) TimeZone() *time.Location { loc, _ := datetime.ParseTimeZone(tz) return loc } +func (session *SafeSession) ForeingKeyChecks() bool { + session.mu.Lock() + fkVal, ok := session.SystemVariables["foreign_key_checks"] + session.mu.Unlock() + + if !ok { + return true + } + switch strings.ToLower(fkVal) { + case "off", "0": + return false + } + return true +} // SetOptions sets the options func (session *SafeSession) SetOptions(options *querypb.ExecuteOptions) { @@ -610,6 +624,12 @@ func (session *SafeSession) SetPreQueries() []string { sysVars[k] = v }) + if session.ForeingKeyChecks() { + k := "foreign_key_checks" + keys = append(keys, k) + sysVars[k] = "1" + } + // if not system variables to set, return if len(keys) == 0 { return nil From f2cbc0222a44a2c1414a346818fe317f1e91dc66 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 6 Dec 2023 14:34:50 +0530 Subject: [PATCH 3/9] fix: add fk checks only when set Signed-off-by: Harshit Gangal --- go/vt/vtgate/safe_session.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/go/vt/vtgate/safe_session.go b/go/vt/vtgate/safe_session.go index b9498779457..257ff21ce4b 100644 --- a/go/vt/vtgate/safe_session.go +++ b/go/vt/vtgate/safe_session.go @@ -571,19 +571,21 @@ func (session *SafeSession) TimeZone() *time.Location { loc, _ := datetime.ParseTimeZone(tz) return loc } -func (session *SafeSession) ForeingKeyChecks() bool { +func (session *SafeSession) ForeignKeyChecks() *string { session.mu.Lock() fkVal, ok := session.SystemVariables["foreign_key_checks"] session.mu.Unlock() if !ok { - return true + return nil } + fkCheck := "0" switch strings.ToLower(fkVal) { case "off", "0": - return false + return &fkCheck } - return true + fkCheck = "1" + return &fkCheck } // SetOptions sets the options @@ -624,10 +626,11 @@ func (session *SafeSession) SetPreQueries() []string { sysVars[k] = v }) - if session.ForeingKeyChecks() { + fkVal := session.ForeignKeyChecks() + if fkVal != nil { k := "foreign_key_checks" keys = append(keys, k) - sysVars[k] = "1" + sysVars[k] = *fkVal } // if not system variables to set, return From cfaa87ada614f12eecaf82ee5dfe7d62ef445871 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 6 Dec 2023 16:53:34 +0530 Subject: [PATCH 4/9] fix: HasSystemVariables should also look for foreign_key_checks on session Signed-off-by: Harshit Gangal --- go/vt/vtgate/safe_session.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/go/vt/vtgate/safe_session.go b/go/vt/vtgate/safe_session.go index 257ff21ce4b..5479f69b435 100644 --- a/go/vt/vtgate/safe_session.go +++ b/go/vt/vtgate/safe_session.go @@ -557,7 +557,7 @@ func (session *SafeSession) HasSystemVariables() (found bool) { session.GetSystemVariables(func(_ string, _ string) { found = true }) - return + return found || session.ForeignKeyChecks() != nil } func (session *SafeSession) TimeZone() *time.Location { @@ -579,13 +579,15 @@ func (session *SafeSession) ForeignKeyChecks() *string { if !ok { return nil } - fkCheck := "0" switch strings.ToLower(fkVal) { case "off", "0": + fkCheck := "0" + return &fkCheck + case "on", "1": + fkCheck := "1" return &fkCheck } - fkCheck = "1" - return &fkCheck + return nil } // SetOptions sets the options From bd1343a4414e2f4328f1ce1bf6095776169356c6 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 6 Dec 2023 18:44:36 +0530 Subject: [PATCH 5/9] fix: replace (insert, delete) - delete to carry the comments from replace statement, make foreign_key_checks as reserved connection variable Signed-off-by: Harshit Gangal --- go/vt/sqlparser/ast_rewriting.go | 2 +- go/vt/sqlparser/ast_rewriting_test.go | 2 +- go/vt/sqlparser/comments.go | 2 +- go/vt/sqlparser/comments_test.go | 16 +++++------ go/vt/sysvars/sysvars.go | 5 ++-- go/vt/vtgate/engine/set.go | 2 -- go/vt/vtgate/planbuilder/operators/insert.go | 1 + go/vt/vtgate/planbuilder/operators/update.go | 4 +-- go/vt/vtgate/planbuilder/update.go | 2 +- go/vt/vtgate/safe_session.go | 29 +++++++++----------- go/vt/vtgate/vcursor_impl.go | 19 ++++--------- 11 files changed, 36 insertions(+), 48 deletions(-) diff --git a/go/vt/sqlparser/ast_rewriting.go b/go/vt/sqlparser/ast_rewriting.go index e20a5b80f70..659383ec6d6 100644 --- a/go/vt/sqlparser/ast_rewriting.go +++ b/go/vt/sqlparser/ast_rewriting.go @@ -191,7 +191,7 @@ func (er *astRewriter) rewriteUp(cursor *Cursor) bool { supportOptimizerHint.SetComments(newComments) } if er.fkChecksState != nil { - newComments := supportOptimizerHint.GetParsedComments().SetMySQLSetVarValue(sysvars.ForeignKeyChecks.Name, FkChecksStateString(er.fkChecksState)) + newComments := supportOptimizerHint.GetParsedComments().SetMySQLSetVarValue(sysvars.ForeignKeyChecks, FkChecksStateString(er.fkChecksState)) supportOptimizerHint.SetComments(newComments) } } diff --git a/go/vt/sqlparser/ast_rewriting_test.go b/go/vt/sqlparser/ast_rewriting_test.go index c8bc0fdbef9..86bab314dd8 100644 --- a/go/vt/sqlparser/ast_rewriting_test.go +++ b/go/vt/sqlparser/ast_rewriting_test.go @@ -365,7 +365,7 @@ func TestRewrites(in *testing.T) { assert.Equal(tc.rowCount, result.NeedsFuncResult(RowCountName), "should need row count") assert.Equal(tc.udv, len(result.NeedUserDefinedVariables), "count of user defined variables") assert.Equal(tc.autocommit, result.NeedsSysVar(sysvars.Autocommit.Name), "should need :__vtautocommit") - assert.Equal(tc.foreignKeyChecks, result.NeedsSysVar(sysvars.ForeignKeyChecks.Name), "should need :__vtforeignKeyChecks") + assert.Equal(tc.foreignKeyChecks, result.NeedsSysVar(sysvars.ForeignKeyChecks), "should need :__vtforeignKeyChecks") assert.Equal(tc.clientFoundRows, result.NeedsSysVar(sysvars.ClientFoundRows.Name), "should need :__vtclientFoundRows") assert.Equal(tc.skipQueryPlanCache, result.NeedsSysVar(sysvars.SkipQueryPlanCache.Name), "should need :__vtskipQueryPlanCache") assert.Equal(tc.sqlSelectLimit, result.NeedsSysVar(sysvars.SQLSelectLimit.Name), "should need :__vtsqlSelectLimit") diff --git a/go/vt/sqlparser/comments.go b/go/vt/sqlparser/comments.go index fbc1e17ba2f..780f1e67594 100644 --- a/go/vt/sqlparser/comments.go +++ b/go/vt/sqlparser/comments.go @@ -558,7 +558,7 @@ func AllowScatterDirective(stmt Statement) bool { func ForeignKeyChecksState(stmt Statement) *bool { cmt, ok := stmt.(Commented) if ok { - fkChecksVal := cmt.GetParsedComments().GetMySQLSetVarValue(sysvars.ForeignKeyChecks.Name) + fkChecksVal := cmt.GetParsedComments().GetMySQLSetVarValue(sysvars.ForeignKeyChecks) // If the value of the `foreign_key_checks` optimizer hint is something that doesn't make sense, // then MySQL just ignores it and treats it like the case, where it is unspecified. We are choosing // to have the same behaviour here. If the value doesn't match any of the acceptable values, we return nil, diff --git a/go/vt/sqlparser/comments_test.go b/go/vt/sqlparser/comments_test.go index 6312acb5994..734cd28e088 100644 --- a/go/vt/sqlparser/comments_test.go +++ b/go/vt/sqlparser/comments_test.go @@ -564,7 +564,7 @@ func TestGetMySQLSetVarValue(t *testing.T) { { name: "SET_VAR clause in the middle", comments: []string{"/*+ NO_RANGE_OPTIMIZATION(t3 PRIMARY, f2_idx) SET_VAR(foreign_key_checks=OFF) NO_ICP(t1, t2) */"}, - valToFind: sysvars.ForeignKeyChecks.Name, + valToFind: sysvars.ForeignKeyChecks, want: "OFF", }, { @@ -582,19 +582,19 @@ func TestGetMySQLSetVarValue(t *testing.T) { { name: "Multiple SET_VAR clauses", comments: []string{"/*+ SET_VAR(sort_buffer_size = 16M) */", "/*+ SET_VAR(optimizer_switch = 'mrr_cost_b(ased=of\"f') */", "/*+ SET_VAR( foReiGn_key_checks = On) */"}, - valToFind: sysvars.ForeignKeyChecks.Name, + valToFind: sysvars.ForeignKeyChecks, want: "", }, { name: "Verify casing", comments: []string{"/*+ SET_VAR(optimizer_switch = 'mrr_cost_b(ased=of\"f') SET_VAR( foReiGn_key_checks = On) */"}, - valToFind: sysvars.ForeignKeyChecks.Name, + valToFind: sysvars.ForeignKeyChecks, want: "On", }, { name: "Leading comment is a normal comment", comments: []string{"/* This is a normal comment */", "/*+ MAX_EXECUTION_TIME(1000) SET_VAR( foreign_key_checks = 1) */"}, - valToFind: sysvars.ForeignKeyChecks.Name, + valToFind: sysvars.ForeignKeyChecks, want: "1", }, } @@ -619,7 +619,7 @@ func TestSetMySQLSetVarValue(t *testing.T) { { name: "SET_VAR clause in the middle", comments: []string{"/*+ NO_RANGE_OPTIMIZATION(t3 PRIMARY, f2_idx) SET_VAR(foreign_key_checks=OFF) NO_ICP(t1, t2) */"}, - key: sysvars.ForeignKeyChecks.Name, + key: sysvars.ForeignKeyChecks, value: "On", commentsWanted: []string{"/*+ NO_RANGE_OPTIMIZATION(t3 PRIMARY, f2_idx) SET_VAR(foreign_key_checks=On) NO_ICP(t1, t2) */"}, }, @@ -640,21 +640,21 @@ func TestSetMySQLSetVarValue(t *testing.T) { { name: "Multiple SET_VAR clauses", comments: []string{"/*+ SET_VAR(sort_buffer_size = 16M) */", "/*+ SET_VAR(optimizer_switch = 'mrr_cost_b(ased=of\"f') */", "/*+ SET_VAR( foReiGn_key_checks = On) */"}, - key: sysvars.ForeignKeyChecks.Name, + key: sysvars.ForeignKeyChecks, value: "1", commentsWanted: []string{"/*+ SET_VAR(sort_buffer_size = 16M) SET_VAR(foreign_key_checks=1) */", "/*+ SET_VAR(optimizer_switch = 'mrr_cost_b(ased=of\"f') */", "/*+ SET_VAR( foReiGn_key_checks = On) */"}, }, { name: "Verify casing", comments: []string{"/*+ SET_VAR(optimizer_switch = 'mrr_cost_b(ased=of\"f') SET_VAR( foReiGn_key_checks = On) */"}, - key: sysvars.ForeignKeyChecks.Name, + key: sysvars.ForeignKeyChecks, value: "off", commentsWanted: []string{"/*+ SET_VAR(optimizer_switch = 'mrr_cost_b(ased=of\"f') SET_VAR(foReiGn_key_checks=off) */"}, }, { name: "Leading comment is a normal comment", comments: []string{"/* This is a normal comment */", "/*+ MAX_EXECUTION_TIME(1000) SET_VAR( foreign_key_checks = 1) */"}, - key: sysvars.ForeignKeyChecks.Name, + key: sysvars.ForeignKeyChecks, value: "Off", commentsWanted: []string{"/* This is a normal comment */", "/*+ MAX_EXECUTION_TIME(1000) SET_VAR(foreign_key_checks=Off) */"}, }, diff --git a/go/vt/sysvars/sysvars.go b/go/vt/sysvars/sysvars.go index 945a60ae965..c8037563ca1 100644 --- a/go/vt/sysvars/sysvars.go +++ b/go/vt/sysvars/sysvars.go @@ -57,6 +57,8 @@ var ( off = "0" utf8mb4 = "'utf8mb4'" + ForeignKeyChecks = "foreign_key_checks" + Autocommit = SystemVariable{Name: "autocommit", IsBoolean: true, Default: on} Charset = SystemVariable{Name: "charset", Default: utf8mb4, IdentifierAsString: true} ClientFoundRows = SystemVariable{Name: "client_found_rows", IsBoolean: true, Default: off} @@ -71,7 +73,6 @@ var ( TxReadOnly = SystemVariable{Name: "tx_read_only", IsBoolean: true, Default: off} Workload = SystemVariable{Name: "workload", IdentifierAsString: true} QueryTimeout = SystemVariable{Name: "query_timeout"} - ForeignKeyChecks = SystemVariable{Name: "foreign_key_checks", IsBoolean: true, SupportSetVar: true} // Online DDL DDLStrategy = SystemVariable{Name: "ddl_strategy", IdentifierAsString: true} @@ -105,7 +106,6 @@ var ( ReadAfterWriteTimeOut, SessionTrackGTIDs, QueryTimeout, - ForeignKeyChecks, } ReadOnly = []SystemVariable{ @@ -188,6 +188,7 @@ var ( {Name: "end_markers_in_json", IsBoolean: true, SupportSetVar: true}, {Name: "eq_range_index_dive_limit", SupportSetVar: true}, {Name: "explicit_defaults_for_timestamp"}, + {Name: ForeignKeyChecks, IsBoolean: true, SupportSetVar: true}, {Name: "group_concat_max_len", SupportSetVar: true}, {Name: "information_schema_stats_expiry"}, {Name: "max_heap_table_size", SupportSetVar: true}, diff --git a/go/vt/vtgate/engine/set.go b/go/vt/vtgate/engine/set.go index a0d4987d85a..9e9500d1ca8 100644 --- a/go/vt/vtgate/engine/set.go +++ b/go/vt/vtgate/engine/set.go @@ -445,8 +445,6 @@ func (svss *SysVarSetAware) Execute(ctx context.Context, vcursor VCursor, env *e switch svss.Name { case sysvars.Autocommit.Name: err = svss.setBoolSysVar(ctx, env, vcursor.Session().SetAutocommit) - case sysvars.ForeignKeyChecks.Name: - err = svss.setBoolSysVar(ctx, env, vcursor.Session().SetSessionForeignKeyChecks) case sysvars.ClientFoundRows.Name: err = svss.setBoolSysVar(ctx, env, vcursor.Session().SetClientFoundRows) case sysvars.SkipQueryPlanCache.Name: diff --git a/go/vt/vtgate/planbuilder/operators/insert.go b/go/vt/vtgate/planbuilder/operators/insert.go index f783ac7a5bc..5cb25feb66c 100644 --- a/go/vt/vtgate/planbuilder/operators/insert.go +++ b/go/vt/vtgate/planbuilder/operators/insert.go @@ -134,6 +134,7 @@ func createOperatorFromInsert(ctx *plancontext.PlanningContext, ins *sqlparser.I whereExpr := getWhereCondExpr(append(uniqKeyCompExprs, pkCompExpr)) delStmt := &sqlparser.Delete{ + Comments: ins.Comments, TableExprs: sqlparser.TableExprs{sqlparser.CloneRefOfAliasedTableExpr(ins.Table)}, Where: sqlparser.NewWhere(sqlparser.WhereClause, whereExpr), } diff --git a/go/vt/vtgate/planbuilder/operators/update.go b/go/vt/vtgate/planbuilder/operators/update.go index ccfdddc3ea9..f780e5405db 100644 --- a/go/vt/vtgate/planbuilder/operators/update.go +++ b/go/vt/vtgate/planbuilder/operators/update.go @@ -442,7 +442,7 @@ func buildChildUpdOpForCascade(ctx *plancontext.PlanningContext, fk vindexes.Chi // Because we could be updating the child to a non-null value, // We have to run with foreign key checks OFF because the parent isn't guaranteed to have // the data being updated to. - parsedComments := (&sqlparser.ParsedComments{}).SetMySQLSetVarValue(sysvars.ForeignKeyChecks.Name, "OFF").Parsed() + parsedComments := (&sqlparser.ParsedComments{}).SetMySQLSetVarValue(sysvars.ForeignKeyChecks, "OFF").Parsed() childUpdStmt := &sqlparser.Update{ Comments: parsedComments, Exprs: childUpdateExprs, @@ -515,7 +515,7 @@ func buildChildUpdOpForSetNull( func getParsedCommentsForFkChecks(ctx *plancontext.PlanningContext) (parsedComments *sqlparser.ParsedComments) { fkState := ctx.VSchema.GetForeignKeyChecksState() if fkState != nil && *fkState { - parsedComments = parsedComments.SetMySQLSetVarValue(sysvars.ForeignKeyChecks.Name, "ON").Parsed() + parsedComments = parsedComments.SetMySQLSetVarValue(sysvars.ForeignKeyChecks, "ON").Parsed() } return parsedComments } diff --git a/go/vt/vtgate/planbuilder/update.go b/go/vt/vtgate/planbuilder/update.go index 9bcc8691f8e..9fbfd85afab 100644 --- a/go/vt/vtgate/planbuilder/update.go +++ b/go/vt/vtgate/planbuilder/update.go @@ -51,7 +51,7 @@ func gen4UpdateStmtPlanner( if ctx.SemTable.HasNonLiteralForeignKeyUpdate(updStmt.Exprs) { // Since we are running the query with foreign key checks off, we have to verify all the foreign keys validity on vtgate. ctx.VerifyAllFKs = true - updStmt.SetComments(updStmt.GetParsedComments().SetMySQLSetVarValue(sysvars.ForeignKeyChecks.Name, "OFF")) + updStmt.SetComments(updStmt.GetParsedComments().SetMySQLSetVarValue(sysvars.ForeignKeyChecks, "OFF")) } // Remove all the foreign keys that don't require any handling. diff --git a/go/vt/vtgate/safe_session.go b/go/vt/vtgate/safe_session.go index 5479f69b435..fd087591024 100644 --- a/go/vt/vtgate/safe_session.go +++ b/go/vt/vtgate/safe_session.go @@ -557,7 +557,7 @@ func (session *SafeSession) HasSystemVariables() (found bool) { session.GetSystemVariables(func(_ string, _ string) { found = true }) - return found || session.ForeignKeyChecks() != nil + return } func (session *SafeSession) TimeZone() *time.Location { @@ -571,23 +571,27 @@ func (session *SafeSession) TimeZone() *time.Location { loc, _ := datetime.ParseTimeZone(tz) return loc } -func (session *SafeSession) ForeignKeyChecks() *string { + +// ForeignKeyChecks returns the foreign_key_checks stores in system_variables map in the session. +func (session *SafeSession) ForeignKeyChecks() (*bool, *string) { session.mu.Lock() - fkVal, ok := session.SystemVariables["foreign_key_checks"] + fkVal, ok := session.SystemVariables[sysvars.ForeignKeyChecks] session.mu.Unlock() if !ok { - return nil + return nil, nil } switch strings.ToLower(fkVal) { case "off", "0": - fkCheck := "0" - return &fkCheck + fkCheckStr := "0" + fkCheckBool := false + return &fkCheckBool, &fkCheckStr case "on", "1": - fkCheck := "1" - return &fkCheck + fkCheckStr := "1" + fkCheckBool := true + return &fkCheckBool, &fkCheckStr } - return nil + return nil, nil } // SetOptions sets the options @@ -628,13 +632,6 @@ func (session *SafeSession) SetPreQueries() []string { sysVars[k] = v }) - fkVal := session.ForeignKeyChecks() - if fkVal != nil { - k := "foreign_key_checks" - keys = append(keys, k) - sysVars[k] = *fkVal - } - // if not system variables to set, return if len(keys) == 0 { return nil diff --git a/go/vt/vtgate/vcursor_impl.go b/go/vt/vtgate/vcursor_impl.go index db678d56354..c548a4aa710 100644 --- a/go/vt/vtgate/vcursor_impl.go +++ b/go/vt/vtgate/vcursor_impl.go @@ -835,9 +835,9 @@ func (vc *vcursorImpl) SetAutocommit(ctx context.Context, autocommit bool) error // SetSessionForeignKeyChecks implements the SessionActions interface func (vc *vcursorImpl) SetSessionForeignKeyChecks(ctx context.Context, foreignKeyChecks bool) error { if foreignKeyChecks { - vc.safeSession.SetSystemVariable(sysvars.ForeignKeyChecks.Name, "1") + vc.safeSession.SetSystemVariable(sysvars.ForeignKeyChecks, "1") } else { - vc.safeSession.SetSystemVariable(sysvars.ForeignKeyChecks.Name, "0") + vc.safeSession.SetSystemVariable(sysvars.ForeignKeyChecks, "0") } return nil } @@ -1326,7 +1326,7 @@ func (vc *vcursorImpl) CloneForReplicaWarming(ctx context.Context) engine.VCurso callerId := callerid.EffectiveCallerIDFromContext(ctx) immediateCallerId := callerid.ImmediateCallerIDFromContext(ctx) - timedCtx, _ := context.WithTimeout(context.Background(), warmingReadsQueryTimeout) //nolint + timedCtx, _ := context.WithTimeout(context.Background(), warmingReadsQueryTimeout) // nolint clonedCtx := callerid.NewContext(timedCtx, callerId, immediateCallerId) v := &vcursorImpl{ @@ -1365,17 +1365,8 @@ func (vc *vcursorImpl) UpdateForeignKeyChecksState(fkStateFromQuery *bool) { return } // If the query doesn't have anything, then we consult the session state. - fkVal, isPresent := vc.safeSession.SystemVariables[sysvars.ForeignKeyChecks.Name] - if isPresent { - switch strings.ToLower(fkVal) { - case "on", "1": - val := true - vc.fkChecksState = &val - case "off", "0": - val := false - vc.fkChecksState = &val - } - } + fkCheck, _ := vc.safeSession.ForeignKeyChecks() + vc.fkChecksState = fkCheck } // GetForeignKeyChecksState gets the stored foreign key checks state in the vcursor. From 45c511b089322fa8d548d0aea1b4cf1e327d9e16 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 6 Dec 2023 18:52:27 +0530 Subject: [PATCH 6/9] minor refactor Signed-off-by: Harshit Gangal --- go/vt/vtgate/safe_session.go | 2 +- go/vt/vtgate/vcursor_impl.go | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/go/vt/vtgate/safe_session.go b/go/vt/vtgate/safe_session.go index fd087591024..34645d87bf3 100644 --- a/go/vt/vtgate/safe_session.go +++ b/go/vt/vtgate/safe_session.go @@ -572,7 +572,7 @@ func (session *SafeSession) TimeZone() *time.Location { return loc } -// ForeignKeyChecks returns the foreign_key_checks stores in system_variables map in the session. +// ForeignKeyChecks returns the foreign_key_checks stored in system_variables map in the session. func (session *SafeSession) ForeignKeyChecks() (*bool, *string) { session.mu.Lock() fkVal, ok := session.SystemVariables[sysvars.ForeignKeyChecks] diff --git a/go/vt/vtgate/vcursor_impl.go b/go/vt/vtgate/vcursor_impl.go index c548a4aa710..d69ffcffe0d 100644 --- a/go/vt/vtgate/vcursor_impl.go +++ b/go/vt/vtgate/vcursor_impl.go @@ -1326,7 +1326,7 @@ func (vc *vcursorImpl) CloneForReplicaWarming(ctx context.Context) engine.VCurso callerId := callerid.EffectiveCallerIDFromContext(ctx) immediateCallerId := callerid.ImmediateCallerIDFromContext(ctx) - timedCtx, _ := context.WithTimeout(context.Background(), warmingReadsQueryTimeout) // nolint + timedCtx, _ := context.WithTimeout(context.Background(), warmingReadsQueryTimeout) //nolint clonedCtx := callerid.NewContext(timedCtx, callerId, immediateCallerId) v := &vcursorImpl{ @@ -1365,8 +1365,7 @@ func (vc *vcursorImpl) UpdateForeignKeyChecksState(fkStateFromQuery *bool) { return } // If the query doesn't have anything, then we consult the session state. - fkCheck, _ := vc.safeSession.ForeignKeyChecks() - vc.fkChecksState = fkCheck + vc.fkChecksState, _ = vc.safeSession.ForeignKeyChecks() } // GetForeignKeyChecksState gets the stored foreign key checks state in the vcursor. From 2d0beef98918ae98265c02ee7774ba0c79bc5e63 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 6 Dec 2023 19:48:06 +0530 Subject: [PATCH 7/9] fix: update test expectation Signed-off-by: Harshit Gangal --- .../vtgate/planbuilder/testdata/foreignkey_checks_on_cases.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/vtgate/planbuilder/testdata/foreignkey_checks_on_cases.json b/go/vt/vtgate/planbuilder/testdata/foreignkey_checks_on_cases.json index b4892a99052..a61a035a8d8 100644 --- a/go/vt/vtgate/planbuilder/testdata/foreignkey_checks_on_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/foreignkey_checks_on_cases.json @@ -1772,7 +1772,7 @@ "Sharded": false }, "TargetTabletType": "PRIMARY", - "Query": "delete from u_tbl1 where (id) in ((1))", + "Query": "delete /*+ SET_VAR(foreign_key_checks=On) */ from u_tbl1 where (id) in ((1))", "Table": "u_tbl1" } ] From cf421cd9f28931b31c4edc89cb30399a6845706e Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 6 Dec 2023 20:01:28 +0530 Subject: [PATCH 8/9] addressed review comments Signed-off-by: Harshit Gangal --- go/vt/vtgate/engine/fake_vcursor_test.go | 8 -------- go/vt/vtgate/engine/primitive.go | 1 - go/vt/vtgate/safe_session.go | 12 +++++------- go/vt/vtgate/vcursor_impl.go | 13 +------------ 4 files changed, 6 insertions(+), 28 deletions(-) diff --git a/go/vt/vtgate/engine/fake_vcursor_test.go b/go/vt/vtgate/engine/fake_vcursor_test.go index f9cedd74bfc..ae1c9e918a7 100644 --- a/go/vt/vtgate/engine/fake_vcursor_test.go +++ b/go/vt/vtgate/engine/fake_vcursor_test.go @@ -275,10 +275,6 @@ func (t *noopVCursor) SetAutocommit(context.Context, bool) error { panic("implement me") } -func (t *noopVCursor) SetSessionForeignKeyChecks(ctx context.Context, foreignKeyChecks bool) error { - panic("implement me") -} - func (t *noopVCursor) SetClientFoundRows(context.Context, bool) error { panic("implement me") } @@ -747,10 +743,6 @@ func (f *loggingVCursor) SetAutocommit(context.Context, bool) error { panic("implement me") } -func (f *loggingVCursor) SetSessionForeignKeyChecks(ctx context.Context, foreignKeyChecks bool) error { - panic("implement me") -} - func (f *loggingVCursor) SetClientFoundRows(context.Context, bool) error { panic("implement me") } diff --git a/go/vt/vtgate/engine/primitive.go b/go/vt/vtgate/engine/primitive.go index 833f4cc3b45..c69423ee3fb 100644 --- a/go/vt/vtgate/engine/primitive.go +++ b/go/vt/vtgate/engine/primitive.go @@ -154,7 +154,6 @@ type ( SetAutocommit(ctx context.Context, autocommit bool) error SetClientFoundRows(context.Context, bool) error - SetSessionForeignKeyChecks(ctx context.Context, autocommit bool) error SetSkipQueryPlanCache(context.Context, bool) error SetSQLSelectLimit(int64) error SetTransactionMode(vtgatepb.TransactionMode) diff --git a/go/vt/vtgate/safe_session.go b/go/vt/vtgate/safe_session.go index 34645d87bf3..2adb5b665a5 100644 --- a/go/vt/vtgate/safe_session.go +++ b/go/vt/vtgate/safe_session.go @@ -573,25 +573,23 @@ func (session *SafeSession) TimeZone() *time.Location { } // ForeignKeyChecks returns the foreign_key_checks stored in system_variables map in the session. -func (session *SafeSession) ForeignKeyChecks() (*bool, *string) { +func (session *SafeSession) ForeignKeyChecks() *bool { session.mu.Lock() fkVal, ok := session.SystemVariables[sysvars.ForeignKeyChecks] session.mu.Unlock() if !ok { - return nil, nil + return nil } switch strings.ToLower(fkVal) { case "off", "0": - fkCheckStr := "0" fkCheckBool := false - return &fkCheckBool, &fkCheckStr + return &fkCheckBool case "on", "1": - fkCheckStr := "1" fkCheckBool := true - return &fkCheckBool, &fkCheckStr + return &fkCheckBool } - return nil, nil + return nil } // SetOptions sets the options diff --git a/go/vt/vtgate/vcursor_impl.go b/go/vt/vtgate/vcursor_impl.go index d69ffcffe0d..616b1ce8846 100644 --- a/go/vt/vtgate/vcursor_impl.go +++ b/go/vt/vtgate/vcursor_impl.go @@ -43,7 +43,6 @@ import ( vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/srvtopo" - "vitess.io/vitess/go/vt/sysvars" "vitess.io/vitess/go/vt/topo" topoprotopb "vitess.io/vitess/go/vt/topo/topoproto" "vitess.io/vitess/go/vt/topotools" @@ -832,16 +831,6 @@ func (vc *vcursorImpl) SetAutocommit(ctx context.Context, autocommit bool) error return nil } -// SetSessionForeignKeyChecks implements the SessionActions interface -func (vc *vcursorImpl) SetSessionForeignKeyChecks(ctx context.Context, foreignKeyChecks bool) error { - if foreignKeyChecks { - vc.safeSession.SetSystemVariable(sysvars.ForeignKeyChecks, "1") - } else { - vc.safeSession.SetSystemVariable(sysvars.ForeignKeyChecks, "0") - } - return nil -} - // SetQueryTimeout implements the SessionActions interface func (vc *vcursorImpl) SetQueryTimeout(maxExecutionTime int64) { vc.safeSession.QueryTimeout = maxExecutionTime @@ -1365,7 +1354,7 @@ func (vc *vcursorImpl) UpdateForeignKeyChecksState(fkStateFromQuery *bool) { return } // If the query doesn't have anything, then we consult the session state. - vc.fkChecksState, _ = vc.safeSession.ForeignKeyChecks() + vc.fkChecksState = vc.safeSession.ForeignKeyChecks() } // GetForeignKeyChecksState gets the stored foreign key checks state in the vcursor. From f18a81f1c7199abe7430369ce8ae9bd7c806819b Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 6 Dec 2023 20:28:11 +0530 Subject: [PATCH 9/9] test: fix test Signed-off-by: Harshit Gangal --- go/vt/vtgate/executor_set_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/go/vt/vtgate/executor_set_test.go b/go/vt/vtgate/executor_set_test.go index 68898b7ea45..5377f72c66b 100644 --- a/go/vt/vtgate/executor_set_test.go +++ b/go/vt/vtgate/executor_set_test.go @@ -319,9 +319,12 @@ func TestExecutorSetOp(t *testing.T) { sysVars: map[string]string{"sql_quote_show_create": "0"}, result: returnResult("sql_quote_show_create", "int64", "0"), }, { - in: "set foreign_key_checks = 1", - sysVars: map[string]string{"foreign_key_checks": "1"}, - result: returnNoResult("foreign_key_checks", "int64"), + in: "set foreign_key_checks = 1", + result: returnNoResult("foreign_key_checks", "int64"), + }, { + in: "set foreign_key_checks = 0", + sysVars: map[string]string{"foreign_key_checks": "0"}, + result: returnResult("foreign_key_checks", "int64", "0"), }, { in: "set unique_checks = 0", sysVars: map[string]string{"unique_checks": "0"},