Repository: vitessio/vitess
Pull request #11541 (2022-10-20 08:55:10)

[15.0] Add VTGate debug/status page link to VTAdmin
<!--
  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. -->

In the VTAdmin vtctld page, we link to the vtctld dashboard. For this purpose the `fqdn` field in the discovery configuration. We can do the same for the vtgate page which currently doesn't link to anything.
This PR makes this change. Now adding a `fqdn` field to the vtgate configuration will allow VTAdmin to link to the debug/status page of vtgate.
The page looks like the following with the hyperlink - 
![Screenshot 2022-10-20 at 2 19 19 PM](https://user-images.githubusercontent.com/35839558/196903528-110f8f73-0b28-4dac-aa1b-cb7b789f4646.png)


## 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/examples/local/vtadmin/discovery.json b/examples/local/vtadmin/discovery.json
index def7dd50f8..92e20fb40d 100644
--- a/examples/local/vtadmin/discovery.json
+++ b/examples/local/vtadmin/discovery.json
@@ -10,6 +10,7 @@
     "vtgates": [
         {
             "host": {
+                "fqdn": "localhost:15001",
                 "hostname": "localhost:15991"
             }
         }
diff --git a/examples/operator/101_initial_cluster.yaml b/examples/operator/101_initial_cluster.yaml
index 0a375ec040..557e07f4f8 100644
--- a/examples/operator/101_initial_cluster.yaml
+++ b/examples/operator/101_initial_cluster.yaml
@@ -9,7 +9,7 @@ metadata:
 spec:
   images:
     vtctld: vitess/lite:v15.0.0-rc1
-    vtadmin: vitess/vtadmin:latest
+    vtadmin: vitess/vtadmin:v15.0.0-rc1
     vtgate: vitess/lite:v15.0.0-rc1
     vttablet: vitess/lite:v15.0.0-rc1
     vtbackup: vitess/lite:v15.0.0-rc1
@@ -72,15 +72,14 @@ spec:
     durabilityPolicy: none
     turndownPolicy: Immediate
     vitessOrchestrator:
-      configSecret:
-        name: example-cluster-config
-        key: orc_config.json
       resources:
         limits:
           memory: 128Mi
         requests:
           cpu: 100m
           memory: 128Mi
+      extraFlags:
+        recovery-period-block-duration: 5s
     partitionings:
     - equal:
         parts: 1
@@ -242,12 +241,3 @@ stringData:
       subjects: ["*"]
       clusters:
         - "local"
-  orc_config.json: |
-    {
-      "Debug": true,
-      "MySQLTopologyUser": "orc_client_user",
-      "MySQLTopologyPassword": "orc_client_user_password",
-      "MySQLReplicaUser": "vt_repl",
-      "MySQLReplicaPassword": "",
-      "RecoveryPeriodBlockSeconds": 5
-    }
diff --git a/examples/operator/201_customer_tablets.yaml b/examples/operator/201_customer_tablets.yaml
index b5c2761ed6..e28270fb60 100644
--- a/examples/operator/201_customer_tablets.yaml
+++ b/examples/operator/201_customer_tablets.yaml
@@ -5,7 +5,7 @@ metadata:
 spec:
   images:
     vtctld: vitess/lite:v15.0.0-rc1
-    vtadmin: vitess/vtadmin:latest
+    vtadmin: vitess/vtadmin:v15.0.0-rc1
     vtgate: vitess/lite:v15.0.0-rc1
     vttablet: vitess/lite:v15.0.0-rc1
     vtbackup: vitess/lite:v15.0.0-rc1
@@ -68,15 +68,14 @@ spec:
     durabilityPolicy: none
     turndownPolicy: Immediate
     vitessOrchestrator:
-      configSecret:
-        name: example-cluster-config
-        key: orc_config.json
       resources:
         limits:
           memory: 128Mi
         requests:
           cpu: 100m
           memory: 128Mi
+      extraFlags:
+        recovery-period-block-duration: 5s
     partitionings:
     - equal:
         parts: 1
diff --git a/examples/operator/302_new_shards.yaml b/examples/operator/302_new_shards.yaml
index 84fb9e6610..a3b055c28f 100644
--- a/examples/operator/302_new_shards.yaml
+++ b/examples/operator/302_new_shards.yaml
@@ -5,7 +5,7 @@ metadata:
 spec:
   images:
     vtctld: vitess/lite:v15.0.0-rc1
-    vtadmin: vitess/vtadmin:latest
+    vtadmin: vitess/vtadmin:v15.0.0-rc1
     vtgate: vitess/lite:v15.0.0-rc1
     vttablet: vitess/lite:v15.0.0-rc1
     vtbackup: vitess/lite:v15.0.0-rc1
@@ -68,15 +68,14 @@ spec:
     durabilityPolicy: none
     turndownPolicy: Immediate
     vitessOrchestrator:
-      configSecret:
-        name: example-cluster-config
-        key: orc_config.json
       resources:
         limits:
           memory: 128Mi
         requests:
           cpu: 100m
           memory: 128Mi
+      extraFlags:
+        recovery-period-block-duration: 5s
     partitionings:
     - equal:
         parts: 1
diff --git a/examples/operator/306_down_shard_0.yaml b/examples/operator/306_down_shard_0.yaml
index 6d49bc6ab9..c3f3bb856d 100644
--- a/examples/operator/306_down_shard_0.yaml
+++ b/examples/operator/306_down_shard_0.yaml
@@ -5,7 +5,7 @@ metadata:
 spec:
   images:
     vtctld: vitess/lite:v15.0.0-rc1
-    vtadmin: vitess/vtadmin:latest
+    vtadmin: vitess/vtadmin:v15.0.0-rc1
     vtgate: vitess/lite:v15.0.0-rc1
     vttablet: vitess/lite:v15.0.0-rc1
     vtbackup: vitess/lite:v15.0.0-rc1
@@ -68,15 +68,14 @@ spec:
     durabilityPolicy: none
     turndownPolicy: Immediate
     vitessOrchestrator:
-      configSecret:
-        name: example-cluster-config
-        key: orc_config.json
       resources:
         limits:
           memory: 128Mi
         requests:
           cpu: 100m
           memory: 128Mi
+      extraFlags:
+        recovery-period-block-duration: 5s
     partitionings:
     - equal:
         parts: 1
diff --git a/examples/operator/operator.yaml b/examples/operator/operator.yaml
index a3bda99723..70c23d5a86 100644
--- a/examples/operator/operator.yaml
+++ b/examples/operator/operator.yaml
@@ -2450,6 +2450,7 @@ spec:
                               properties:
                                 parts:
                                   format: int32
+                                  maximum: 65536
                                   minimum: 1
                                   type: integer
                                 shardTemplate:
@@ -2801,17 +2802,6 @@ spec:
                             additionalProperties:
                               type: string
                             type: object
-                          configSecret:
-                            properties:
-                              key:
-                                type: string
-                              name:
-                                type: string
-                              volumeName:
-                                type: string
-                            required:
-                              - key
-                            type: object
                           extraEnv:
                             items:
                               properties:
@@ -2936,8 +2926,6 @@ spec:
                             x-kubernetes-preserve-unknown-fields: true
                           tolerations:
                             x-kubernetes-preserve-unknown-fields: true
-                        required:
-                          - configSecret
                         type: object
                     required:
                       - name
@@ -4002,6 +3990,7 @@ spec:
                         properties:
                           parts:
                             format: int32
+                            maximum: 65536
                             minimum: 1
                             type: integer
                           shardTemplate:
@@ -4387,17 +4376,6 @@ spec:
                       additionalProperties:
                         type: string
                       type: object
-                    configSecret:
-                      properties:
-                        key:
-                          type: string
-                        name:
-                          type: string
-                        volumeName:
-                          type: string
-                      required:
-                        - key
-                      type: object
                     extraEnv:
                       items:
                         properties:
@@ -4522,8 +4500,6 @@ spec:
                       x-kubernetes-preserve-unknown-fields: true
                     tolerations:
                       x-kubernetes-preserve-unknown-fields: true
-                  required:
-                    - configSecret
                   type: object
                 zoneMap:
                   additionalProperties:
@@ -5258,17 +5234,6 @@ spec:
                       additionalProperties:
                         type: string
                       type: object
-                    configSecret:
-                      properties:
-                        key:
-                          type: string
-                        name:
-                          type: string
-                        volumeName:
-                          type: string
-                      required:
-                        - key
-                      type: object
                     extraEnv:
                       items:
                         properties:
@@ -5393,8 +5358,6 @@ spec:
                       x-kubernetes-preserve-unknown-fields: true
                     tolerations:
                       x-kubernetes-preserve-unknown-fields: true
-                  required:
-                    - configSecret
                   type: object
                 zoneMap:
                   additionalProperties:
diff --git a/examples/region_sharding/vtadmin/discovery.json b/examples/region_sharding/vtadmin/discovery.json
index def7dd50f8..92e20fb40d 100644
--- a/examples/region_sharding/vtadmin/discovery.json
+++ b/examples/region_sharding/vtadmin/discovery.json
@@ -10,6 +10,7 @@
     "vtgates": [
         {
             "host": {
+                "fqdn": "localhost:15001",
                 "hostname": "localhost:15991"
             }
         }
diff --git a/go/vt/vtgate/planbuilder/rewrite.go b/go/vt/vtgate/planbuilder/rewrite.go
index 284ffa9c07..4ff3edd9fd 100644
--- a/go/vt/vtgate/planbuilder/rewrite.go
+++ b/go/vt/vtgate/planbuilder/rewrite.go
@@ -174,29 +174,40 @@ func rewriteHavingClause(node *sqlparser.Select) {
 	exprs := sqlparser.SplitAndExpression(nil, node.Having.Expr)
 	node.Having = nil
 	for _, expr := range exprs {
-		var hasAggr bool
-		sqlparser.Rewrite(expr, func(cursor *sqlparser.Cursor) bool {
-			switch x := cursor.Node().(type) {
-			case *sqlparser.ColName:
-				if !x.Qualifier.IsEmpty() {
-					return false
-				}
-				originalExpr, isInMap := selectExprMap[x.Name.Lowered()]
-				if isInMap && sqlparser.ContainsAggregation(originalExpr) {
-					hasAggr = true
-				}
-				return false
-			default:
-				_, isAggregate := x.(sqlparser.AggrFunc)
-				hasAggr = hasAggr || isAggregate
-			}
-			return true
-		}, nil)
-
+		hasAggr := sqlparser.ContainsAggregation(expr)
+		if !hasAggr {
+			sqlparser.Rewrite(expr, func(cursor *sqlparser.Cursor) bool {
+				visitColName(cursor.Node(), selectExprMap, func(original sqlparser.Expr) {
+					if sqlparser.ContainsAggregation(original) {
+						hasAggr = true
+					}
+				})
+				return true
+			}, nil)
+		}
 		if hasAggr {
 			node.AddHaving(expr)
 		} else {
+			sqlparser.Rewrite(expr, func(cursor *sqlparser.Cursor) bool {
+				visitColName(cursor.Node(), selectExprMap, func(original sqlparser.Expr) {
+					cursor.Replace(original)
+				})
+				return true
+			}, nil)
 			node.AddWhere(expr)
 		}
 	}
 }
+func visitColName(cursor sqlparser.SQLNode, selectExprMap map[string]sqlparser.Expr, f func(original sqlparser.Expr)) {
+	switch x := cursor.(type) {
+	case *sqlparser.ColName:
+		if !x.Qualifier.IsEmpty() {
+			return
+		}
+		originalExpr, isInMap := selectExprMap[x.Name.Lowered()]
+		if isInMap {
+			f(originalExpr)
+		}
+		return
+	}
+}
diff --git a/go/vt/vtgate/semantics/early_rewriter.go b/go/vt/vtgate/semantics/early_rewriter.go
index 6f006efd41..7b9f5cdb49 100644
--- a/go/vt/vtgate/semantics/early_rewriter.go
+++ b/go/vt/vtgate/semantics/early_rewriter.go
@@ -128,7 +128,19 @@ func rewriteHavingAndOrderBy(cursor *sqlparser.Cursor, node sqlparser.SQLNode) {
 					continue
 				}
 				if ae.As.Equal(col.Name) {
-					inner.Replace(ae.Expr)
+					safeToRewrite := true
+					sqlparser.Rewrite(ae.Expr, func(cursor *sqlparser.Cursor) bool {
+						switch cursor.Node().(type) {
+						case *sqlparser.ColName:
+							safeToRewrite = false
+						case sqlparser.AggrFunc:
+							return false
+						}
+						return true
+					}, nil)
+					if safeToRewrite {
+						inner.Replace(ae.Expr)
+					}
 				}
 			}
 		}
diff --git a/go/vt/vtgate/vtgate.go b/go/vt/vtgate/vtgate.go
index 8d969f4ab9..de3fba2fbe 100644
--- a/go/vt/vtgate/vtgate.go
+++ b/go/vt/vtgate/vtgate.go
@@ -20,9 +20,11 @@ package vtgate
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"os"
+	"regexp"
 	"strings"
 	"time"
 
@@ -588,6 +590,12 @@ func recordAndAnnotateError(err error, statsKey []string, request map[string]any
 		ec.String(),
 	}
 
+	if terseErrors {
+		regexpBv := regexp.MustCompile(`BindVars: \{.*\}`)
+		str := regexpBv.ReplaceAllString(err.Error(), "BindVars: {REDACTED}")
+		err = errors.New(str)
+	}
+
 	// Traverse the request structure and truncate any long values
 	request = truncateErrorStrings(request)
 
diff --git a/web/vtadmin/src/components/routes/Gates.tsx b/web/vtadmin/src/components/routes/Gates.tsx
index 354d4507a3..5e0f0fefd2 100644
--- a/web/vtadmin/src/components/routes/Gates.tsx
+++ b/web/vtadmin/src/components/routes/Gates.tsx
@@ -41,6 +41,7 @@ export const Gates = () => {
             hostname: g.hostname,
             keyspaces: g.keyspaces,
             pool: g.pool,
+            fqdn: g.FQDN,
         }));
         const filtered = filterNouns(filter, mapped);
         return orderBy(filtered, ['cluster', 'pool', 'hostname', 'cell']);
