Merge #68101 #68141

68101: roachtest: rudimentary support for secure clusters r=stevendanna,richardjcai a=tbg

TL;DR: roachtest can now start a secure cluster and `c.Conn` will "just work".

- roachprod: improve impl of pgurl
- roachtest: support using Conn() against secure clusters
- roachtest: remove unused method from Cluster interface
- roachtest: remove ad-hoc methods for secure clusters


68141: sql, opt: support hint to disallow zigzag join, support bounded staleness checks r=rytaft a=rytaft

**opt,sql: support hint to disallow zigzag join**

Release note (sql change): Added support for a new index hint,
`NO_ZIGZAG_JOIN`, which will prevent the optimizer from planning a
zigzag join for the specified table. The hint can be used in the
same way as other existing index hints. For example,
`SELECT * FROM table_name@{NO_ZIGZAG_JOIN};`.

**sql,opt: pass tree.SemaContext to the execbuilder**

This commit updates the execbuilder to include the `tree.SemaContext`
as one of its arguments. This will allow it to use the `AsOfSystemTime`
information in a following commit.

Release note: None

**opt: add execbuilder checks for bounded staleness queries**

This commit adds checks in the execbuilder to ensure that bounded staleness
can only be used with queries that touch at most one row. It also applies
hints for such queries in the optbuilder to ensure that an invalid plan is
not produced. In particular, the hints ensure that no plans with an index
join or zigzag join will be produced.

Fixes #67558

Release note: None

Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Rebecca Taft <[email protected]>

docs/generated/sql/bnf/stmt_block.bnf

@@ -976,6 +976,7 @@ unreserved_keyword ::=
 	| 'NO'
 	| 'NORMAL'
 	| 'NO_INDEX_JOIN'
+	| 'NO_ZIGZAG_JOIN'
 	| 'NOCREATEDB'
 	| 'NOCREATELOGIN'
 	| 'NOCANCELQUERY'
@@ -2752,6 +2753,7 @@ materialize_clause ::=
 index_flags_param ::=
 	'FORCE_INDEX' '=' index_name
 	| 'NO_INDEX_JOIN'
+	| 'NO_ZIGZAG_JOIN'
 
 opt_asc_desc ::=
 	'ASC'

pkg/ccl/logictestccl/testdata/logic_test/as_of

@@ -1,7 +1,7 @@
 # LogicTest: local
 
 statement ok
-CREATE TABLE t (i INT)
+CREATE TABLE t (i INT PRIMARY KEY, j INT UNIQUE, k INT, UNIQUE (k) STORING (j))
 
 statement ok
 INSERT INTO t VALUES (2)
@@ -114,29 +114,164 @@ true
 statement error interval duration for with_max_staleness must be greater or equal to 0
 SELECT with_max_staleness(-'1s')
 
-statement ok
+#
+# Tests for optimizer bounded staleness checks.
+#
+
+statement error unimplemented: cannot use bounded staleness for queries that may touch more than one range or require an index join
 SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms')
 
-statement ok
+statement error unimplemented: cannot use bounded staleness for queries that may touch more than one range or require an index join
 SELECT * FROM t AS OF SYSTEM TIME with_min_timestamp(statement_timestamp() - '1ms')
 
