Repository: vitessio/vitess
Pull request #11490 (2022-10-13 10:57:57)

fix: reserve connection to follow query timeout when outside of transaction
<!--
  Thank you for your contribution to the Vitess project.
  How to contribute: https://vitess.io/docs/contributing/
  Please first make sure there is an open Issue to discuss the feature/fix suggested in this PR.
  If this is a new feature, please mark the Issue as "RFC".
 -->

<!-- if this PR is Work in Progress please create it as a Draft Pull Request -->

## Description

<!-- A few sentences describing the overall goals of the pull request's commits. -->
<!-- If this is a bug fix and you think the fix should be backported, please write so. -->

This PR fixes the bug in reserve connection query execution on `ReserveExecute` and `ReserveStreamExecute` calls to not use transaction timeout for queries outside of transaction.

## Related Issue(s)

<!-- List related issues and pull requests. If this PR fixes an issue, please add it using Fixes #????  -->

## Checklist

-   [X] "Backport me!" label has been added if this change should be backported
-   [X] Tests were added or are not required
-   [X] Documentation was added or is not required

## Deployment Notes

<!-- Notes regarding deployment of the contained body of work. These should note any db migrations, etc. -->


vitess-bot[bot]: ### Review Checklist
            
Hello reviewers! :wave: Please follow this checklist when reviewing this Pull Request.

#### General
- [ ] Ensure that the Pull Request has a descriptive title.
- [ ] If this is a change that users need to know about, please apply the `release notes (needs details)` label so that merging is blocked unless the summary release notes document is included.

#### If a new flag is being introduced:
- [ ] Is it really necessary to add this flag?
- [ ] Flag names should be clear and intuitive (as far as possible)
- [ ] Help text should be descriptive.
- [ ] Flag names should use dashes (`-`) as word separators rather than underscores (`_`).

#### If a workflow is added or modified:
- [ ] Each item in `Jobs` should be named in order to mark it as `required`.
- [ ] If the workflow should be required, the maintainer team should be notified.

#### Bug fixes
- [ ] There should be at least one unit or end-to-end test.
- [ ] The Pull Request description should include a link to an issue that describes the bug.

#### Non-trivial changes
- [ ] There should be some code comments as to why things are implemented the way they are.

#### New/Existing features
- [ ] Should be documented, either by modifying the existing documentation or creating new documentation.
- [ ] New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

#### Backward compatibility            
- [ ] Protobuf changes should be wire-compatible.
- [ ] Changes to `_vt` tables and RPCs need to be backward compatible.
- [ ] `vtctl` command output order should be stable and `awk`-able.
- [ ] RPC changes should be compatible with vitess-operator
- [ ] If a flag is removed, then it should also be removed from VTop, if used there.


Patch:
diff --git a/go/vt/vtgate/semantics/early_rewriter.go b/go/vt/vtgate/semantics/early_rewriter.go
index 206a0d89db..d4ca8dc581 100644
--- a/go/vt/vtgate/semantics/early_rewriter.go
+++ b/go/vt/vtgate/semantics/early_rewriter.go
@@ -79,6 +79,23 @@ func (r *earlyRewriter) down(cursor *sqlparser.Cursor) error {
 		if newNode != nil {
 			node.Expr = newNode
 		}
+	case *sqlparser.ComparisonExpr:
+		lft, lftOK := node.Left.(sqlparser.ValTuple)
+		rgt, rgtOK := node.Right.(sqlparser.ValTuple)
+		if !lftOK || !rgtOK || len(lft) != len(rgt) || node.Operator != sqlparser.EqualOp {
+			return nil
+		}
+		var predicates []sqlparser.Expr
+		for i, l := range lft {
+			r := rgt[i]
+			predicates = append(predicates, &sqlparser.ComparisonExpr{
+				Operator: sqlparser.EqualOp,
+				Left:     l,
+				Right:    r,
+				Escape:   node.Escape,
+			})
+		}
+		cursor.Replace(sqlparser.AndExpressions(predicates...))
 	}
 	return nil
 }
diff --git a/go/vt/vttablet/endtoend/framework/server.go b/go/vt/vttablet/endtoend/framework/server.go
index ade79b5d89..1f5b719d77 100644
--- a/go/vt/vttablet/endtoend/framework/server.go
+++ b/go/vt/vttablet/endtoend/framework/server.go
@@ -116,6 +116,8 @@ func StartServer(connParams, connAppDebugParams mysql.ConnParams, dbName string)
 	config.SignalSchemaChangeReloadIntervalSeconds = tabletenv.Seconds(2.1)
 	config.SignalWhenSchemaChange = true
 	config.Healthcheck.IntervalSeconds = 0.1
