Repository: vitessio/vitess
Pull request #6099 (2020-04-21 08:50:34)

Set CheckAndIgnore to Log Warn than Error on failing the SysVar settings check
Signed-off-by: Harshit Gangal <harshit@planetscale.com>


Patch:
diff --git a/go/vt/vtgate/engine/set.go b/go/vt/vtgate/engine/set.go
index 42d5a173f2..0c52c4972d 100644
--- a/go/vt/vtgate/engine/set.go
+++ b/go/vt/vtgate/engine/set.go
@@ -61,7 +61,7 @@ type (
 		Name              string
 		Keyspace          *vindexes.Keyspace
 		TargetDestination key.Destination
-		CheckSysVarQuery  string
+		Expr              string
 	}
 )
 
@@ -196,12 +196,17 @@ func (svci *SysVarCheckAndIgnore) Execute(vcursor VCursor, bindVars map[string]*
 	if len(rss) != 1 {
 		return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Unexpected error, DestinationKeyspaceID mapping to multiple shards: %v", svci.TargetDestination)
 	}
-	result, err := execShard(vcursor, svci.CheckSysVarQuery, bindVars, rss[0], false /* rollbackOnError */, false /* canAutocommit */)
+	checkSysVarQuery := fmt.Sprintf("select 1 from dual where @@%s = %s", svci.Name, svci.Expr)
+	result, err := execShard(vcursor, checkSysVarQuery, bindVars, rss[0], false /* rollbackOnError */, false /* canAutocommit */)
 	if err != nil {
 		return err
 	}
-	if result.RowsAffected != 1 {
-		return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Modification not allowed using set construct for: %s", svci.Name)
+	var warning *querypb.QueryWarning
+	if result.RowsAffected == 0 {
+		warning = &querypb.QueryWarning{Code: mysql.ERNotSupportedYet, Message: fmt.Sprintf("Modification not allowed using set construct for: %s", svci.Name)}
+	} else {
+		warning = &querypb.QueryWarning{Code: mysql.ERNotSupportedYet, Message: fmt.Sprintf("Ignored inapplicable SET %v = %v", svci.Name, svci.Expr)}
 	}
+	vcursor.Session().RecordWarning(warning)
 	return nil
 }
diff --git a/go/vt/vtgate/planbuilder/set.go b/go/vt/vtgate/planbuilder/set.go
index d145a5f547..c932934ade 100644
--- a/go/vt/vtgate/planbuilder/set.go
+++ b/go/vt/vtgate/planbuilder/set.go
@@ -17,7 +17,6 @@ limitations under the License.
 package planbuilder
 
 import (
-	"fmt"
 	"strings"
 
 	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
@@ -104,6 +103,6 @@ func buildSetOpCheckAndIgnore(expr *sqlparser.SetExpr, vschema ContextVSchema) (
 		Name:              expr.Name.Lowered(),
 		Keyspace:          keyspace,
 		TargetDestination: dest,
-		CheckSysVarQuery:  fmt.Sprintf("select 1 from dual where @@%s = %s", expr.Name.Lowered(), buf.String()),
+		Expr:              buf.String(),
 	}, nil
 }


Test patch:
diff --git a/go/test/endtoend/vtgate/setstatement/sysvar_test.go b/go/test/endtoend/vtgate/setstatement/sysvar_test.go
index 3db7aaadb8..11c4840006 100644
--- a/go/test/endtoend/vtgate/setstatement/sysvar_test.go
+++ b/go/test/endtoend/vtgate/setstatement/sysvar_test.go
@@ -54,8 +54,8 @@ func TestSetSysVar(t *testing.T) {
 		query:        `set @@sql_mode = concat(@@sql_mode,"")`,
 		expectedRows: ``, rowsAffected: 0,
 	}, {
-		query:  `set @@sql_mode = concat(@@sql_mode,"ALLOW_INVALID_DATES")`,
-		errMsg: "Modification not allowed using set construct for: sql_mode",
+		query:           `set @@sql_mode = concat(@@sql_mode,"ALLOW_INVALID_DATES")`,
+		expectedWarning: "[[VARCHAR(\"Warning\") UINT16(1235) VARCHAR(\"Modification not allowed using set construct for: sql_mode\")]]",
 	}}
 
 	conn, err := mysql.Connect(ctx, &vtParams)