@@ -53,7 +54,17 @@ export const Gates = () => {
                     <div>{gate.pool}</div>
                     <div className="text-sm text-secondary">{gate.cluster}</div>
                 </DataCell>
-                <DataCell className="whitespace-nowrap">{gate.hostname}</DataCell>
+                <DataCell className="whitespace-nowrap">
+                    {gate.fqdn ? (
+                        <div className="font-bold">
+                            <a href={`//${gate.fqdn}`} rel="noopener noreferrer" target="_blank">
+                                {gate.hostname}
+                            </a>
+                        </div>
+                    ) : (
+                        gate.hostname
+                    )}
+                </DataCell>
                 <DataCell className="whitespace-nowrap">{gate.cell}</DataCell>
                 <DataCell>{(gate.keyspaces || []).join(', ')}</DataCell>
             </tr>


Test patch:
diff --git a/go/test/endtoend/vtgate/queries/dml/insert_test.go b/go/test/endtoend/vtgate/queries/dml/insert_test.go
index 22ca01d32b..d7c904dee3 100644
--- a/go/test/endtoend/vtgate/queries/dml/insert_test.go
+++ b/go/test/endtoend/vtgate/queries/dml/insert_test.go
@@ -384,3 +384,13 @@ func TestInsertSelectUnshardedUsingSharded(t *testing.T) {
 		})
 	}
 }
