Repository: vitessio/vitess
Pull request #11056 (2022-08-21 09:07:54)

Use available method to compare tables
We already have a method that compares while ignoring the name, so use that here too. This avoids a copy and custom logic here.

Testing on a large schema shows that this reduces allocations from over 2 million to around 200k, removing the largest allocation in diff computations for large schemas.

Moving to embedding the pointer instead of the struct removes another set of cases where we copy the object unnecessary. 

## Related Issue(s)

#10203

## 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

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, review whether it is really needed. The flag names should be clear and intuitive (as far as possible), and the flag's help should be descriptive. Additionally, flag names should use dashes (`-`) as word separators rather than underscores (`_`).
- [ ] If a workflow is added or modified, each items in `Jobs` should be named in order to mark it as `required`. If the workflow should be required, the GitHub Admin should be notified.

#### Bug fixes
- [ ] There should be at least one unit or end-to-end test.
- [ ] The Pull Request description should either include a link to an issue that describes the bug OR an actual description of the bug and how to reproduce, along with a description of the fix.

#### 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.


Patch:
diff --git a/go/flags/endtoend/vtctld.txt b/go/flags/endtoend/vtctld.txt
index 6e51bf570c..e3c18f1fac 100644
--- a/go/flags/endtoend/vtctld.txt
+++ b/go/flags/endtoend/vtctld.txt
@@ -278,7 +278,7 @@ Usage of vtctld:
       --vreplication_healthcheck_timeout duration                        healthcheck retry delay (default 1m0s)
       --vreplication_healthcheck_topology_refresh duration               refresh interval for re-reading the topology (default 30s)
       --vreplication_heartbeat_update_interval int                       Frequency (in seconds, default 1, max 60) at which the time_updated column of a vreplication stream when idling (default 1)
-      --vreplication_max_time_to_retry_on_error duration                 stop automatically retrying when we've had consecutive failures with the same error for this long after the first occurrence (default 15m0s)
+      --vreplication_max_time_to_retry_on_error duration                 stop automatically retrying when we've had consecutive failures with the same error for this long after the first occurrence (default 0s)
       --vreplication_replica_lag_tolerance duration                      Replica lag threshold duration: once lag is below this we switch from copy phase to the replication (streaming) phase (default 1m0s)
       --vreplication_retry_delay duration                                delay before retrying a failed workflow event in the replication phase (default 5s)
       --vreplication_store_compressed_gtid                               Store compressed gtids in the pos column of _vt.vreplication
diff --git a/go/flags/endtoend/vtexplain.txt b/go/flags/endtoend/vtexplain.txt
index eceb3cb92e..f292904df3 100644
--- a/go/flags/endtoend/vtexplain.txt
+++ b/go/flags/endtoend/vtexplain.txt
@@ -267,7 +267,7 @@ Usage of vtexplain:
       --vreplication_healthcheck_timeout duration                        healthcheck retry delay (default 1m0s)
       --vreplication_healthcheck_topology_refresh duration               refresh interval for re-reading the topology (default 30s)
       --vreplication_heartbeat_update_interval int                       Frequency (in seconds, default 1, max 60) at which the time_updated column of a vreplication stream when idling (default 1)
-      --vreplication_max_time_to_retry_on_error duration                 stop automatically retrying when we've had consecutive failures with the same error for this long after the first occurrence (default 15m0s)
+      --vreplication_max_time_to_retry_on_error duration                 stop automatically retrying when we've had consecutive failures with the same error for this long after the first occurrence (default 0s)
       --vreplication_replica_lag_tolerance duration                      Replica lag threshold duration: once lag is below this we switch from copy phase to the replication (streaming) phase (default 1m0s)
       --vreplication_retry_delay duration                                delay before retrying a failed workflow event in the replication phase (default 5s)
       --vreplication_store_compressed_gtid                               Store compressed gtids in the pos column of _vt.vreplication
diff --git a/go/flags/endtoend/vttablet.txt b/go/flags/endtoend/vttablet.txt
index 287cce1d98..e0dca3c6b5 100644
--- a/go/flags/endtoend/vttablet.txt
+++ b/go/flags/endtoend/vttablet.txt
@@ -460,7 +460,7 @@ Usage of vttablet:
       --vreplication_healthcheck_timeout duration                        healthcheck retry delay (default 1m0s)
       --vreplication_healthcheck_topology_refresh duration               refresh interval for re-reading the topology (default 30s)
       --vreplication_heartbeat_update_interval int                       Frequency (in seconds, default 1, max 60) at which the time_updated column of a vreplication stream when idling (default 1)
-      --vreplication_max_time_to_retry_on_error duration                 stop automatically retrying when we've had consecutive failures with the same error for this long after the first occurrence (default 15m0s)
+      --vreplication_max_time_to_retry_on_error duration                 stop automatically retrying when we've had consecutive failures with the same error for this long after the first occurrence (default 0s)
       --vreplication_replica_lag_tolerance duration                      Replica lag threshold duration: once lag is below this we switch from copy phase to the replication (streaming) phase (default 1m0s)
       --vreplication_retry_delay duration                                delay before retrying a failed workflow event in the replication phase (default 5s)
       --vreplication_store_compressed_gtid                               Store compressed gtids in the pos column of _vt.vreplication