+statement error unimplemented: cannot use bounded staleness for MERGE JOIN
+SELECT * FROM t AS t1 JOIN t AS t2 ON t1.i = t2.i AS OF SYSTEM TIME with_max_staleness('1ms')
+
+statement error unimplemented: cannot use bounded staleness for INNER JOIN
+SELECT * FROM t AS t1 INNER HASH JOIN t AS t2 ON t1.i = t2.i AS OF SYSTEM TIME with_min_timestamp(statement_timestamp() - '1ms')
+
+statement error unimplemented: cannot use bounded staleness for LOOKUP JOIN
+SELECT * FROM t AS t1 LEFT LOOKUP JOIN t AS t2 ON t1.i = t2.i AS OF SYSTEM TIME with_max_staleness('1ms')
+
+statement error unimplemented: cannot use bounded staleness for UNION
+SELECT * FROM (SELECT * FROM t UNION SELECT * FROM t) AS OF SYSTEM TIME with_max_staleness('1ms')
+
+statement error unimplemented: cannot use bounded staleness for INTERSECT ALL
+SELECT * FROM (SELECT * FROM t INTERSECT ALL SELECT * FROM t) AS OF SYSTEM TIME with_min_timestamp(statement_timestamp() - '1ms')
+
+statement ok
+SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE i = 2
+
+statement ok
+SELECT * FROM t AS OF SYSTEM TIME with_min_timestamp(statement_timestamp() - '1ms') WHERE i = 1
+
+# Projections are supported.
+statement ok
+SELECT i+2 FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE i = 1
+
+# Select is supported.
+statement ok
+SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE i = 2 AND j > 5
+
+# Aggregations are supported.
+statement ok
+SELECT sum(i) FROM t AS OF SYSTEM TIME with_min_timestamp(statement_timestamp() - '1ms') WHERE i = 2
+
+# Scan from a secondary index is supported.
+statement ok
+SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE k = 2
+
+# Scan from a secondary index is not supported if it requires an index join.
+statement error unimplemented: cannot use bounded staleness for queries that may touch more than one range or require an index join
+SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE j = 2
+
+# No index join or zigzag join is produced.
+query T
+EXPLAIN (OPT, MEMO) SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE j = 2 AND i = 1
+----
+memo (optimized, ~7KB, required=[presentation: info:6])
+ ├── G1: (explain G2 [presentation: i:1,j:2,k:3])
+ │    └── [presentation: info:6]
+ │         ├── best: (explain G2="[presentation: i:1,j:2,k:3]" [presentation: i:1,j:2,k:3])
+ │         └── cost: 5.17
+ ├── G2: (select G3 G4) (select G5 G6)
+ │    └── [presentation: i:1,j:2,k:3]
+ │         ├── best: (select G5 G6)
+ │         └── cost: 5.16
+ ├── G3: (scan t,cols=(1-3)) (scan t@t_k_key,cols=(1-3))
+ │    └── []
+ │         ├── best: (scan t,cols=(1-3))
+ │         └── cost: 1146.41
+ ├── G4: (filters G7 G8)
+ ├── G5: (scan t,cols=(1-3),constrained)
+ │    └── []
+ │         ├── best: (scan t,cols=(1-3),constrained)
+ │         └── cost: 5.13
+ ├── G6: (filters G7)
+ ├── G7: (eq G9 G10)
+ ├── G8: (eq G11 G12)
+ ├── G9: (variable j)
+ ├── G10: (const 2)
+ ├── G11: (variable i)
+ └── G12: (const 1)
+select
+ ├── scan t
+ │    ├── constraint: /1: [/1 - /1]
+ │    └── flags: no-index-join no-zigzag-join
+ └── filters
+      └── j = 2
+
+# Scan may produce multiple rows.
+statement error unimplemented: cannot use bounded staleness for queries that may touch more than one range or require an index join
+SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE k IS NULL
+
+# Scan may produce multiple rows.
+statement error unimplemented: cannot use bounded staleness for queries that may touch more than one range or require an index join
+SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE k IS NULL LIMIT 10
+
+# Even though the scan is limited to 1 row, from KV's perspective, this is a
+# multi-row scan with a limit. That means that the scan can span multiple
+# ranges, but we expect it to short-circuit once it hits the first row. In
+# practice, we expect that to very often be in the first range we hit, but
+# there's no guarantee of that - we could have empty ranges.
+statement error unimplemented: cannot use bounded staleness for queries that may touch more than one range or require an index join
+SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE k IS NULL LIMIT 1
+
+# Subquery contains the only scan, so it succeeds.
+statement ok
+SELECT (SELECT k FROM t WHERE i = 1) FROM generate_series(1, 100) AS OF SYSTEM TIME with_max_staleness('1ms')
+
+# Subquery does not scan data, so it succeeds.
+statement ok
+SELECT (SELECT random()) FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE k = 1
+
+# Subqueries that perform an additional scan are not supported.
+statement error unimplemented: cannot use bounded staleness for queries that may touch more than one range or require an index join
+SELECT (SELECT k FROM t WHERE i = 1) FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE k = 1
+
+# Bounded staleness function must match outer query if used in subquery.
+statement ok
+SELECT (
+  SELECT k FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE i = 1
+) FROM generate_series(1, 100) AS OF SYSTEM TIME with_max_staleness('1ms')
+
+# Bounded staleness function must match outer query if used in subquery.
+statement error unimplemented: cannot specify AS OF SYSTEM TIME with different timestamps
+SELECT (
+  SELECT k FROM t AS OF SYSTEM TIME with_max_staleness('2ms') WHERE i = 1
+) FROM generate_series(1, 100) AS OF SYSTEM TIME with_max_staleness('1ms')
+
+# Bounded staleness function must match outer query if used in subquery.
+statement error AS OF SYSTEM TIME must be provided on a top-level statement
+SELECT (
+  SELECT k FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE i = 1
+) FROM generate_series(1, 100)
+
+#
+# Tests for nearest_only argument.
+#
+
 statement error with_max_staleness: expected bool argument for nearest_only
 SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms', 5)
 
 statement error with_min_timestamp: expected bool argument for nearest_only
 SELECT * FROM t AS OF SYSTEM TIME with_min_timestamp(statement_timestamp(), 5)
 
 statement ok
-SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms', false)
+SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms', false) WHERE i = 2
 
 statement ok
-SELECT * FROM t AS OF SYSTEM TIME with_min_timestamp(statement_timestamp() - '1ms', false)
+SELECT * FROM t AS OF SYSTEM TIME with_min_timestamp(statement_timestamp() - '1ms', false) WHERE i = 2
 
 statement ok
-SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms', true)
+SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms', true) WHERE i = 2
 
 statement ok
-SELECT * FROM t AS OF SYSTEM TIME with_min_timestamp(statement_timestamp() - '1ms', true)
+SELECT * FROM t AS OF SYSTEM TIME with_min_timestamp(statement_timestamp() - '1ms', true) WHERE i = 2
+
+#
+# Tests for running bounded staleness queries in an explicit transaction.
+#
 
 statement error AS OF SYSTEM TIME: only constant expressions or follower_read_timestamp are allowed
 BEGIN AS OF SYSTEM TIME with_max_staleness('1ms')
@@ -147,6 +282,10 @@ BEGIN; SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms')
 statement ok
 ROLLBACK
 
+#
+# Tests for bounded staleness with prepared statements.
+#
+
 statement error bounded staleness queries do not yet work with prepared statements
 PREPARE prep_stmt AS SELECT * FROM t AS OF SYSTEM TIME with_min_timestamp(statement_timestamp() - '10s'::interval)
 

pkg/cmd/roachprod/install/cluster_synced.go