+
+func TestRedactDupError(t *testing.T) {
+	mcmp, closer := start(t)
+	defer closer()
+
+	mcmp.Exec("insert into order_tbl(region_id, oid, cust_no) values (1,1,100),(1,2,200),(1,3,300)")
+
+	// inserting same rows, throws error.
+	mcmp.AssertContainsError("insert into order_tbl(region_id, oid, cust_no) select region_id, oid, cust_no from order_tbl", `BindVars: {REDACTED}`)
+}
diff --git a/go/test/endtoend/vtgate/queries/dml/main_test.go b/go/test/endtoend/vtgate/queries/dml/main_test.go
index de3c7897ae..7fb361837f 100644
--- a/go/test/endtoend/vtgate/queries/dml/main_test.go
+++ b/go/test/endtoend/vtgate/queries/dml/main_test.go
@@ -98,6 +98,8 @@ func TestMain(m *testing.M) {
 			return 1
 		}
 
+		clusterInstance.VtGateExtraArgs = append(clusterInstance.VtGateExtraArgs, "--vtgate-config-terse-errors")
+
 		// Start vtgate
 		clusterInstance.VtGatePlannerVersion = planbuilder.Gen4
 		err = clusterInstance.StartVtgate()
diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.json b/go/vt/vtgate/planbuilder/testdata/filter_cases.json
index e9ec88d47c..5ba0fea900 100644
--- a/go/vt/vtgate/planbuilder/testdata/filter_cases.json
+++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.json
@@ -6380,5 +6380,79 @@
         "user.user"
       ]
     }
