Repository: vitessio/vitess
Pull request #11504 (2022-10-14 10:24:27)

[main] bug fix: using self-referencing columns in HAVING should not overflow (#11499)
## Description
This is a forwardport of #11499.


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/doc/releasenotes/15_0_0_release_notes.md b/doc/releasenotes/15_0_0_release_notes.md
index 3b87fca370..d1c9f772de 100644
--- a/doc/releasenotes/15_0_0_release_notes.md
+++ b/doc/releasenotes/15_0_0_release_notes.md
@@ -11,6 +11,7 @@
 - [Mysql Compatibility](#mysql-compatibility)
 - [Durability Policy](#durability-policy)
 - [New EXPLAIN format](#new-explain-format)
+- [VTOrc](#vtorc)
 
 ## Known Issues
 
@@ -27,7 +28,7 @@
 - The deprecated flag `--master_connect_retry` has been removed. Please use `--replication_connect_retry` instead.
 - `vtctl` commands that take shard names and ranges as positional arguments (e.g. `vtctl Reshard ks.workflow -80 -40,40-80`) need to have their positional arguments separated from their flag arguments by a double-dash separator to avoid the new parsing library from mistaking them as flags (e.g. `vtctl Reshard ks.workflow -- -80 -40,40-80`).
 
-#### Vindex Interface
+#### <a id="vindex-interface"/>Vindex Interface
 
 All the vindex interface methods are changed by adding `context.Context` as an input parameter.
 
@@ -41,7 +42,7 @@ Map(ctx context.Context, vcursor VCursor, .... ) ....
 This only affects users who have added their own custom vindex implementation. 
 They are required to change their implementation with these new interface method expectations.
 
-#### LogStats Table and Keyspace deprecated
+#### <a id="logstats-table-and-keyspace-deprecated"/>LogStats Table and Keyspace deprecated
 
 Information about which tables are used was being reported through the `Keyspace` and `Table` fields on LogStats.
 For multi-table queries, this output can be confusing, so we have added `TablesUsed`, that is a string array, listing all tables and which keyspace they are on.
@@ -58,7 +59,7 @@ The connection pool with prefilled connections have been removed. The pool now d
 Following flags are deprecated: `queryserver-config-pool-prefill-parallelism`, `queryserver-config-stream-pool-prefill-parallelism`, `queryserver-config-transaction-prefill-parallelism`
 and will be removed in future version.
 
-### Command-line syntax deprecations
+### <a id="command-line-syntax-deprecations"/>Command-line syntax deprecations
 
 #### vttablet startup flag deletions
 The following VTTablet flags were deprecated in 7.0. They have now been deleted
@@ -76,14 +77,14 @@ The following VTTablet flags were deprecated in 7.0. They have now been deleted
 - --enable_semi_sync is now deprecated. It will be removed in v16. Instead, set the correct durability policy using `SetKeyspaceDurabilityPolicy`
 - --queryserver-config-pool-prefill-parallelism, --queryserver-config-stream-pool-prefill-parallelism and --queryserver-config-transaction-prefill-parallelism have all been deprecated. They will be removed in v16.
 
-### New command line flags and behavior
+### <a id="new-command-line-flags-and-behavior"/>New command line flags and behavior
 
 #### vtgate --mysql-server-pool-conn-read-buffers
 
 `--mysql-server-pool-conn-read-buffers` enables pooling of buffers used to read from incoming
 connections, similar to the way pooling happens for write buffers. Defaults to off.
 
-### VDiff2
+### <a id="vdiff2"/>VDiff2
 
 We introduced the ability to resume a VDiff2 workflow:
 ```
@@ -189,7 +190,7 @@ require transactional guarantees such as consistency or atomicity.
 Until now, you could only specify the caller-id for the security context used to authorize queries.
 As of now, you can specify the principal of the caller, and any groups they belong to.
 
-### Online DDL changes
+### <a id="online-ddl-changes"/>Online DDL changes
 
 #### Concurrent vitess migrations
 
@@ -218,7 +219,7 @@ ALTER VITESS_MIGRATION COMPLETE ALL
 
 This works on all pending migrations (`queued`, `ready`, `running`) and internally issues a `ALTER VITESS_MIGRATION '<uuid>' COMPLETE` for each one. The command is useful for completing multiple concurrent migrations (see above) that are open-ended (`--postpone-completion`).
 
-### Tablet throttler
+### <a id="tablet-throttler"/>Tablet throttler
 
 #### API changes
 
@@ -240,7 +241,7 @@ $ curl -s http://127.0.0.1:15100/debug/vars | jq . | grep Throttler
   "ThrottlerProbesTotal": 74,
 ```
 
-### Mysql Compatibility
+### <a id="mysql-compatibility"/>Mysql Compatibility
 
 #### System Settings
 Vitess supported system settings from release 7.0 onwards, but it was always with a pinch of salt.
@@ -259,20 +260,20 @@ In future releases, we will make this flag enabled by default.
 Lookup vindexes now support a new parameter `multi_shard_autocommit`. If this is set to `true`, lookup vindex dml queries will be sent as autocommit to all shards instead of being wrapped in a transaction.
 This is different from the existing `autocommit` parameter where the query is sent in its own transaction separate from the ongoing transaction if any i.e. begin -> lookup query execs -> commit/rollback
 
-### Durability Policy
+### <a id="durability-policy"/>Durability Policy
 
 #### Cross Cell
 
 A new durability policy `cross_cell` is now supported. `cross_cell` durability policy only allows replica tablets from a different cell than the current primary to
 send semi-sync ACKs. This ensures that any committed write exists in at least 2 tablets belonging to different cells.
 
-### New EXPLAIN format
+### <a id="new-explain-format"/>New EXPLAIN format
 
 #### FORMAT=vtexplain
 
 With this new `explain` format, you can get an output that is very similar to the command line `vtexplain` app, but from a running `vtgate`, through a MySQL query.
 
-### VTOrc
+### <a id="vtorc"/>VTOrc
 
 #### Old UI Removal and Replacement
 
diff --git a/go/vt/vtgate/planbuilder/rewrite.go b/go/vt/vtgate/planbuilder/rewrite.go
index d1b6a789a1..284ffa9c07 100644
--- a/go/vt/vtgate/planbuilder/rewrite.go
+++ b/go/vt/vtgate/planbuilder/rewrite.go
@@ -182,12 +182,8 @@ func rewriteHavingClause(node *sqlparser.Select) {
 					return false
 				}
 				originalExpr, isInMap := selectExprMap[x.Name.Lowered()]
-				if isInMap {
-					if sqlparser.ContainsAggregation(originalExpr) {
-						hasAggr = true
-					} else {
-						cursor.Replace(originalExpr)
-					}
+				if isInMap && sqlparser.ContainsAggregation(originalExpr) {
+					hasAggr = true
 				}
 				return false
 			default:
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 b6230ae234..3f7d0ea26c 100644
--- a/go/vt/vtgate/planbuilder/testdata/filter_cases.json
+++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.json
@@ -4128,6 +4128,7 @@
     }
   },
   {
+    "comment": "select * from samecolvin where col = :col",
     "query": "select * from samecolvin where col = :col",
     "v3-plan": {
       "QueryType": "SELECT",
@@ -6383,5 +6384,42 @@
         "user.music"
       ]
     }
+  },
+  {
+    "comment": "Self referencing columns in HAVING should work",
+    "query": "select a+2 as a from user having a = 42",
+    "v3-plan": {
+      "QueryType": "SELECT",
+      "Original": "select a+2 as a from user having a = 42",
+      "Instructions": {
+        "OperatorType": "Route",
+        "Variant": "Scatter",
+        "Keyspace": {
+          "Name": "user",
+          "Sharded": true
+        },
+        "FieldQuery": "select a + 2 as a from `user` where 1 != 1",
+        "Query": "select a + 2 as a from `user` having a = 42",
+        "Table": "`user`"
+      }
+    },
+    "gen4-plan": {
+      "QueryType": "SELECT",
+      "Original": "select a+2 as a from user having a = 42",
+      "Instructions": {
+        "OperatorType": "Route",
+        "Variant": "Scatter",
+        "Keyspace": {
+          "Name": "user",
+          "Sharded": true
+        },
+        "FieldQuery": "select a + 2 as a from `user` where 1 != 1",
+        "Query": "select a + 2 as a from `user` where a + 2 = 42",
+        "Table": "`user`"
+      },
+      "TablesUsed": [
+        "user.user"
+      ]
+    }
   }
 ]
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())
+}