@@ -68,6 +68,7 @@ type SyncedCluster struct {
 	// all other fields are populated in newCluster.
 	Nodes          []int
 	Secure         bool
+	CertsDir       string
 	Env            string
 	Args           []string
 	Tag            string
@@ -894,6 +895,7 @@ rm -fr certs
 mkdir -p certs
 %[1]s cert create-ca --certs-dir=certs --ca-key=certs/ca.key
 %[1]s cert create-client root --certs-dir=certs --ca-key=certs/ca.key
+%[1]s cert create-client testuser --certs-dir=certs --ca-key=certs/ca.key
 %[1]s cert create-node localhost %[2]s --certs-dir=certs --ca-key=certs/ca.key
 tar cvf certs.tar certs
 `, cockroachNodeBinary(c, 1), strings.Join(nodeNames, " "))

pkg/cmd/roachprod/install/cockroach.go

@@ -14,6 +14,7 @@ import (
 	_ "embed" // required for go:embed
 	"fmt"
 	"log"
+	"net/url"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -265,15 +266,21 @@ func (Cockroach) CertsDir(c *SyncedCluster, index int) string {
 
 // NodeURL implements the ClusterImpl.NodeDir interface.
 func (Cockroach) NodeURL(c *SyncedCluster, host string, port int) string {
-	url := fmt.Sprintf("'postgres://root@%s:%d", host, port)
+	var u url.URL
+	u.User = url.User("root")
+	u.Scheme = "postgres"
+	u.Host = fmt.Sprintf("%s:%d", host, port)
+	v := url.Values{}
 	if c.Secure {
-		url += "?sslcert=certs%2Fclient.root.crt&sslkey=certs%2Fclient.root.key&" +
-			"sslrootcert=certs%2Fca.crt&sslmode=verify-full"
+		v.Add("sslcert", c.CertsDir+"/client.root.crt")
+		v.Add("sslkey", c.CertsDir+"/client.root.key")
+		v.Add("sslrootcert", c.CertsDir+"/ca.crt")
+		v.Add("sslmode", "verify-full")
 	} else {
-		url += "?sslmode=disable"
+		v.Add("sslmode", "disable")
 	}
-	url += "'"
-	return url
+	u.RawQuery = v.Encode()
+	return "'" + u.String() + "'"
 }
 
 // NodePort implements the ClusterImpl.NodeDir interface.

pkg/cmd/roachprod/main.go

@@ -96,6 +96,7 @@ var (
 	nodeArgs          []string
 	tag               string
 	external          = false
+	certsDir          string
 	adminurlOpen      = false
 	adminurlPath      = ""
 	adminurlIPs       = false
@@ -181,6 +182,7 @@ Available clusters:
 	}
 	c.Nodes = nodes
 	c.Secure = secure
+	c.CertsDir = certsDir
 	c.Env = strings.Join(nodeEnv, " ")
 	c.Args = nodeArgs
 	if tag != "" {
@@ -1943,6 +1945,8 @@ func main() {
 
 	pgurlCmd.Flags().BoolVar(
 		&external, "external", false, "return pgurls for external connections")
+	pgurlCmd.Flags().StringVar(
+		&certsDir, "certs-dir", "./certs", "cert dir to use for secure connections")
 
 	pprofCmd.Flags().DurationVar(
 		&pprofOptions.duration, "duration", 30*time.Second, "Duration of profile to capture")

pkg/cmd/roachtest/cluster.go

@@ -16,6 +16,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"io/fs"
 	"io/ioutil"
 	"math/rand"
 	"net"
@@ -624,8 +625,11 @@ type clusterImpl struct {
 	// l is the logger used to log various cluster operations.
 	// DEPRECATED for use outside of cluster methods: Use a test's t.L() instead.
 	// This is generally set to the current test's logger.
-	l          *logger.Logger
-	expiration time.Time
+	l *logger.Logger
+	// localCertsDir is a local copy of the certs for this cluster. If this is empty,
+	// the cluster is running in insecure mode.
+	localCertsDir string
+	expiration    time.Time
 	// encryptDefault is true if the cluster should default to having encryption
 	// at rest enabled. The default only applies if encryption is not explicitly
 	// enabled or disabled by options passed to Start.
@@ -1749,7 +1753,36 @@ func (c *clusterImpl) StartE(ctx context.Context, opts ...option.Option) error {
 			}
 		}
 	}
-	return execCmd(ctx, c.l, args...)
+	if err := execCmd(ctx, c.l, args...); err != nil {
+		return err
+	}
+	if argExists(args, "--secure") {
+		var err error
+		c.localCertsDir, err = ioutil.TempDir("", "roachtest-certs")
+		if err != nil {
+			return err
+		}
+		// `roachprod get` behaves differently with `--local` depending on whether
+		// the target dir exists. With `--local`, it'll put the files into the
+		// existing dir. Without `--local`, it'll create a new subdir to house the
+		// certs. Bypass that distinction (which should be fixed independently, but
+		// that might cause fallout) by using a non-existing dir here.
+		c.localCertsDir = filepath.Join(c.localCertsDir, "certs")
+		// Get the certs from the first node.
+		if err := c.Get(ctx, c.l, "./certs", c.localCertsDir, c.Node(1)); err != nil {
+			return err
+		}
+		// Need to prevent world readable files or lib/pq will complain.
+		if err := filepath.Walk(c.localCertsDir, func(path string, info fs.FileInfo, err error) error {
+			if info.IsDir() {
+				return nil
+			}
+			return os.Chmod(path, 0600)
+		}); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 // Start is like StartE() except that it will fatal the test on error.
@@ -1979,6 +2012,9 @@ func (c *clusterImpl) pgURLErr(
 	if external {
 		args = append(args, `--external`)
 	}
+	if c.localCertsDir != "" {
+		args = append(args, "--secure", "--certs-dir", c.localCertsDir)
+	}
 	nodes := c.MakeNodes(node)
 	args = append(args, nodes)
 	cmd := execCmdEx(ctx, c.l, args...)
@@ -2020,23 +2056,6 @@ func (c *clusterImpl) ExternalPGUrl(
 	return c.pgURLErr(ctx, node, true /* external */)
 }
 
-// ExternalPGUrlSecure returns the external Postgres endpoint for the specified
-// nodes.
-func (c *clusterImpl) ExternalPGUrlSecure(
-	ctx context.Context, node option.NodeListOption, user string, certsDir string, port int,
-) ([]string, error) {
-	urlTemplate := "postgres://%s@%s:%d?sslcert=%s/client.%s.crt&sslkey=%s/client.%s.key&sslrootcert=%s/ca.crt&sslmode=require"
-	ips, err := c.ExternalIP(ctx, node)
-	if err != nil {
-		return nil, err
-	}
-	var urls []string
-	for _, ip := range ips {
-		urls = append(urls, fmt.Sprintf(urlTemplate, user, ip, port, certsDir, user, certsDir, user, certsDir))
-	}
-	return urls, nil
-}
-
 func addrToAdminUIAddr(c *clusterImpl, addr string) (string, error) {
 	host, port, err := net.SplitHostPort(addr)
 	if err != nil {
@@ -2225,21 +2244,6 @@ func (c *clusterImpl) ConnE(ctx context.Context, node int) (*gosql.DB, error) {
 	return db, nil
 }
 
-// ConnSecure returns a secure SQL connection to the specified node.
-func (c *clusterImpl) ConnSecure(
-	ctx context.Context, node int, user string, certsDir string, port int,
-) (*gosql.DB, error) {
-	urls, err := c.ExternalPGUrlSecure(ctx, c.Node(node), user, certsDir, port)
-	if err != nil {
-		return nil, err
-	}
-	db, err := gosql.Open("postgres", urls[0])
-	if err != nil {
-		return nil, err
-	}
-	return db, nil
-}
-
 func (c *clusterImpl) MakeNodes(opts ...option.Option) string {
 	var r option.NodeListOption
 	for _, o := range opts {