+  },
+  {
+    "comment": "HAVING predicates that use table columns are safe to rewrite if we can move them to the WHERE clause",
+    "query": "select user.col + 2 as a from user having a = 42",
+    "v3-plan": {
+      "QueryType": "SELECT",
+      "Original": "select user.col + 2 as a from user having a = 42",
+      "Instructions": {
+        "OperatorType": "Route",
+        "Variant": "Scatter",
+        "Keyspace": {
+          "Name": "user",
+          "Sharded": true
+        },
+        "FieldQuery": "select `user`.col + 2 as a from `user` where 1 != 1",
+        "Query": "select `user`.col + 2 as a from `user` having a = 42",
+        "Table": "`user`"
+      }
+    },
+    "gen4-plan": {
+      "QueryType": "SELECT",
+      "Original": "select user.col + 2 as a from user having a = 42",
+      "Instructions": {
+        "OperatorType": "Route",
+        "Variant": "Scatter",
+        "Keyspace": {
+          "Name": "user",
+          "Sharded": true
+        },
+        "FieldQuery": "select `user`.col + 2 as a from `user` where 1 != 1",
+        "Query": "select `user`.col + 2 as a from `user` where `user`.col + 2 = 42",
+        "Table": "`user`"
+      },
+      "TablesUsed": [
+        "user.user"
+      ]
+    }
+  },
+  {
+    "comment": "HAVING predicates that use table columns should not get rewritten on unsharded keyspaces",
+    "query": "select col + 2 as a from unsharded having a = 42",
+    "v3-plan": {
+      "QueryType": "SELECT",
+      "Original": "select col + 2 as a from unsharded having a = 42",
+      "Instructions": {
+        "OperatorType": "Route",
+        "Variant": "Unsharded",
+        "Keyspace": {
+          "Name": "main",
+          "Sharded": false
+        },
+        "FieldQuery": "select col + 2 as a from unsharded where 1 != 1",
+        "Query": "select col + 2 as a from unsharded having a = 42",
+        "Table": "unsharded"
+      }
+    },
+    "gen4-plan": {
+      "QueryType": "SELECT",
+      "Original": "select col + 2 as a from unsharded having a = 42",
+      "Instructions": {
+        "OperatorType": "Route",
+        "Variant": "Unsharded",
+        "Keyspace": {
+          "Name": "main",
+          "Sharded": false
+        },
+        "FieldQuery": "select col + 2 as a from unsharded where 1 != 1",
+        "Query": "select col + 2 as a from unsharded having a = 42",
+        "Table": "unsharded"
+      },
+      "TablesUsed": [
+        "main.unsharded"
+      ]
+    }
   }
 ]
diff --git a/go/vt/vtgate/semantics/early_rewriter_test.go b/go/vt/vtgate/semantics/early_rewriter_test.go
index 9b40227af9..509126efc2 100644
--- a/go/vt/vtgate/semantics/early_rewriter_test.go
+++ b/go/vt/vtgate/semantics/early_rewriter_test.go
@@ -145,7 +145,7 @@ func TestExpandStar(t *testing.T) {
 		expSQL: "select t1.b as b, t1.a as a, t1.c as c, t5.a as a from t1 join t5 where t1.b = t5.b",
 	}, {
 		sql:    "select * from t1 join t5 using (b) having b = 12",
-		expSQL: "select t1.b as b, t1.a as a, t1.c as c, t5.a as a from t1 join t5 where t1.b = t5.b having t1.b = 12",
+		expSQL: "select t1.b as b, t1.a as a, t1.c as c, t5.a as a from t1 join t5 where t1.b = t5.b having b = 12",
 	}, {
 		sql:    "select 1 from t1 join t5 using (b) having b = 12",
 		expSQL: "select 1 from t1 join t5 where t1.b = t5.b having t1.b = 12",