diff --git a/go/test/endtoend/vtgate/setstatement/udv_test.go b/go/test/endtoend/vtgate/setstatement/udv_test.go
index 8e2cff6e72..b7fc1f8ff5 100644
--- a/go/test/endtoend/vtgate/setstatement/udv_test.go
+++ b/go/test/endtoend/vtgate/setstatement/udv_test.go
@@ -46,7 +46,7 @@ func TestSetUDV(t *testing.T) {
 		expectedRows: "", rowsAffected: 0,
 	}, {
 		query:        "select @foo, @bar, @baz",
-		expectedRows: `[[VARCHAR("abc") INT64(42) DECIMAL(30.5)]]`, rowsAffected: 1,
+		expectedRows: `[[VARBINARY("abc") INT64(42) FLOAT64(30.5)]]`, rowsAffected: 1,
 	}, {
 		query:        "insert into test(id, val1, val2, val3) values(1, @foo, null, null), (2, null, @bar, null), (3, null, null, @baz)",
 		expectedRows: ``, rowsAffected: 3,
diff --git a/go/vt/vtgate/engine/set_test.go b/go/vt/vtgate/engine/set_test.go
index a243445b4c..3c4249f3ee 100644
--- a/go/vt/vtgate/engine/set_test.go
+++ b/go/vt/vtgate/engine/set_test.go
@@ -79,7 +79,7 @@ func TestSetTable(t *testing.T) {
 						Sharded: true,
 					},
 					TargetDestination: key.DestinationAnyShard{},
-					CheckSysVarQuery:  "dummy_query",
+					Expr:              "dummy_expr",
 				},
 			},
 			qr: []*sqltypes.Result{sqltypes.MakeTestResult(
@@ -91,7 +91,10 @@ func TestSetTable(t *testing.T) {
 			)},
 			expectedQueryLog: []string{
 				`ResolveDestinations ks [] Destinations:DestinationAnyShard()`,
-				`ExecuteMultiShard ks.-20: dummy_query {} false false`,
+				`ExecuteMultiShard ks.-20: select 1 from dual where @@x = dummy_expr {} false false`,
+			},
+			expectedWarning: []*querypb.QueryWarning{
+				{Code: 1235, Message: "Ignored inapplicable SET x = dummy_expr"},
 			},
 		},
 		{
@@ -104,14 +107,16 @@ func TestSetTable(t *testing.T) {
 						Sharded: true,
 					},
 					TargetDestination: key.DestinationAnyShard{},
-					CheckSysVarQuery:  "dummy_query",
+					Expr:              "dummy_expr",
 				},
 			},
 			expectedQueryLog: []string{
 				`ResolveDestinations ks [] Destinations:DestinationAnyShard()`,
-				`ExecuteMultiShard ks.-20: dummy_query {} false false`,
+				`ExecuteMultiShard ks.-20: select 1 from dual where @@x = dummy_expr {} false false`,
+			},
+			expectedWarning: []*querypb.QueryWarning{
+				{Code: 1235, Message: "Modification not allowed using set construct for: x"},
 			},
-			expectedError: "Modification not allowed using set construct for: x",
 		},
 		{
 			testName: "sysvar checkAndIgnore multi destination error",
@@ -123,7 +128,7 @@ func TestSetTable(t *testing.T) {
 						Sharded: true,
 					},
 					TargetDestination: key.DestinationAllShards{},
-					CheckSysVarQuery:  "dummy_query",
+					Expr:              "dummy_expr",
 				},
 			},
 			expectedQueryLog: []string{
@@ -151,16 +156,17 @@ func TestSetTable(t *testing.T) {
 						Sharded: true,
 					},
 					TargetDestination: key.DestinationAnyShard{},
-					CheckSysVarQuery:  "dummy_query",
+					Expr:              "dummy_expr",
 				},
 			},
 			expectedQueryLog: []string{
 				`UDV set with (x,INT64(1))`,
 				`ResolveDestinations ks [] Destinations:DestinationAnyShard()`,
-				`ExecuteMultiShard ks.-20: dummy_query {} false false`,
+				`ExecuteMultiShard ks.-20: select 1 from dual where @@z = dummy_expr {} false false`,
 			},
 			expectedWarning: []*querypb.QueryWarning{
 				{Code: 1235, Message: "Ignored inapplicable SET y = 2"},
+				{Code: 1235, Message: "Ignored inapplicable SET z = dummy_expr"},
 			},
 			qr: []*sqltypes.Result{sqltypes.MakeTestResult(
 				sqltypes.MakeTestFields(
diff --git a/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt b/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt
index 5d554ab27a..38665d8d61 100644
--- a/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt
@@ -33,7 +33,7 @@
           "Sharded": false
         },
         "TargetDestination": {},
-        "CheckSysVarQuery": "select 1 from dual where @@sql_mode = concat(@@sql_mode, ',NO_AUTO_CREATE_USER')"
+        "Expr": "concat(@@sql_mode, ',NO_AUTO_CREATE_USER')"
       }
     ]
   }