Merge cockroachdb#95674

95674: sql: assorted fixes for bugs found via TestRandomSyntaxGeneration r=stevendanna a=knz

Fixes cockroachdb#95612.

See the individual commits for details.

Co-authored-by: Raphael 'kena' Poss <[email protected]>

pkg/ccl/logictestccl/testdata/logic_test/tenant

@@ -53,3 +53,9 @@ SELECT * FROM crdb_internal.cluster_queries
 
 statement ok
 SELECT * FROM crdb_internal.cluster_contention_events
+
+subtest regression_95612
+
+statement ok
+PREPARE pstmt AS ALTER TENANT 'foo' COMPLETE REPLICATION TO LATEST
+

pkg/sql/parser/testdata/alter_tenant

@@ -190,3 +190,12 @@ ALTER TENANT 'foo' RENAME TO bar
 ALTER TENANT ('foo') RENAME TO (bar) -- fully parenthesized
 ALTER TENANT '_' RENAME TO bar -- literals removed
 ALTER TENANT 'foo' RENAME TO _ -- identifiers removed
+
+# Regression test for #95612
+parse
+ALTER TENANT INTERVAL 'string' MINUTE RESET CLUSTER SETTING ident
+----
+ALTER TENANT ('string'::INTERVAL MINUTE) SET CLUSTER SETTING ident = DEFAULT -- normalized!
+ALTER TENANT ((('string')::INTERVAL MINUTE)) SET CLUSTER SETTING ident = (DEFAULT) -- fully parenthesized
+ALTER TENANT ('_'::INTERVAL MINUTE) SET CLUSTER SETTING ident = DEFAULT -- literals removed
+ALTER TENANT ('string'::INTERVAL MINUTE) SET CLUSTER SETTING ident = DEFAULT -- identifiers removed

pkg/sql/parser/testdata/backup_restore

@@ -855,3 +855,12 @@ DETAIL: source SQL:
 RESTORE ROLE foo, bar FROM 'baz'
              ^
 HINT: try \h RESTORE
+
+# Regression test for #95612
+parse
+BACKUP INTO LATEST IN UNLOGGED WITH OPTIONS ( DETACHED = FALSE )
+----
+BACKUP INTO LATEST IN 'unlogged' WITH detached = FALSE -- normalized!
+BACKUP INTO LATEST IN ('unlogged') WITH detached = FALSE -- fully parenthesized
+BACKUP INTO LATEST IN '_' WITH detached = FALSE -- literals removed
+BACKUP INTO LATEST IN 'unlogged' WITH detached = FALSE -- identifiers removed

pkg/sql/sem/tree/alter_tenant.go

@@ -55,12 +55,67 @@ type TenantSpec struct {
 	All    bool
 }
 
+// alreadyDelimitedAsSyntacticDExpr is an interface that marks
+// Expr types for which there is never an ambiguity when
+// the expression syntax is followed by a non-reserved
+// keyword. When this property is true, that expression
+// can be pretty-printed without enclosing parentheses in
+// a context followed by more non-reserved keywords, and
+// result in syntax that is still unambiguous.
+// That is, given an expression E and an arbitrary following
+// word X, the syntax "E X" is always unambiguously parsed
+// as "(E) X".
+//
+// This property is obviously true of "atomic" expressions such as
+// string and number literals, and also obviously true of
+// well-enclosed expressions "(...)" / "[...]". However, it is not
+// always true of other composite expression types. For example,
+// "A::B" (CastExpr) is not well-delimited because there are
+// identifiers/keywords such that "A::B C" can be parsed as "A::(B
+// C)". Consider "'a'::INTERVAL" and the non-reserved keyword
+// "MINUTE".
+//
+// This property is closely related to the d_expr syntactic rule in
+// the grammar, hence its name. *Approximately* the expression types
+// produced by the d_expr rule tend to exhibit the "well-delimited"
+// property. However, this is not a proper equivalence: certain Expr
+// types are _also_ produced by other parsing rules than d_expr, so
+// inspection of the contents of the Expr object is necessary to
+// determine whether it is well-delimited or not (for example, some
+// FuncExpr objects are well-delimited, and others are not).
+// Therefore, it is not generally correct to assign the property to
+// all the d_expr expression *types*. We can only do so for a few
+// types for which we know that *all possible objects* of that type
+// are well-delimited, such as Subquery, NumVal or Placeholder.
+type alreadyDelimitedAsSyntacticDExpr interface {
+	Expr
+	alreadyDelimitedAsSyntacticDExpr()
+}
+
+func (*UnresolvedName) alreadyDelimitedAsSyntacticDExpr() {}
+func (*ParenExpr) alreadyDelimitedAsSyntacticDExpr()      {}
+func (*Subquery) alreadyDelimitedAsSyntacticDExpr()       {}
+func (*Placeholder) alreadyDelimitedAsSyntacticDExpr()    {}
+func (*NumVal) alreadyDelimitedAsSyntacticDExpr()         {}
+func (*StrVal) alreadyDelimitedAsSyntacticDExpr()         {}
+func (dNull) alreadyDelimitedAsSyntacticDExpr()           {}
+
 // Format implements the NodeFormatter interface.
 func (n *TenantSpec) Format(ctx *FmtCtx) {
 	if n.All {
 		ctx.WriteString("ALL")
 	} else if n.IsName {
+		// Beware to enclose the expression within parentheses if it is
+		// not a simple identifier and is not already enclosed in
+		// parentheses.
+		_, canOmitParentheses := n.Expr.(alreadyDelimitedAsSyntacticDExpr)
+		if !canOmitParentheses {
+			ctx.WriteByte('(')
+		}
 		ctx.FormatNode(n.Expr)
+		if !canOmitParentheses {
+			ctx.WriteByte(')')
+		}
 	} else {
 		ctx.WriteByte('[')
 		ctx.FormatNode(n.Expr)

pkg/sql/sem/tree/walk.go

@@ -994,7 +994,7 @@ func (n *AlterTenantReplication) walkStmt(v Visitor) Statement {
 		}
 		ret.TenantSpec = ts
 	}
-	if n.Cutover != nil {
+	if n.Cutover != nil && n.Cutover.Timestamp != nil {
 		e, changed := WalkExpr(v, n.Cutover.Timestamp)
 		if changed {
 			if ret == n {