diff --git a/go/tools/graphviz/graph.go b/go/tools/graphviz/graph.go
new file mode 100644
index 0000000000..b3b12a33ea
--- /dev/null
+++ b/go/tools/graphviz/graph.go
@@ -0,0 +1,205 @@
+/*
+Copyright 2022 The Vitess Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package graphviz
+
+import (
+	"fmt"
+	"os"
+	"os/exec"
+	"runtime"
+	"strconv"
+	"strings"
+)
+
+type (
+	Graph struct {
+		lastID int
+		nodes  []*Node
+		edges  []*Edge
+	}
+	Node struct {
+		id      int
+		Name    string
+		attrs   []string
+		tooltip string
+	}
+	Edge struct {
+		From, To *Node
+	}
+)
+
+func escape(s string) string {
+	s = strings.ReplaceAll(s, "<", "\\<")
+	s = strings.ReplaceAll(s, ">", "\\>")
+	s = strings.ReplaceAll(s, "\"", "\\\"")
+	s = strings.ReplaceAll(s, "|", "\\|")
+	s = strings.ReplaceAll(s, "{", "\\{")
+	s = strings.ReplaceAll(s, "}", "\\}")
+	return s
+}
+
+func (n *Node) AddAttribute(s string) {
+	n.attrs = append(n.attrs, escape(s))
+}
+func (n *Node) AddTooltip(s string) {
+	n.tooltip = escape(s)
+}
+
+func (g *Graph) produceDot() string {
+	var dot strings.Builder
+	dot.WriteString(`digraph {
+node [shape=record, fontsize=10]
+`)
+	for _, node := range g.nodes {
+		labels := node.Name
+		for i, attr := range node.attrs {
+			if i == 0 {
+				labels += "|{" + attr
+			} else {
+				labels += "|" + attr
+			}
+
+		}
+		labels += "}"
+		if node.tooltip != "" {
+			dot.WriteString(fmt.Sprintf(`n%d [label="%s", tooltip="%s"]`, node.id, labels, node.tooltip))
+		} else {
+			dot.WriteString(fmt.Sprintf(`n%d [label="%s"]`, node.id, labels))
+		}
+		dot.WriteString(";\n")
+	}
+	for _, edge := range g.edges {
+		dot.WriteString(fmt.Sprintf(`n%d -> n%d`, edge.From.id, edge.To.id))
+		dot.WriteString(";\n")
+	}
+	dot.WriteString("}")
+	return dot.String()
+}
+
+func (g *Graph) AddNode(name string) *Node {
+	n := &Node{
+		id:   g.lastID,
+		Name: name,
+	}
+	g.lastID++
+	g.nodes = append(g.nodes, n)
+	return n
+}
+
+func (g *Graph) AddEdge(from, to *Node) *Edge {
+	e := &Edge{
+		From: from,
+		To:   to,
+	}
+	g.edges = append(g.edges, e)
+	return e
+}
+
+const htmlTemplate = `
+<!DOCTYPE html>
+<html>
+<head>
+    <meta charset="UTF-8">
+    <title>GraphViz Viewer</title>
+    <script src="https://cdn.jsdelivr.net/npm/@hpcc-js/wasm/dist/index.min.js"></script>
+	<script src="https://cdn.jsdelivr.net/npm/svg-pan-zoom/dist/svg-pan-zoom.min.js"></script>
+    <script>
+        var hpccWasm = window["@hpcc-js/wasm"];
+    </script>
+	<style>
+		#graph svg {
+			width: 90vw;
+			height: 90vh;
+			border-width: 1px;
+			border-style: dotted;
+		}
+	</style>
+</head>
+<body>
+    <div id="graph"></div>
+    <script>
+		const dot = %s;
+        hpccWasm.graphviz.layout(dot, "svg", "dot").then(svg => {
+            const div = document.getElementById("graph");
+            div.innerHTML = svg;
+			svgPanZoom(div.querySelector('svg'), { controlIconsEnabled: true });
+        });
+    </script>
+</body>
+</html>
+`
+
+func (g *Graph) Render() error {
+
+	dot := g.produceDot()
+
+	browsers := func() []string {
+		var cmds []string
+		if userBrowser := os.Getenv("BROWSER"); userBrowser != "" {
+			cmds = append(cmds, userBrowser)
+		}
+		switch runtime.GOOS {
+		case "darwin":
+			cmds = append(cmds, "/usr/bin/open")
+		case "windows":
+			cmds = append(cmds, "cmd /c start")
+		default:
+			// Commands opening browsers are prioritized over xdg-open, so browser()
+			// command can be used on linux to open the .svg file generated by the -web
+			// command (the .svg file includes embedded javascript so is best viewed in
+			// a browser).
+			cmds = append(cmds, []string{"chrome", "google-chrome", "chromium", "firefox", "sensible-browser"}...)
+			if os.Getenv("DISPLAY") != "" {
+				// xdg-open is only for use in a desktop environment.
+				cmds = append(cmds, "xdg-open")
+			}
+		}
+		return cmds
+	}
+
+	tmpfile, err := os.CreateTemp("", "graphviz_*.html")
+	if err != nil {
+		return err
+	}
+
+	_, err = fmt.Fprintf(tmpfile, htmlTemplate, strconv.Quote(dot))
+	if err != nil {
+		return err
+	}
+	err = tmpfile.Close()
+	if err != nil {
+		return err
+	}
+
+	for _, b := range browsers() {
+		args := strings.Split(b, " ")
+		if len(args) == 0 {
+			continue
+		}
+		viewer := exec.Command(args[0], append(args[1:], tmpfile.Name())...)
+		viewer.Stderr = os.Stderr
+		if err := viewer.Start(); err == nil {
+			return nil
+		}
+	}
+
+	return fmt.Errorf("failed to open browser for SVG debugging")
+}
+
+func New() *Graph {
+	return &Graph{}
+}
diff --git a/go/vt/schemadiff/schema.go b/go/vt/schemadiff/schema.go
index 7c8dd674b3..3cb6f4436b 100644
--- a/go/vt/schemadiff/schema.go
+++ b/go/vt/schemadiff/schema.go
@@ -212,7 +212,7 @@ func (s *Schema) normalize() error {
 				continue
 			}
 			// Not handled. Is this view dependent on already handled objects?
-			dependentNames, err := getViewDependentTableNames(&v.CreateView)
+			dependentNames, err := getViewDependentTableNames(v.CreateView)
 			if err != nil {
 				return err
 			}
@@ -497,7 +497,7 @@ func (s *Schema) apply(diffs []EntityDiff) error {
 			if _, ok := s.named[name]; ok {
 				return &ApplyDuplicateEntityError{Entity: name}
 			}
-			s.tables = append(s.tables, &CreateTableEntity{CreateTable: *diff.createTable})
+			s.tables = append(s.tables, &CreateTableEntity{CreateTable: diff.createTable})
 			_, s.named[name] = diff.Entities()
 		case *CreateViewEntityDiff:
 			// We expect the view to not exist
@@ -505,7 +505,7 @@ func (s *Schema) apply(diffs []EntityDiff) error {
 			if _, ok := s.named[name]; ok {
 				return &ApplyDuplicateEntityError{Entity: name}
 			}
-			s.views = append(s.views, &CreateViewEntity{CreateView: *diff.createView})
+			s.views = append(s.views, &CreateViewEntity{CreateView: diff.createView})
 			_, s.named[name] = diff.Entities()
 		case *DropTableEntityDiff:
 			// We expect the table to exist
diff --git a/go/vt/schemadiff/table.go b/go/vt/schemadiff/table.go
index c2227bdf2a..e9ec2e50f3 100644
--- a/go/vt/schemadiff/table.go
+++ b/go/vt/schemadiff/table.go
@@ -120,7 +120,7 @@ func (d *CreateTableEntityDiff) IsEmpty() bool {
 
 // Entities implements EntityDiff
 func (d *CreateTableEntityDiff) Entities() (from Entity, to Entity) {
-	return nil, &CreateTableEntity{CreateTable: *d.createTable}
+	return nil, &CreateTableEntity{CreateTable: d.createTable}
 }
 
 // Statement implements EntityDiff
@@ -279,14 +279,14 @@ func (d *RenameTableEntityDiff) SetSubsequentDiff(EntityDiff) {
 
 // CreateTableEntity stands for a TABLE construct. It contains the table's CREATE statement.
 type CreateTableEntity struct {
-	sqlparser.CreateTable
+	*sqlparser.CreateTable
 }
 
 func NewCreateTableEntity(c *sqlparser.CreateTable) (*CreateTableEntity, error) {
 	if !c.IsFullyParsed() {
 		return nil, &NotFullyParsedError{Entity: c.Table.Name.String(), Statement: sqlparser.CanonicalString(c)}
 	}
-	entity := &CreateTableEntity{CreateTable: *c}
+	entity := &CreateTableEntity{CreateTable: c}
 	entity.normalize()
 	return entity, nil
 }
@@ -326,7 +326,7 @@ func (c *CreateTableEntity) normalizeTableOptions() {
 }
 
 func (c *CreateTableEntity) Clone() Entity {
-	return &CreateTableEntity{CreateTable: *sqlparser.CloneRefOfCreateTable(&c.CreateTable)}
+	return &CreateTableEntity{CreateTable: sqlparser.CloneRefOfCreateTable(c.CreateTable)}
 }
 
 // Right now we assume MySQL 8.0 for the collation normalization handling.
@@ -615,22 +615,19 @@ func (c *CreateTableEntity) Diff(other Entity, hints *DiffHints) (EntityDiff, er
 // It returns an AlterTable statement if changes are found, or nil if not.
 // the other table may be of different name; its name is ignored.
 func (c *CreateTableEntity) TableDiff(other *CreateTableEntity, hints *DiffHints) (*AlterTableEntityDiff, error) {
-	otherStmt := other.CreateTable
-	otherStmt.Table = c.CreateTable.Table
-
 	if !c.CreateTable.IsFullyParsed() {
-		return nil, &NotFullyParsedError{Entity: c.Name(), Statement: sqlparser.CanonicalString(&c.CreateTable)}
+		return nil, &NotFullyParsedError{Entity: c.Name(), Statement: sqlparser.CanonicalString(c.CreateTable)}
 	}
-	if !otherStmt.IsFullyParsed() {
-		return nil, &NotFullyParsedError{Entity: other.Name(), Statement: sqlparser.CanonicalString(&otherStmt)}
+	if !other.CreateTable.IsFullyParsed() {
+		return nil, &NotFullyParsedError{Entity: other.Name(), Statement: sqlparser.CanonicalString(other.CreateTable)}
 	}
 
-	if sqlparser.EqualsRefOfCreateTable(&c.CreateTable, &otherStmt) {
+	if c.identicalOtherThanName(other) {
 		return nil, nil
 	}
 
 	alterTable := &sqlparser.AlterTable{
-		Table: otherStmt.Table,
+		Table: c.CreateTable.Table,
 	}
 	diffedTableCharset := ""
 	var parentAlterTableEntityDiff *AlterTableEntityDiff
@@ -688,7 +685,7 @@ func (c *CreateTableEntity) TableDiff(other *CreateTableEntity, hints *DiffHints
 	}
 	for _, superfluousFulltextKey := range superfluousFulltextKeys {
 		alterTable := &sqlparser.AlterTable{
-			Table:        otherStmt.Table,
+			Table:        c.CreateTable.Table,
 			AlterOptions: []sqlparser.AlterOption{superfluousFulltextKey},
 		}
 		diff := &AlterTableEntityDiff{alterTable: alterTable, from: c, to: other}
@@ -698,7 +695,7 @@ func (c *CreateTableEntity) TableDiff(other *CreateTableEntity, hints *DiffHints
 	}
 	for _, partitionSpec := range partitionSpecs {
 		alterTable := &sqlparser.AlterTable{
-			Table:         otherStmt.Table,
+			Table:         c.CreateTable.Table,
 			PartitionSpec: partitionSpec,
 		}
 		diff := &AlterTableEntityDiff{alterTable: alterTable, from: c, to: other}
@@ -1477,7 +1474,7 @@ func heuristicallyDetectColumnRenames(
 
 // Create implements Entity interface
 func (c *CreateTableEntity) Create() EntityDiff {
-	return &CreateTableEntityDiff{to: c, createTable: &c.CreateTable}
+	return &CreateTableEntityDiff{to: c, createTable: c.CreateTable}
 }
 
 // Drop implements Entity interface
diff --git a/go/vt/schemadiff/view.go b/go/vt/schemadiff/view.go
index 5a85eabe4b..46cb66dcff 100644
--- a/go/vt/schemadiff/view.go
+++ b/go/vt/schemadiff/view.go
@@ -90,7 +90,7 @@ func (d *CreateViewEntityDiff) IsEmpty() bool {
 
 // Entities implements EntityDiff
 func (d *CreateViewEntityDiff) Entities() (from Entity, to Entity) {
-	return nil, &CreateViewEntity{CreateView: *d.createView}
+	return nil, &CreateViewEntity{CreateView: d.createView}
 }
 
 // Statement implements EntityDiff
@@ -192,14 +192,14 @@ func (d *DropViewEntityDiff) SetSubsequentDiff(EntityDiff) {
 
 // CreateViewEntity stands for a VIEW construct. It contains the view's CREATE statement.
 type CreateViewEntity struct {
-	sqlparser.CreateView
+	*sqlparser.CreateView
 }
 
 func NewCreateViewEntity(c *sqlparser.CreateView) (*CreateViewEntity, error) {
 	if !c.IsFullyParsed() {
 		return nil, &NotFullyParsedError{Entity: c.ViewName.Name.String(), Statement: sqlparser.CanonicalString(c)}
 	}
-	entity := &CreateViewEntity{CreateView: *c}
+	entity := &CreateViewEntity{CreateView: c}
 	entity.normalize()
 	return entity, nil
 }
@@ -234,35 +234,32 @@ func (c *CreateViewEntity) Diff(other Entity, hints *DiffHints) (EntityDiff, err
 // It returns an AlterView statement if changes are found, or nil if not.
 // the other view may be of different name; its name is ignored.
 func (c *CreateViewEntity) ViewDiff(other *CreateViewEntity, _ *DiffHints) (*AlterViewEntityDiff, error) {
-	otherStmt := other.CreateView
-	otherStmt.ViewName = c.CreateView.ViewName
-
-	if !c.CreateView.IsFullyParsed() {
-		return nil, &NotFullyParsedError{Entity: c.Name(), Statement: sqlparser.CanonicalString(&c.CreateView)}
+	if !c.IsFullyParsed() {
+		return nil, &NotFullyParsedError{Entity: c.Name(), Statement: sqlparser.CanonicalString(c.CreateView)}
 	}
-	if !otherStmt.IsFullyParsed() {
-		return nil, &NotFullyParsedError{Entity: c.Name(), Statement: sqlparser.CanonicalString(&otherStmt)}
+	if !other.CreateView.IsFullyParsed() {
+		return nil, &NotFullyParsedError{Entity: c.Name(), Statement: sqlparser.CanonicalString(other.CreateView)}
 	}
 
-	if sqlparser.EqualsRefOfCreateView(&c.CreateView, &otherStmt) {
+	if c.identicalOtherThanName(other) {
 		return nil, nil
 	}
 
 	alterView := &sqlparser.AlterView{
-		ViewName:    otherStmt.ViewName,
-		Algorithm:   otherStmt.Algorithm,
-		Definer:     otherStmt.Definer,
-		Security:    otherStmt.Security,
-		Columns:     otherStmt.Columns,
-		Select:      otherStmt.Select,
-		CheckOption: otherStmt.CheckOption,
+		ViewName:    c.CreateView.ViewName,
+		Algorithm:   other.CreateView.Algorithm,
+		Definer:     other.CreateView.Definer,
+		Security:    other.CreateView.Security,
+		Columns:     other.CreateView.Columns,
+		Select:      other.CreateView.Select,
+		CheckOption: other.CreateView.CheckOption,
 	}
 	return &AlterViewEntityDiff{alterView: alterView, from: c, to: other}, nil
 }
 
 // Create implements Entity interface
 func (c *CreateViewEntity) Create() EntityDiff {
-	return &CreateViewEntityDiff{createView: &c.CreateView}
+	return &CreateViewEntityDiff{createView: c.CreateView}
 }
 
 // Drop implements Entity interface
@@ -296,5 +293,19 @@ func (c *CreateViewEntity) Apply(diff EntityDiff) (Entity, error) {
 }
 
 func (c *CreateViewEntity) Clone() Entity {
-	return &CreateViewEntity{CreateView: *sqlparser.CloneRefOfCreateView(&c.CreateView)}
+	return &CreateViewEntity{CreateView: sqlparser.CloneRefOfCreateView(c.CreateView)}
+}
+
+func (c *CreateViewEntity) identicalOtherThanName(other *CreateViewEntity) bool {
+	if other == nil {
+		return false
+	}
+	return c.Algorithm == other.Algorithm &&
+		c.Security == other.Security &&
+		c.CheckOption == other.CheckOption &&
+		c.IsReplace == other.IsReplace &&
+		sqlparser.EqualsRefOfDefiner(c.Definer, other.Definer) &&
+		sqlparser.EqualsColumns(c.Columns, other.Columns) &&
+		sqlparser.EqualsSelectStatement(c.Select, other.Select) &&
+		sqlparser.EqualsRefOfParsedComments(c.Comments, other.Comments)
 }
diff --git a/go/vt/vtgate/engine/plan_description.go b/go/vt/vtgate/engine/plan_description.go
index 3c84f82776..4433dcd69d 100644
--- a/go/vt/vtgate/engine/plan_description.go
+++ b/go/vt/vtgate/engine/plan_description.go
@@ -19,8 +19,10 @@ package engine
 import (
 	"bytes"
 	"encoding/json"
+	"fmt"
 	"sort"
 
+	"vitess.io/vitess/go/tools/graphviz"
 	"vitess.io/vitess/go/vt/key"
 	topodatapb "vitess.io/vitess/go/vt/proto/topodata"
 	"vitess.io/vitess/go/vt/vtgate/vindexes"
@@ -91,6 +93,54 @@ func (pd PrimitiveDescription) MarshalJSON() ([]byte, error) {
 	return buf.Bytes(), nil
 }
 
+func (pd PrimitiveDescription) addToGraph(g *graphviz.Graph) (*graphviz.Node, error) {
+	var nodes []*graphviz.Node
+	for _, input := range pd.Inputs {
+		n, err := input.addToGraph(g)
+		if err != nil {
+			return nil, err
+		}
+		nodes = append(nodes, n)
+	}
+	name := pd.OperatorType + ":" + pd.Variant
+	if pd.Variant == "" {
+		name = pd.OperatorType
+	}
+	this := g.AddNode(name)
+	for k, v := range pd.Other {
+		switch k {
+		case "Query":
+			this.AddTooltip(fmt.Sprintf("%v", v))
+		case "FieldQuery":
+		// skip these
+		default:
+			slice, ok := v.([]string)
+			if ok {
+				this.AddAttribute(k)
+				for _, s := range slice {
+					this.AddAttribute(s)
+				}
+			} else {
+				this.AddAttribute(fmt.Sprintf("%s:%v", k, v))
+			}
+		}
+	}
+	for _, n := range nodes {
+		g.AddEdge(this, n)
+	}
+	return this, nil
+}
+
+func GraphViz(p Primitive) (*graphviz.Graph, error) {
+	g := graphviz.New()
+	description := PrimitiveToPlanDescription(p)
+	_, err := description.addToGraph(g)
+	if err != nil {
+		return nil, err
+	}
+	return g, nil
+}
+
 func addMap(input map[string]any, buf *bytes.Buffer) error {
 	var mk []string
 	for k, v := range input {
diff --git a/go/vt/vttablet/tabletmanager/vreplication/controller.go b/go/vt/vttablet/tabletmanager/vreplication/controller.go
index 6b9a2c4ddc..4a4ece3638 100644
--- a/go/vt/vttablet/tabletmanager/vreplication/controller.go
+++ b/go/vt/vttablet/tabletmanager/vreplication/controller.go
@@ -48,7 +48,7 @@ var (
 	_          = flag.Duration("vreplication_healthcheck_timeout", 1*time.Minute, "healthcheck retry delay")
 	retryDelay = flag.Duration("vreplication_retry_delay", 5*time.Second, "delay before retrying a failed workflow event in the replication phase")
 
-	maxTimeToRetryError = flag.Duration("vreplication_max_time_to_retry_on_error", 15*time.Minute, "stop automatically retrying when we've had consecutive failures with the same error for this long after the first occurrence")
+	maxTimeToRetryError = flag.Duration("vreplication_max_time_to_retry_on_error", 0, "stop automatically retrying when we've had consecutive failures with the same error for this long after the first occurrence")
 )
 
 // controller is created by Engine. Members are initialized upfront.
diff --git a/go/vt/vttablet/tabletmanager/vreplication/last_error.go b/go/vt/vttablet/tabletmanager/vreplication/last_error.go
index a3080973c9..e3a73d5b06 100644
--- a/go/vt/vttablet/tabletmanager/vreplication/last_error.go
+++ b/go/vt/vttablet/tabletmanager/vreplication/last_error.go
@@ -62,10 +62,18 @@ func (le *lastError) record(err error) {
 func (le *lastError) shouldRetry() bool {
 	le.mu.Lock()
 	defer le.mu.Unlock()
-	if !le.firstSeen.IsZero() && time.Since(le.firstSeen) > le.maxTimeInError {
-		log.Errorf("VReplication encountered the same error continuously since %s, we will assume this is a non-recoverable error and will not retry anymore; the workflow will need to be manually restarted once error '%s' has been addressed",
-			le.firstSeen.UTC(), le.err)
-		return false
+	if le.maxTimeInError == 0 {
+		// The value of 0 means "no time limit"
+		return true
 	}
-	return true
+	if le.firstSeen.IsZero() {
+		return true
+	}
+	if time.Since(le.firstSeen) <= le.maxTimeInError {
+		// within the max time range
+		return true
+	}
+	log.Errorf("VReplication encountered the same error continuously since %s, we will assume this is a non-recoverable error and will not retry anymore; the workflow will need to be manually restarted once error '%s' has been addressed",
+		le.firstSeen.UTC(), le.err)
+	return false
 }


Test patch:
diff --git a/go/tools/graphviz/graph_test.go b/go/tools/graphviz/graph_test.go
new file mode 100644
index 0000000000..3f9d7958e6
--- /dev/null
+++ b/go/tools/graphviz/graph_test.go
@@ -0,0 +1,38 @@
+/*
+Copyright 2022 The Vitess Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package graphviz
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestSimple(t *testing.T) {
+	g := New()
+	n1 := g.AddNode("apa")
+	n1.AddAttribute("aåö**|")
+	n1.AddAttribute("value")
+	n2 := g.AddNode("banan")
+	g.AddEdge(n1, n2)
+
+	fmt.Println(g.produceDot())
+
+	err := g.Render()
+	require.NoError(t, err)
+}
diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go
index 3d929e044b..faeca866f1 100644
--- a/go/vt/vtgate/planbuilder/plan_test.go
+++ b/go/vt/vtgate/planbuilder/plan_test.go
@@ -229,28 +229,28 @@ func TestPlan(t *testing.T) {
 	// the column is named as Id. This is to make sure that
 	// column names are case-preserved, but treated as
 	// case-insensitive even if they come from the vschema.
-	testFile(t, "aggr_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "dml_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "from_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "filter_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "postprocess_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "select_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "symtab_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "unsupported_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "vindex_func_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "wireup_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "memory_sort_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "use_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "set_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "union_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "transaction_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "lock_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "large_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "ddl_cases_no_default_keyspace.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "flush_cases_no_default_keyspace.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "show_cases_no_default_keyspace.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "stream_cases.txt", testOutputTempDir, vschemaWrapper)
-	testFile(t, "systemtables_cases.txt", testOutputTempDir, vschemaWrapper)
+	testFile(t, "aggr_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "dml_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "from_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "filter_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "postprocess_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "select_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "symtab_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "unsupported_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "vindex_func_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "wireup_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "memory_sort_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "use_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "set_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "union_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "transaction_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "lock_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "large_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "ddl_cases_no_default_keyspace.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "flush_cases_no_default_keyspace.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "show_cases_no_default_keyspace.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "stream_cases.txt", testOutputTempDir, vschemaWrapper, false)
+	testFile(t, "systemtables_cases.txt", testOutputTempDir, vschemaWrapper, false)
 }
 
 func TestSysVarSetDisabled(t *testing.T) {
@@ -259,7 +259,7 @@ func TestSysVarSetDisabled(t *testing.T) {
 		sysVarEnabled: false,
 	}
 
-	testFile(t, "set_sysvar_disabled_cases.txt", makeTestOutput(t), vschemaWrapper)
+	testFile(t, "set_sysvar_disabled_cases.txt", makeTestOutput(t), vschemaWrapper, false)
 }
 
 func TestOne(t *testing.T) {
@@ -267,7 +267,7 @@ func TestOne(t *testing.T) {
 		v: loadSchema(t, "schema_test.json", true),
 	}
 
-	testFile(t, "onecase.txt", "", vschema)
+	testFile(t, "onecase.txt", "", vschema, true)
 }
 
 func TestOneWithMainAsDefault(t *testing.T) {
@@ -279,7 +279,7 @@ func TestOneWithMainAsDefault(t *testing.T) {
 		},
 	}
 
-	testFile(t, "onecase.txt", "", vschema)
+	testFile(t, "onecase.txt", "", vschema, false)
 }
 
 func TestOneWithSecondUserAsDefault(t *testing.T) {
@@ -291,7 +291,7 @@ func TestOneWithSecondUserAsDefault(t *testing.T) {
 		},
 	}
 
-	testFile(t, "onecase.txt", "", vschema)
+	testFile(t, "onecase.txt", "", vschema, false)
 }
 
 func TestOneWithUserAsDefault(t *testing.T) {
@@ -303,7 +303,7 @@ func TestOneWithUserAsDefault(t *testing.T) {
 		},
 	}
 
-	testFile(t, "onecase.txt", "", vschema)
+	testFile(t, "onecase.txt", "", vschema, false)
 }
 
 func TestRubyOnRailsQueries(t *testing.T) {
@@ -312,7 +312,7 @@ func TestRubyOnRailsQueries(t *testing.T) {
 		sysVarEnabled: true,
 	}
 
-	testFile(t, "rails_cases.txt", makeTestOutput(t), vschemaWrapper)
+	testFile(t, "rails_cases.txt", makeTestOutput(t), vschemaWrapper, false)
 }
 
 func TestOLTP(t *testing.T) {
@@ -321,7 +321,7 @@ func TestOLTP(t *testing.T) {
 		sysVarEnabled: true,
 	}
 
-	testFile(t, "oltp_cases.txt", makeTestOutput(t), vschemaWrapper)
+	testFile(t, "oltp_cases.txt", makeTestOutput(t), vschemaWrapper, false)
 }
 
 func TestTPCC(t *testing.T) {
@@ -330,7 +330,7 @@ func TestTPCC(t *testing.T) {
 		sysVarEnabled: true,
 	}
 
-	testFile(t, "tpcc_cases.txt", makeTestOutput(t), vschemaWrapper)
+	testFile(t, "tpcc_cases.txt", makeTestOutput(t), vschemaWrapper, false)
 }
 
 func TestTPCH(t *testing.T) {
@@ -339,7 +339,7 @@ func TestTPCH(t *testing.T) {
 		sysVarEnabled: true,
 	}
 
-	testFile(t, "tpch_cases.txt", makeTestOutput(t), vschemaWrapper)
+	testFile(t, "tpch_cases.txt", makeTestOutput(t), vschemaWrapper, false)
 }
 
 func BenchmarkOLTP(b *testing.B) {
@@ -382,7 +382,7 @@ func TestBypassPlanningShardTargetFromFile(t *testing.T) {
 		tabletType: topodatapb.TabletType_PRIMARY,
 		dest:       key.DestinationShard("-80")}
 
-	testFile(t, "bypass_shard_cases.txt", makeTestOutput(t), vschema)
+	testFile(t, "bypass_shard_cases.txt", makeTestOutput(t), vschema, false)
 }
 func TestBypassPlanningKeyrangeTargetFromFile(t *testing.T) {
 	keyRange, _ := key.ParseShardingSpec("-")
@@ -397,7 +397,7 @@ func TestBypassPlanningKeyrangeTargetFromFile(t *testing.T) {
 		dest:       key.DestinationExactKeyRange{KeyRange: keyRange[0]},
 	}
 
-	testFile(t, "bypass_keyrange_cases.txt", makeTestOutput(t), vschema)
+	testFile(t, "bypass_keyrange_cases.txt", makeTestOutput(t), vschema, false)
 }
 
 func TestWithDefaultKeyspaceFromFile(t *testing.T) {
@@ -412,12 +412,12 @@ func TestWithDefaultKeyspaceFromFile(t *testing.T) {
 	}
 
 	testOutputTempDir := makeTestOutput(t)
-	testFile(t, "alterVschema_cases.txt", testOutputTempDir, vschema)
-	testFile(t, "ddl_cases.txt", testOutputTempDir, vschema)
-	testFile(t, "migration_cases.txt", testOutputTempDir, vschema)
-	testFile(t, "flush_cases.txt", testOutputTempDir, vschema)
-	testFile(t, "show_cases.txt", testOutputTempDir, vschema)
-	testFile(t, "call_cases.txt", testOutputTempDir, vschema)
+	testFile(t, "alterVschema_cases.txt", testOutputTempDir, vschema, false)
+	testFile(t, "ddl_cases.txt", testOutputTempDir, vschema, false)
+	testFile(t, "migration_cases.txt", testOutputTempDir, vschema, false)
+	testFile(t, "flush_cases.txt", testOutputTempDir, vschema, false)
+	testFile(t, "show_cases.txt", testOutputTempDir, vschema, false)
+	testFile(t, "call_cases.txt", testOutputTempDir, vschema, false)
 }
 
 func TestWithDefaultKeyspaceFromFileSharded(t *testing.T) {
@@ -432,7 +432,7 @@ func TestWithDefaultKeyspaceFromFileSharded(t *testing.T) {
 	}
 
 	testOutputTempDir := makeTestOutput(t)
-	testFile(t, "select_cases_with_default.txt", testOutputTempDir, vschema)
+	testFile(t, "select_cases_with_default.txt", testOutputTempDir, vschema, false)
 }
 
 func TestWithUserDefaultKeyspaceFromFileSharded(t *testing.T) {
@@ -447,7 +447,7 @@ func TestWithUserDefaultKeyspaceFromFileSharded(t *testing.T) {
 	}
 
 	testOutputTempDir := makeTestOutput(t)
-	testFile(t, "select_cases_with_user_as_default.txt", testOutputTempDir, vschema)
+	testFile(t, "select_cases_with_user_as_default.txt", testOutputTempDir, vschema, false)
 }
 
 func TestWithSystemSchemaAsDefaultKeyspace(t *testing.T) {
@@ -458,7 +458,7 @@ func TestWithSystemSchemaAsDefaultKeyspace(t *testing.T) {
 		tabletType: topodatapb.TabletType_PRIMARY,
 	}
 
-	testFile(t, "sysschema_default.txt", makeTestOutput(t), vschema)
+	testFile(t, "sysschema_default.txt", makeTestOutput(t), vschema, false)
 }
 
 func TestOtherPlanningFromFile(t *testing.T) {
@@ -473,8 +473,8 @@ func TestOtherPlanningFromFile(t *testing.T) {
 	}
 
 	testOutputTempDir := makeTestOutput(t)
-	testFile(t, "other_read_cases.txt", testOutputTempDir, vschema)
-	testFile(t, "other_admin_cases.txt", testOutputTempDir, vschema)
+	testFile(t, "other_read_cases.txt", testOutputTempDir, vschema, false)
+	testFile(t, "other_admin_cases.txt", testOutputTempDir, vschema, false)
 }
 
 func loadSchema(t testing.TB, filename string, setCollation bool) *vindexes.VSchema {
@@ -692,7 +692,7 @@ func (vw *vschemaWrapper) currentDb() string {
 	return ksName
 }
 
-func testFile(t *testing.T, filename, tempDir string, vschema *vschemaWrapper) {
+func testFile(t *testing.T, filename, tempDir string, vschema *vschemaWrapper, render bool) {
 	t.Run(filename, func(t *testing.T) {
 		expected := &strings.Builder{}
 		var outFirstPlanner string
@@ -700,6 +700,12 @@ func testFile(t *testing.T, filename, tempDir string, vschema *vschemaWrapper) {
 			t.Run(fmt.Sprintf("%d V3: %s", tcase.lineno, tcase.comments), func(t *testing.T) {
 				vschema.version = V3
 				plan, err := TestBuilder(tcase.input, vschema, vschema.currentDb())
+				if render && plan != nil {
+					viz, err := engine.GraphViz(plan.Instructions)
+					if err == nil {
+						_ = viz.Render()
+					}
+				}
 				out := getPlanOrErrorOutput(err, plan)
 
 				if out != tcase.output {
@@ -717,7 +723,7 @@ func testFile(t *testing.T, filename, tempDir string, vschema *vschemaWrapper) {
 			})
 
 			vschema.version = Gen4
-			out, err := getPlanOutput(tcase, vschema)
+			out, err := getPlanOutput(tcase, vschema, render)
 			if err != nil && tcase.output2ndPlanner == "" && strings.HasPrefix(err.Error(), "gen4 does not yet support") {
 				expected.WriteString("\n")
 				continue
@@ -760,13 +766,19 @@ func testFile(t *testing.T, filename, tempDir string, vschema *vschemaWrapper) {
 	})
 }
 
-func getPlanOutput(tcase testCase, vschema *vschemaWrapper) (out string, err error) {
+func getPlanOutput(tcase testCase, vschema *vschemaWrapper, render bool) (out string, err error) {
 	defer func() {
 		if r := recover(); r != nil {
 			out = fmt.Sprintf("panicked: %v\n%s", r, string(debug.Stack()))
 		}
 	}()
 	plan, err := TestBuilder(tcase.input, vschema, vschema.currentDb())
+	if render && plan != nil {
+		viz, err := engine.GraphViz(plan.Instructions)
+		if err == nil {
+			_ = viz.Render()
+		}
+	}
 	out = getPlanOrErrorOutput(err, plan)
 	return out, err
 }