+	config.Oltp.TxTimeoutSeconds = 5
+	config.Olap.TxTimeoutSeconds = 5
 	gotBytes, _ := yaml2.Marshal(config)
 	log.Infof("Config:\n%s", gotBytes)
 	return StartCustomServer(connParams, connAppDebugParams, dbName, config)
diff --git a/go/vt/vttablet/tabletserver/tabletserver.go b/go/vt/vttablet/tabletserver/tabletserver.go
index bf1bf878d0..c8959d5db7 100644
--- a/go/vt/vttablet/tabletserver/tabletserver.go
+++ b/go/vt/vttablet/tabletserver/tabletserver.go
@@ -1278,7 +1278,7 @@ func (tsv *TabletServer) ReserveExecute(ctx context.Context, target *querypb.Tar
 		return state, nil, err
 	}
 
-	result, err = tsv.execute(ctx, target, sql, bindVariables, state.ReservedID, state.ReservedID, nil, options)
+	result, err = tsv.execute(ctx, target, sql, bindVariables, transactionID, state.ReservedID, nil, options)
 	return state, result, err
 }
 
@@ -1328,7 +1328,7 @@ func (tsv *TabletServer) ReserveStreamExecute(
 		return state, err
 	}
 
-	err = tsv.streamExecute(ctx, target, sql, bindVariables, state.ReservedID, state.ReservedID, nil, options, callback)
+	err = tsv.streamExecute(ctx, target, sql, bindVariables, transactionID, state.ReservedID, nil, options, callback)
 	return state, err
 }
 


Test patch:
diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.json b/go/vt/vtgate/planbuilder/testdata/filter_cases.json
index 76f6def665..b6230ae234 100644
--- a/go/vt/vtgate/planbuilder/testdata/filter_cases.json
+++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.json
@@ -5701,6 +5701,47 @@
       ]
     }
   },
+  {
+    "comment": "deconstruct tuple equality comparisons",
+    "query": "select id from user where (id, name) = (34, 'apa')",
+    "v3-plan": {
+      "QueryType": "SELECT",
+      "Original": "select id from user where (id, name) = (34, 'apa')",
+      "Instructions": {
+        "OperatorType": "Route",
+        "Variant": "Scatter",
+        "Keyspace": {
+          "Name": "user",
+          "Sharded": true
+        },
+        "FieldQuery": "select id from `user` where 1 != 1",
+        "Query": "select id from `user` where (id, `name`) = (34, 'apa')",
+        "Table": "`user`"
+      }
+    },
+    "gen4-plan": {
+      "QueryType": "SELECT",
+      "Original": "select id from user where (id, name) = (34, 'apa')",
+      "Instructions": {
+        "OperatorType": "Route",
+        "Variant": "EqualUnique",
+        "Keyspace": {
+          "Name": "user",
+          "Sharded": true
+        },
+        "FieldQuery": "select id from `user` where 1 != 1",
+        "Query": "select id from `user` where id = 34 and `name` = 'apa'",
+        "Table": "`user`",
+        "Values": [
+          "INT64(34)"
+        ],
+        "Vindex": "user_index"
+      },
+      "TablesUsed": [
+        "user.user"
+      ]
+    }
+  },
   {
     "comment": "multi column vindex with both IN predicate and equality predicate",
     "query": "select * from multicol_tbl where cola in (1,10) and cola = 4 and colb in (5,6) and colb = 7",
@@ -6343,4 +6384,4 @@
       ]
     }
   }
-]
\ No newline at end of file
+]
diff --git a/go/vt/vttablet/endtoend/reserve_test.go b/go/vt/vttablet/endtoend/reserve_test.go
index 095370453c..355e4d5b95 100644
--- a/go/vt/vttablet/endtoend/reserve_test.go
+++ b/go/vt/vttablet/endtoend/reserve_test.go
@@ -1032,3 +1032,17 @@ func TestFailInfiniteSessions(t *testing.T) {
 			client.Release())
 	}
 }
+
+func TestReserveQueryTimeout(t *testing.T) {
+	client := framework.NewClient()
+
+	_, err := client.ReserveExecute("select sleep(19)", []string{"set sql_mode = ''"}, nil)
+	assert.NoError(t, err)
+	assert.NoError(t,
+		client.Release())
+
+	_, err = client.ReserveStreamExecute("select sleep(19)", []string{"set sql_mode = ''"}, nil)
+	assert.NoError(t, err)
+	assert.NoError(t,
+		client.Release())
+}