Repository: vitessio/vitess
Pull request #11540 (2022-10-20 07:18:44)

Redact bind variables in mysql errors
## Description

This PR redacts the bindvars in the errors that are produced by MySQL. For instance, a duplicate key error coming from MySQL will no longer contain the bind variables.

## 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/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)
 


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",