From abf84b63ed50194154f5011f666202ad399fbfd8 Mon Sep 17 00:00:00 2001 From: Rebecca Taft Date: Tue, 27 Jul 2021 08:58:46 -0500 Subject: [PATCH 1/8] 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};`. --- docs/generated/sql/bnf/stmt_block.bnf | 2 + .../logictest/testdata/logic_test/zigzag_join | 8 ++- pkg/sql/opt/exec/execbuilder/testdata/join | 20 +++++- pkg/sql/opt/memo/expr.go | 5 +- pkg/sql/opt/memo/expr_format.go | 13 +++- pkg/sql/opt/memo/interner.go | 1 + pkg/sql/opt/optbuilder/select.go | 1 + pkg/sql/opt/xform/select_funcs.go | 9 ++- pkg/sql/opt/xform/testdata/rules/select | 72 ++++++++++++++++--- pkg/sql/parser/sql.y | 9 ++- pkg/sql/parser/testdata/select_clauses | 18 +++-- pkg/sql/sem/tree/select.go | 16 ++++- 12 files changed, 144 insertions(+), 30 deletions(-) diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf index 14923a6f2f9e..18aed974c217 100644 --- a/docs/generated/sql/bnf/stmt_block.bnf +++ b/docs/generated/sql/bnf/stmt_block.bnf @@ -972,6 +972,7 @@ unreserved_keyword ::= | 'NO' | 'NORMAL' | 'NO_INDEX_JOIN' + | 'NO_ZIGZAG_JOIN' | 'NOCREATEDB' | 'NOCREATELOGIN' | 'NOCANCELQUERY' @@ -2734,6 +2735,7 @@ materialize_clause ::= index_flags_param ::= 'FORCE_INDEX' '=' index_name | 'NO_INDEX_JOIN' + | 'NO_ZIGZAG_JOIN' opt_asc_desc ::= 'ASC' diff --git a/pkg/sql/logictest/testdata/logic_test/zigzag_join b/pkg/sql/logictest/testdata/logic_test/zigzag_join index 79c907918b0c..47ee97fa5363 100644 --- a/pkg/sql/logictest/testdata/logic_test/zigzag_join +++ b/pkg/sql/logictest/testdata/logic_test/zigzag_join @@ -31,7 +31,13 @@ SELECT * FROM a WHERE a = 5 AND b = 2 AND c = 'foo' ---- 5 5 2 foo -# Turn off zigzag joins and verify output. +# Turn off zigzag joins and verify output. First with a hint, then with the +# session variable. +query III rowsort +SELECT n,a,b FROM a@{NO_ZIGZAG_JOIN} WHERE a = 4 AND b = 1 +---- +4 4 1 + statement ok SET enable_zigzag_join = false diff --git a/pkg/sql/opt/exec/execbuilder/testdata/join b/pkg/sql/opt/exec/execbuilder/testdata/join index 16e1670de7b2..c2d57e25bca3 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/join +++ b/pkg/sql/opt/exec/execbuilder/testdata/join @@ -2033,7 +2033,25 @@ CREATE TABLE zigzag ( INDEX c_idx(c) ) -# No zigzag join should be planned if enable_zigzag_join is false. +# No zigzag join should be planned if there is a hint or enable_zigzag_join is +# false. +query T +EXPLAIN SELECT a,b,c FROM zigzag@{NO_ZIGZAG_JOIN} WHERE b = 5 AND c = 6.0 +---- +distribution: local +vectorized: true +· +• filter +│ filter: c = 6.0 +│ +└── • index join + │ table: zigzag@primary + │ + └── • scan + missing stats + table: zigzag@b_idx + spans: [/5 - /5] + statement ok SET enable_zigzag_join = false diff --git a/pkg/sql/opt/memo/expr.go b/pkg/sql/opt/memo/expr.go index 6ed981e22fcf..75b6a0efefd5 100644 --- a/pkg/sql/opt/memo/expr.go +++ b/pkg/sql/opt/memo/expr.go @@ -349,6 +349,9 @@ type ScanFlags struct { // this table. NoIndexJoin bool + // NoZigzagJoin disallows use of a zigzag join for scanning this table. + NoZigzagJoin bool + // ForceIndex forces the use of a specific index (specified in Index). // ForceIndex and NoIndexJoin cannot both be set at the same time. ForceIndex bool @@ -358,7 +361,7 @@ type ScanFlags struct { // Empty returns true if there are no flags set. func (sf *ScanFlags) Empty() bool { - return !sf.NoIndexJoin && !sf.ForceIndex + return !sf.NoIndexJoin && !sf.NoZigzagJoin && !sf.ForceIndex } // JoinFlags stores restrictions on the join execution method, derived from diff --git a/pkg/sql/opt/memo/expr_format.go b/pkg/sql/opt/memo/expr_format.go index 03a74fe987f1..07cd3628fec0 100644 --- a/pkg/sql/opt/memo/expr_format.go +++ b/pkg/sql/opt/memo/expr_format.go @@ -397,9 +397,12 @@ func (f *ExprFmtCtx) formatRelational(e RelExpr, tp treeprinter.Node) { tp.Childf("limit: %s", private.HardLimit) } if !private.Flags.Empty() { + var b strings.Builder + b.WriteString("flags:") if private.Flags.NoIndexJoin { - tp.Childf("flags: no-index-join") - } else if private.Flags.ForceIndex { + b.WriteString(" no-index-join") + } + if private.Flags.ForceIndex { idx := md.Table(private.Table).Index(private.Flags.Index) dir := "" switch private.Flags.Direction { @@ -409,8 +412,12 @@ func (f *ExprFmtCtx) formatRelational(e RelExpr, tp treeprinter.Node) { case tree.Descending: dir = ",rev" } - tp.Childf("flags: force-index=%s%s", idx.Name(), dir) + b.WriteString(fmt.Sprintf(" force-index=%s%s", idx.Name(), dir)) + } + if private.Flags.NoZigzagJoin { + b.WriteString(" no-zigzag-join") } + tp.Child(b.String()) } if private.Locking != nil { strength := "" diff --git a/pkg/sql/opt/memo/interner.go b/pkg/sql/opt/memo/interner.go index e54f7759ff5a..e47ac1f87616 100644 --- a/pkg/sql/opt/memo/interner.go +++ b/pkg/sql/opt/memo/interner.go @@ -487,6 +487,7 @@ func (h *hasher) HashScanLimit(val ScanLimit) { func (h *hasher) HashScanFlags(val ScanFlags) { h.HashBool(val.NoIndexJoin) + h.HashBool(val.NoZigzagJoin) h.HashBool(val.ForceIndex) h.HashInt(int(val.Direction)) h.HashUint64(uint64(val.Index)) diff --git a/pkg/sql/opt/optbuilder/select.go b/pkg/sql/opt/optbuilder/select.go index 07c85b0e4be8..2a64d627679b 100644 --- a/pkg/sql/opt/optbuilder/select.go +++ b/pkg/sql/opt/optbuilder/select.go @@ -510,6 +510,7 @@ func (b *Builder) buildScan( private := memo.ScanPrivate{Table: tabID, Cols: scanColIDs} if indexFlags != nil { private.Flags.NoIndexJoin = indexFlags.NoIndexJoin + private.Flags.NoZigzagJoin = indexFlags.NoZigzagJoin if indexFlags.Index != "" || indexFlags.IndexID != 0 { idx := -1 for i := 0; i < tab.IndexCount(); i++ { diff --git a/pkg/sql/opt/xform/select_funcs.go b/pkg/sql/opt/xform/select_funcs.go index 18943be35f49..d6205a80a01d 100644 --- a/pkg/sql/opt/xform/select_funcs.go +++ b/pkg/sql/opt/xform/select_funcs.go @@ -913,20 +913,19 @@ func (c *CustomFuncs) canMaybeConstrainNonInvertedIndex( func (c *CustomFuncs) GenerateZigzagJoins( grp memo.RelExpr, scanPrivate *memo.ScanPrivate, filters memo.FiltersExpr, ) { - tab := c.e.mem.Metadata().Table(scanPrivate.Table) - // Short circuit unless zigzag joins are explicitly enabled. - if !c.e.evalCtx.SessionData.ZigzagJoinEnabled { + if !c.e.evalCtx.SessionData.ZigzagJoinEnabled || scanPrivate.Flags.NoZigzagJoin { return } fixedCols := memo.ExtractConstColumns(filters, c.e.evalCtx) - if fixedCols.Len() < 2 { // Zigzagging requires at least 2 columns to have fixed values. return } + tab := c.e.mem.Metadata().Table(scanPrivate.Table) + // Zigzag joins aren't currently equipped to produce system columns, so // don't generate any if some system columns are requested. foundSystemCol := false @@ -1271,7 +1270,7 @@ func (c *CustomFuncs) GenerateInvertedIndexZigzagJoins( grp memo.RelExpr, scanPrivate *memo.ScanPrivate, filters memo.FiltersExpr, ) { // Short circuit unless zigzag joins are explicitly enabled. - if !c.e.evalCtx.SessionData.ZigzagJoinEnabled { + if !c.e.evalCtx.SessionData.ZigzagJoinEnabled || scanPrivate.Flags.NoZigzagJoin { return } diff --git a/pkg/sql/opt/xform/testdata/rules/select b/pkg/sql/opt/xform/testdata/rules/select index 2391ae624c9f..670aa001025f 100644 --- a/pkg/sql/opt/xform/testdata/rules/select +++ b/pkg/sql/opt/xform/testdata/rules/select @@ -5232,7 +5232,7 @@ CREATE TABLE zz_redundant ( # Simple zigzag case - where all requested columns are in the indexes being # joined. -opt +opt expect=GenerateZigzagJoins SELECT q,r FROM pqr WHERE q = 1 AND r = 2 ---- inner-join (zigzag pqr@q pqr@r) @@ -5245,7 +5245,7 @@ inner-join (zigzag pqr@q pqr@r) ├── q:2 = 1 [outer=(2), constraints=(/2: [/1 - /1]; tight), fd=()-->(2)] └── r:3 = 2 [outer=(3), constraints=(/3: [/2 - /2]; tight), fd=()-->(3)] -opt +opt expect=GenerateZigzagJoins SELECT q,r FROM pqr WHERE q = 1 AND r IS NULL ---- inner-join (zigzag pqr@q pqr@r) @@ -5258,7 +5258,7 @@ inner-join (zigzag pqr@q pqr@r) ├── q:2 = 1 [outer=(2), constraints=(/2: [/1 - /1]; tight), fd=()-->(2)] └── r:3 IS NULL [outer=(3), constraints=(/3: [/NULL - /NULL]; tight), fd=()-->(3)] -memo +memo expect=GenerateZigzagJoins SELECT q,r FROM pqr WHERE q = 1 AND r = 2 ---- memo (optimized, ~13KB, required=[presentation: q:2,r:3]) @@ -5305,7 +5305,7 @@ memo (optimized, ~13KB, required=[presentation: q:2,r:3]) └── G17: (const 2) # Case where the fixed columns are extracted from a complicated expression. -opt +opt expect=GenerateZigzagJoins SELECT q,r FROM pqr WHERE q = 1 AND ((r < 1 AND r > 1) OR (r >= 2 AND r <= 2)) ---- inner-join (zigzag pqr@q pqr@r) @@ -5320,7 +5320,7 @@ inner-join (zigzag pqr@q pqr@r) # Nested zigzag case - zigzag join needs to be wrapped in a lookup join to # satisfy required columns. -opt +opt expect=GenerateZigzagJoins SELECT q,r,s FROM pqr WHERE q = 1 AND r = 2 ---- inner-join (lookup pqr) @@ -5339,7 +5339,7 @@ inner-join (lookup pqr) │ └── r:3 = 2 [outer=(3), constraints=(/3: [/2 - /2]; tight), fd=()-->(3)] └── filters (true) -memo +memo expect=GenerateZigzagJoins SELECT q,r,s FROM pqr WHERE q = 1 AND r = 2 ---- memo (optimized, ~15KB, required=[presentation: q:2,r:3,s:4]) @@ -5391,7 +5391,7 @@ memo (optimized, ~15KB, required=[presentation: q:2,r:3,s:4]) └── G19: (const 2) # Zigzag with fixed columns of different types. -opt +opt expect=GenerateZigzagJoins SELECT q,s FROM pqr WHERE q = 1 AND s = 'foo' ---- inner-join (zigzag pqr@q pqr@s) @@ -5404,7 +5404,7 @@ inner-join (zigzag pqr@q pqr@s) ├── q:2 = 1 [outer=(2), constraints=(/2: [/1 - /1]; tight), fd=()-->(2)] └── s:4 = 'foo' [outer=(4), constraints=(/4: [/'foo' - /'foo']; tight), fd=()-->(4)] -memo +memo expect=GenerateZigzagJoins SELECT q,s FROM pqr WHERE q = 1 AND s = 'foo' ---- memo (optimized, ~11KB, required=[presentation: q:2,s:4]) @@ -5445,7 +5445,7 @@ memo (optimized, ~11KB, required=[presentation: q:2,s:4]) # Zigzag with implicit equality column in addition to primary key: # indexes on (r,s) and (t,s) should be chosen even though s is not being fixed # in the ON clause. -opt +opt expect=GenerateZigzagJoins SELECT r,t FROM pqr WHERE r = 1 AND t = 'foo' ---- inner-join (zigzag pqr@rs pqr@ts) @@ -5458,7 +5458,7 @@ inner-join (zigzag pqr@rs pqr@ts) ├── r:3 = 1 [outer=(3), constraints=(/3: [/1 - /1]; tight), fd=()-->(3)] └── t:5 = 'foo' [outer=(5), constraints=(/5: [/'foo' - /'foo']; tight), fd=()-->(5)] -memo +memo expect=GenerateZigzagJoins SELECT r,t FROM pqr WHERE r = 1 AND t = 'foo' ---- memo (optimized, ~13KB, required=[presentation: r:3,t:5]) @@ -5505,7 +5505,7 @@ memo (optimized, ~13KB, required=[presentation: r:3,t:5]) └── G17: (const 'foo') # Zigzag with choice between indexes for multiple equality predicates. -opt +opt expect=GenerateZigzagJoins SELECT p,q,r,s FROM pqr WHERE q = 1 AND r = 1 AND s = 'foo' ---- inner-join (zigzag pqr@q pqr@s) @@ -5520,6 +5520,25 @@ inner-join (zigzag pqr@q pqr@s) ├── r:3 = 1 [outer=(3), constraints=(/3: [/1 - /1]; tight), fd=()-->(3)] └── s:4 = 'foo' [outer=(4), constraints=(/4: [/'foo' - /'foo']; tight), fd=()-->(4)] +# Zigzag join should not be produced when there is a NO_ZIGZAG_JOIN hint. +opt expect-not=GenerateZigzagJoins +SELECT q,r FROM pqr@{NO_ZIGZAG_JOIN} WHERE q = 1 AND r = 2 +---- +select + ├── columns: q:2!null r:3!null + ├── fd: ()-->(2,3) + ├── index-join pqr + │ ├── columns: q:2 r:3 + │ ├── fd: ()-->(2) + │ └── scan pqr@q + │ ├── columns: p:1!null q:2!null + │ ├── constraint: /2/1: [/1 - /1] + │ ├── flags: no-zigzag-join + │ ├── key: (1) + │ └── fd: ()-->(2) + └── filters + └── r:3 = 2 [outer=(3), constraints=(/3: [/2 - /2]; tight), fd=()-->(3)] + # Tests for zigzag joins over partial indexes. exec-ddl @@ -6253,6 +6272,37 @@ index-join b ├── key: (1) └── fd: (1)-->(9) +# Zigzag join should not be produced when there is a NO_ZIGZAG_JOIN hint. +opt expect-not=GenerateInvertedIndexZigzagJoins +SELECT k FROM b@{NO_ZIGZAG_JOIN} WHERE j @> '{"a": "b", "c": "d"}' +---- +project + ├── columns: k:1!null + ├── immutable + ├── key: (1) + └── inverted-filter + ├── columns: k:1!null + ├── inverted expression: /9 + │ ├── tight: true, unique: true + │ ├── union spans: empty + │ └── INTERSECTION + │ ├── span expression + │ │ ├── tight: true, unique: true + │ │ └── union spans: ["7a\x00\x01\x12b\x00\x01", "7a\x00\x01\x12b\x00\x01"] + │ └── span expression + │ ├── tight: true, unique: true + │ └── union spans: ["7c\x00\x01\x12d\x00\x01", "7c\x00\x01\x12d\x00\x01"] + ├── key: (1) + └── scan b@j_inv_idx + ├── columns: k:1!null j_inverted_key:9!null + ├── inverted constraint: /9/1 + │ └── spans + │ ├── ["7a\x00\x01\x12b\x00\x01", "7a\x00\x01\x12b\x00\x01"] + │ └── ["7c\x00\x01\x12d\x00\x01", "7c\x00\x01\x12d\x00\x01"] + ├── flags: no-zigzag-join + ├── key: (1) + └── fd: (1)-->(9) + exec-ddl CREATE TABLE inv_zz_partial ( k INT PRIMARY KEY, diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y index 85469ada2f86..2601491a346f 100644 --- a/pkg/sql/parser/sql.y +++ b/pkg/sql/parser/sql.y @@ -790,7 +790,7 @@ func (u *sqlSymUnion) alterDefaultPrivilegesTargetObject() tree.AlterDefaultPriv %token MULTIPOLYGON MULTIPOLYGONM MULTIPOLYGONZ MULTIPOLYGONZM %token NAN NAME NAMES NATURAL NEVER NEXT NO NOCANCELQUERY NOCONTROLCHANGEFEED NOCONTROLJOB -%token NOCREATEDB NOCREATELOGIN NOCREATEROLE NOLOGIN NOMODIFYCLUSTERSETTING NO_INDEX_JOIN +%token NOCREATEDB NOCREATELOGIN NOCREATEROLE NOLOGIN NOMODIFYCLUSTERSETTING NO_INDEX_JOIN NO_ZIGZAG_JOIN %token NONE NON_VOTERS NORMAL NOT NOTHING NOTNULL NOVIEWACTIVITY NOWAIT NULL NULLIF NULLS NUMERIC %token OF OFF OFFSET OID OIDS OIDVECTOR ON ONLY OPT OPTION OPTIONS OR @@ -9435,6 +9435,11 @@ index_flags_param: { $$.val = &tree.IndexFlags{NoIndexJoin: true} } +| + NO_ZIGZAG_JOIN + { + $$.val = &tree.IndexFlags{NoZigzagJoin: true} + } | IGNORE_FOREIGN_KEYS { @@ -9499,6 +9504,7 @@ opt_index_flags: // Index flags: // '{' FORCE_INDEX = [, ...] '}' // '{' NO_INDEX_JOIN [, ...] '}' +// '{' NO_ZIGZAG_JOIN [, ...] '}' // '{' IGNORE_FOREIGN_KEYS [, ...] '}' // // Join types: @@ -12939,6 +12945,7 @@ unreserved_keyword: | NO | NORMAL | NO_INDEX_JOIN +| NO_ZIGZAG_JOIN | NOCREATEDB | NOCREATELOGIN | NOCANCELQUERY diff --git a/pkg/sql/parser/testdata/select_clauses b/pkg/sql/parser/testdata/select_clauses index b71ddf1fbe70..37220ab5c80a 100644 --- a/pkg/sql/parser/testdata/select_clauses +++ b/pkg/sql/parser/testdata/select_clauses @@ -318,6 +318,14 @@ SELECT ('a') FROM t@{NO_INDEX_JOIN} -- fully parenthesized SELECT _ FROM t@{NO_INDEX_JOIN} -- literals removed SELECT 'a' FROM _@{NO_INDEX_JOIN} -- identifiers removed +parse +SELECT 'a' FROM t@{NO_ZIGZAG_JOIN} +---- +SELECT 'a' FROM t@{NO_ZIGZAG_JOIN} +SELECT ('a') FROM t@{NO_ZIGZAG_JOIN} -- fully parenthesized +SELECT _ FROM t@{NO_ZIGZAG_JOIN} -- literals removed +SELECT 'a' FROM _@{NO_ZIGZAG_JOIN} -- identifiers removed + parse SELECT 'a' FROM t@{IGNORE_FOREIGN_KEYS} ---- @@ -335,12 +343,12 @@ SELECT _ FROM t@{FORCE_INDEX=idx,ASC} -- literals removed SELECT 'a' FROM _@{FORCE_INDEX=_,ASC} -- identifiers removed parse -SELECT 'a' FROM t@{FORCE_INDEX=idx,DESC,IGNORE_FOREIGN_KEYS} +SELECT 'a' FROM t@{FORCE_INDEX=idx,DESC,IGNORE_FOREIGN_KEYS,NO_ZIGZAG_JOIN} ---- -SELECT 'a' FROM t@{FORCE_INDEX=idx,DESC,IGNORE_FOREIGN_KEYS} -SELECT ('a') FROM t@{FORCE_INDEX=idx,DESC,IGNORE_FOREIGN_KEYS} -- fully parenthesized -SELECT _ FROM t@{FORCE_INDEX=idx,DESC,IGNORE_FOREIGN_KEYS} -- literals removed -SELECT 'a' FROM _@{FORCE_INDEX=_,DESC,IGNORE_FOREIGN_KEYS} -- identifiers removed +SELECT 'a' FROM t@{FORCE_INDEX=idx,DESC,NO_ZIGZAG_JOIN,IGNORE_FOREIGN_KEYS} -- normalized! +SELECT ('a') FROM t@{FORCE_INDEX=idx,DESC,NO_ZIGZAG_JOIN,IGNORE_FOREIGN_KEYS} -- fully parenthesized +SELECT _ FROM t@{FORCE_INDEX=idx,DESC,NO_ZIGZAG_JOIN,IGNORE_FOREIGN_KEYS} -- literals removed +SELECT 'a' FROM _@{FORCE_INDEX=_,DESC,NO_ZIGZAG_JOIN,IGNORE_FOREIGN_KEYS} -- identifiers removed error SELECT a FROM foo@{FORCE_INDEX} diff --git a/pkg/sql/sem/tree/select.go b/pkg/sql/sem/tree/select.go index 1ae1526c471c..e6cd5d593088 100644 --- a/pkg/sql/sem/tree/select.go +++ b/pkg/sql/sem/tree/select.go @@ -268,6 +268,7 @@ type IndexID uint32 // - FORCE_INDEX= // - ASC / DESC // - NO_INDEX_JOIN +// - NO_ZIGZAG_JOIN // - IGNORE_FOREIGN_KEYS // It is used optionally after a table name in SELECT statements. type IndexFlags struct { @@ -278,6 +279,8 @@ type IndexFlags struct { Direction Direction // NoIndexJoin cannot be specified together with an index. NoIndexJoin bool + // NoZigzagJoin indicates we should not plan a zigzag join for this scan. + NoZigzagJoin bool // IgnoreForeignKeys disables optimizations based on outbound foreign key // references from this table. This is useful in particular for scrub queries // used to verify the consistency of foreign key relations. @@ -299,6 +302,9 @@ func (ih *IndexFlags) CombineWith(other *IndexFlags) error { if ih.NoIndexJoin && other.NoIndexJoin { return errors.New("NO_INDEX_JOIN specified multiple times") } + if ih.NoZigzagJoin && other.NoZigzagJoin { + return errors.New("NO_ZIGZAG_JOIN specified multiple times") + } if ih.IgnoreForeignKeys && other.IgnoreForeignKeys { return errors.New("IGNORE_FOREIGN_KEYS specified multiple times") } @@ -307,6 +313,7 @@ func (ih *IndexFlags) CombineWith(other *IndexFlags) error { } result := *ih result.NoIndexJoin = ih.NoIndexJoin || other.NoIndexJoin + result.NoZigzagJoin = ih.NoZigzagJoin || other.NoZigzagJoin result.IgnoreForeignKeys = ih.IgnoreForeignKeys || other.IgnoreForeignKeys result.IgnoreUniqueWithoutIndexKeys = ih.IgnoreUniqueWithoutIndexKeys || other.IgnoreUniqueWithoutIndexKeys @@ -348,8 +355,8 @@ func (ih *IndexFlags) Check() error { // Format implements the NodeFormatter interface. func (ih *IndexFlags) Format(ctx *FmtCtx) { ctx.WriteByte('@') - if !ih.NoIndexJoin && !ih.IgnoreForeignKeys && !ih.IgnoreUniqueWithoutIndexKeys && - ih.Direction == 0 { + if !ih.NoIndexJoin && !ih.NoZigzagJoin && !ih.IgnoreForeignKeys && + !ih.IgnoreUniqueWithoutIndexKeys && ih.Direction == 0 { if ih.Index != "" { ctx.FormatNode(&ih.Index) } else { @@ -379,6 +386,11 @@ func (ih *IndexFlags) Format(ctx *FmtCtx) { ctx.WriteString("NO_INDEX_JOIN") } + if ih.NoZigzagJoin { + sep() + ctx.WriteString("NO_ZIGZAG_JOIN") + } + if ih.IgnoreForeignKeys { sep() ctx.WriteString("IGNORE_FOREIGN_KEYS") From 25b4d57d5438b7c2733a1baf31aed3cd9ea66a39 Mon Sep 17 00:00:00 2001 From: Rebecca Taft Date: Tue, 27 Jul 2021 09:06:40 -0500 Subject: [PATCH 2/8] 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 --- pkg/sql/opt/bench/bench_test.go | 2 ++ pkg/sql/opt/exec/execbuilder/builder.go | 3 +++ pkg/sql/opt/exec/execbuilder/cascades.go | 4 +++- pkg/sql/opt/exec/execbuilder/format.go | 1 + pkg/sql/opt/exec/execbuilder/relational.go | 4 +++- pkg/sql/opt/exec/execbuilder/statement.go | 9 ++++++++- .../opt/idxconstraint/index_constraints_test.go | 2 +- pkg/sql/opt/partialidx/implicator_test.go | 2 +- pkg/sql/plan_opt.go | 17 +++++++++++++++-- pkg/sql/sem/tree/eval_test.go | 2 +- 10 files changed, 38 insertions(+), 8 deletions(-) diff --git a/pkg/sql/opt/bench/bench_test.go b/pkg/sql/opt/bench/bench_test.go index 9c242546b23d..cbd392dbcc9b 100644 --- a/pkg/sql/opt/bench/bench_test.go +++ b/pkg/sql/opt/bench/bench_test.go @@ -484,6 +484,7 @@ func (h *harness) runSimple(tb testing.TB, query benchQuery, phase Phase) { execMemo, nil, /* catalog */ root, + &h.semaCtx, &h.evalCtx, true, /* allowAutoCommit */ ) @@ -537,6 +538,7 @@ func (h *harness) runPrepared(tb testing.TB, phase Phase) { execMemo, nil, /* catalog */ root, + &h.semaCtx, &h.evalCtx, true, /* allowAutoCommit */ ) diff --git a/pkg/sql/opt/exec/execbuilder/builder.go b/pkg/sql/opt/exec/execbuilder/builder.go index 10d6061d2181..506b94ad9173 100644 --- a/pkg/sql/opt/exec/execbuilder/builder.go +++ b/pkg/sql/opt/exec/execbuilder/builder.go @@ -38,6 +38,7 @@ type Builder struct { catalog cat.Catalog e opt.Expr disableTelemetry bool + semaCtx *tree.SemaContext evalCtx *tree.EvalContext // subqueries accumulates information about subqueries that are part of scalar @@ -111,6 +112,7 @@ func New( mem *memo.Memo, catalog cat.Catalog, e opt.Expr, + semaCtx *tree.SemaContext, evalCtx *tree.EvalContext, allowAutoCommit bool, ) *Builder { @@ -120,6 +122,7 @@ func New( mem: mem, catalog: catalog, e: e, + semaCtx: semaCtx, evalCtx: evalCtx, allowAutoCommit: allowAutoCommit, initialAllowAutoCommit: allowAutoCommit, diff --git a/pkg/sql/opt/exec/execbuilder/cascades.go b/pkg/sql/opt/exec/execbuilder/cascades.go index 691f40858df9..04fa2cdb80b2 100644 --- a/pkg/sql/opt/exec/execbuilder/cascades.go +++ b/pkg/sql/opt/exec/execbuilder/cascades.go @@ -293,7 +293,9 @@ func (cb *cascadeBuilder) planCascade( } // 5. Execbuild the optimized expression. - eb := New(execFactory, &o, factory.Memo(), cb.b.catalog, optimizedExpr, evalCtx, allowAutoCommit) + eb := New( + execFactory, &o, factory.Memo(), cb.b.catalog, optimizedExpr, semaCtx, evalCtx, allowAutoCommit, + ) if bufferRef != nil { // Set up the With binding. eb.addBuiltWithExpr(cascadeInputWithID, bufferColMap, bufferRef) diff --git a/pkg/sql/opt/exec/execbuilder/format.go b/pkg/sql/opt/exec/execbuilder/format.go index 2d209e37dbdc..cc7f68a78663 100644 --- a/pkg/sql/opt/exec/execbuilder/format.go +++ b/pkg/sql/opt/exec/execbuilder/format.go @@ -41,6 +41,7 @@ func fmtInterceptor(f *memo.ExprFmtCtx, scalar opt.ScalarExpr) string { f.Memo, nil, /* catalog */ scalar, + nil, /* semaCtx */ nil, /* evalCtx */ false, /* allowAutoCommit */ ) diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go index 69a0a179c1b9..efae88f7e741 100644 --- a/pkg/sql/opt/exec/execbuilder/relational.go +++ b/pkg/sql/opt/exec/execbuilder/relational.go @@ -915,7 +915,9 @@ func (b *Builder) buildApplyJoin(join memo.RelExpr) (execPlan, error) { return nil, err } - eb := New(ef, &o, f.Memo(), b.catalog, newRightSide, b.evalCtx, false /* allowAutoCommit */) + eb := New( + ef, &o, f.Memo(), b.catalog, newRightSide, b.semaCtx, b.evalCtx, false, /* allowAutoCommit */ + ) eb.disableTelemetry = true eb.withExprs = withExprs plan, err := eb.Build() diff --git a/pkg/sql/opt/exec/execbuilder/statement.go b/pkg/sql/opt/exec/execbuilder/statement.go index 343b6ec00ef4..48aa5f222ec4 100644 --- a/pkg/sql/opt/exec/execbuilder/statement.go +++ b/pkg/sql/opt/exec/execbuilder/statement.go @@ -130,7 +130,14 @@ func (b *Builder) buildExplain(explain *memo.ExplainExpr) (execPlan, error) { func(ef exec.ExplainFactory) (exec.Plan, error) { // Create a separate builder for the explain query. explainBld := New( - ef, b.optimizer, b.mem, b.catalog, explain.Input, b.evalCtx, b.initialAllowAutoCommit, + ef, + b.optimizer, + b.mem, + b.catalog, + explain.Input, + b.semaCtx, + b.evalCtx, + b.initialAllowAutoCommit, ) explainBld.disableTelemetry = true return explainBld.Build() diff --git a/pkg/sql/opt/idxconstraint/index_constraints_test.go b/pkg/sql/opt/idxconstraint/index_constraints_test.go index 7a6a5eac96d4..84279b440ecd 100644 --- a/pkg/sql/opt/idxconstraint/index_constraints_test.go +++ b/pkg/sql/opt/idxconstraint/index_constraints_test.go @@ -135,7 +135,7 @@ func TestIndexConstraints(t *testing.T) { if !remainingFilter.IsTrue() { execBld := execbuilder.New( nil /* execFactory */, nil /* optimizer */, f.Memo(), nil, /* catalog */ - &remainingFilter, &evalCtx, false, /* allowAutoCommit */ + &remainingFilter, &semaCtx, &evalCtx, false, /* allowAutoCommit */ ) expr, err := execBld.BuildScalar() if err != nil { diff --git a/pkg/sql/opt/partialidx/implicator_test.go b/pkg/sql/opt/partialidx/implicator_test.go index dc56b67c20bf..58a4431a1476 100644 --- a/pkg/sql/opt/partialidx/implicator_test.go +++ b/pkg/sql/opt/partialidx/implicator_test.go @@ -113,7 +113,7 @@ func TestImplicator(t *testing.T) { } else { execBld := execbuilder.New( nil /* factory */, nil /* optimizer */, f.Memo(), nil, /* catalog */ - &remainingFilters, &evalCtx, false, /* allowAutoCommit */ + &remainingFilters, &semaCtx, &evalCtx, false, /* allowAutoCommit */ ) expr, err := execBld.BuildScalar() if err != nil { diff --git a/pkg/sql/plan_opt.go b/pkg/sql/plan_opt.go index a132a1aa06f6..e64f43d2c0ae 100644 --- a/pkg/sql/plan_opt.go +++ b/pkg/sql/plan_opt.go @@ -210,6 +210,7 @@ func (p *planner) makeOptimizerPlan(ctx context.Context) error { &p.stmt, newDistSQLSpecExecFactory(p, planningMode), execMemo, + p.SemaCtx(), p.EvalContext(), p.autoCommit, ) @@ -244,6 +245,7 @@ func (p *planner) makeOptimizerPlan(ctx context.Context) error { &p.stmt, newDistSQLSpecExecFactory(p, distSQLLocalOnlyPlanning), execMemo, + p.SemaCtx(), p.EvalContext(), p.autoCommit, ) @@ -264,6 +266,7 @@ func (p *planner) makeOptimizerPlan(ctx context.Context) error { &p.stmt, newExecFactory(p), execMemo, + p.SemaCtx(), p.EvalContext(), p.autoCommit, ) @@ -554,6 +557,7 @@ func (opc *optPlanningCtx) runExecBuilder( stmt *Statement, f exec.Factory, mem *memo.Memo, + semaCtx *tree.SemaContext, evalCtx *tree.EvalContext, allowAutoCommit bool, ) error { @@ -563,7 +567,9 @@ func (opc *optPlanningCtx) runExecBuilder( var containsFullIndexScan bool if !planTop.instrumentation.ShouldBuildExplainPlan() { // No instrumentation. - bld := execbuilder.New(f, &opc.optimizer, mem, &opc.catalog, mem.RootExpr(), evalCtx, allowAutoCommit) + bld := execbuilder.New( + f, &opc.optimizer, mem, &opc.catalog, mem.RootExpr(), semaCtx, evalCtx, allowAutoCommit, + ) plan, err := bld.Build() if err != nil { return err @@ -576,7 +582,14 @@ func (opc *optPlanningCtx) runExecBuilder( // Create an explain factory and record the explain.Plan. explainFactory := explain.NewFactory(f) bld := execbuilder.New( - explainFactory, &opc.optimizer, mem, &opc.catalog, mem.RootExpr(), evalCtx, allowAutoCommit, + explainFactory, + &opc.optimizer, + mem, + &opc.catalog, + mem.RootExpr(), + semaCtx, + evalCtx, + allowAutoCommit, ) plan, err := bld.Build() if err != nil { diff --git a/pkg/sql/sem/tree/eval_test.go b/pkg/sql/sem/tree/eval_test.go index 6c2c5a549de6..bfc2b4631526 100644 --- a/pkg/sql/sem/tree/eval_test.go +++ b/pkg/sql/sem/tree/eval_test.go @@ -104,7 +104,7 @@ func optBuildScalar(evalCtx *tree.EvalContext, e tree.Expr) (tree.TypedExpr, err bld := execbuilder.New( nil /* factory */, &o, o.Memo(), nil /* catalog */, o.Memo().RootExpr(), - evalCtx, false, /* allowAutoCommit */ + &semaCtx, evalCtx, false, /* allowAutoCommit */ ) expr, err := bld.BuildScalar() if err != nil { From a08e6fcce9d44d0ce19dd9a1c67b755957d66ef7 Mon Sep 17 00:00:00 2001 From: Rebecca Taft Date: Tue, 27 Jul 2021 21:22:45 -0500 Subject: [PATCH 3/8] 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 --- .../logictestccl/testdata/logic_test/as_of | 153 +++++++++++++++++- pkg/sql/opt/exec/execbuilder/builder.go | 10 ++ pkg/sql/opt/exec/execbuilder/relational.go | 55 +++++++ pkg/sql/opt/optbuilder/select.go | 5 +- 4 files changed, 215 insertions(+), 8 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/as_of b/pkg/ccl/logictestccl/testdata/logic_test/as_of index 5a327e6ce246..30ad26818c39 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/as_of +++ b/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,12 +114,143 @@ 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) @@ -127,16 +258,20 @@ 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) diff --git a/pkg/sql/opt/exec/execbuilder/builder.go b/pkg/sql/opt/exec/execbuilder/builder.go index 506b94ad9173..0ad67903c267 100644 --- a/pkg/sql/opt/exec/execbuilder/builder.go +++ b/pkg/sql/opt/exec/execbuilder/builder.go @@ -94,6 +94,10 @@ type Builder struct { // containsFullIndexScan is set to true if the statement contains a secondary // index scan. ContainsFullIndexScan bool + + // containsBoundedStalenessScan is true if the query uses bounded + // staleness and contains a scan. + containsBoundedStalenessScan bool } // New constructs an instance of the execution node builder using the @@ -226,6 +230,12 @@ func (b *Builder) findBuiltWithExpr(id opt.WithID) *builtWithExpr { return nil } +// boundedStaleness returns true if this query uses bounded staleness. +func (b *Builder) boundedStaleness() bool { + return b.semaCtx != nil && b.semaCtx.AsOfSystemTime != nil && + b.semaCtx.AsOfSystemTime.BoundedStaleness +} + // mdVarContainer is an IndexedVarContainer implementation used by BuildScalar - // it maps indexed vars to columns in the metadata. type mdVarContainer struct { diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go index efae88f7e741..60694f6c3353 100644 --- a/pkg/sql/opt/exec/execbuilder/relational.go +++ b/pkg/sql/opt/exec/execbuilder/relational.go @@ -170,6 +170,15 @@ func (b *Builder) buildRelational(e memo.RelExpr) (execPlan, error) { "cannot execute %s in a read-only transaction", b.statementTag(e)) } + // Raise error if bounded staleness is used incorrectly. + if b.boundedStaleness() { + if _, ok := boundedStalenessAllowList[e.Op()]; !ok { + return execPlan{}, unimplemented.NewWithIssuef(67562, + "cannot use bounded staleness for %s", b.statementTag(e), + ) + } + } + // Collect usage telemetry for relational node, if appropriate. if !b.disableTelemetry { if c := opt.OpTelemetryCounters[e.Op()]; c != nil { @@ -562,6 +571,30 @@ func (b *Builder) scanParams( softLimit := int64(math.Ceil(reqProps.LimitHint)) hardLimit := scan.HardLimit.RowCount() + // If this is a bounded staleness query, check that it touches at most one + // range. + if b.boundedStaleness() { + valid := true + if b.containsBoundedStalenessScan { + // We already planned a scan, perhaps as part of a subquery. + valid = false + } else if hardLimit != 0 { + // If hardLimit is not 0, from KV's perspective, this is a multi-row scan + // with a limit. That means that even if the limit is 1, the scan can span + // multiple ranges if the first range is empty. + valid = false + } else { + maxResults, ok := b.indexConstraintMaxResults(scan, relProps) + valid = ok && maxResults == 1 + } + if !valid { + return exec.ScanParams{}, opt.ColMap{}, unimplemented.NewWithIssuef(67562, + "cannot use bounded staleness for queries that may touch more than one range or require an index join", + ) + } + b.containsBoundedStalenessScan = true + } + parallelize := false if hardLimit == 0 && softLimit == 0 { maxResults, ok := b.indexConstraintMaxResults(scan, relProps) @@ -2472,3 +2505,25 @@ func (b *Builder) statementTag(expr memo.RelExpr) string { return expr.Op().SyntaxTag() } } + +// boundedStalenessAllowList contains the operators that may be used with +// bounded staleness queries. +var boundedStalenessAllowList = map[opt.Operator]struct{}{ + opt.ValuesOp: {}, + opt.ScanOp: {}, + opt.PlaceholderScanOp: {}, + opt.SelectOp: {}, + opt.ProjectOp: {}, + opt.GroupByOp: {}, + opt.ScalarGroupByOp: {}, + opt.DistinctOnOp: {}, + opt.EnsureDistinctOnOp: {}, + opt.LimitOp: {}, + opt.OffsetOp: {}, + opt.SortOp: {}, + opt.OrdinalityOp: {}, + opt.Max1RowOp: {}, + opt.ProjectSetOp: {}, + opt.WindowOp: {}, + opt.ExplainOp: {}, +} diff --git a/pkg/sql/opt/optbuilder/select.go b/pkg/sql/opt/optbuilder/select.go index 2a64d627679b..6812b116d31d 100644 --- a/pkg/sql/opt/optbuilder/select.go +++ b/pkg/sql/opt/optbuilder/select.go @@ -539,6 +539,10 @@ func (b *Builder) buildScan( if locking.isSet() { private.Locking = locking.get() } + if b.semaCtx.AsOfSystemTime != nil && b.semaCtx.AsOfSystemTime.BoundedStaleness { + private.Flags.NoIndexJoin = true + private.Flags.NoZigzagJoin = true + } b.addCheckConstraintsForTable(tabMeta) b.addComputedColsForTable(tabMeta) @@ -1182,7 +1186,6 @@ func (b *Builder) buildFromWithLateral( // validateAsOf ensures that any AS OF SYSTEM TIME timestamp is consistent with // that of the root statement. func (b *Builder) validateAsOf(asOfClause tree.AsOfClause) { - // TODO(#67558): prohibit bounded staleness in subqueries. asOf, err := tree.EvalAsOfTimestamp( b.ctx, asOfClause, From db265db6661f510dff453280b458d899df66fdda Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Tue, 27 Jul 2021 10:37:20 +0200 Subject: [PATCH 4/8] roachprod: improve impl of pgurl Rather than manually cobbling together an url, use `url.URL`. Also, add a `--certs-dir` parameter to `pgurl` that allows customizing the location of the certs represented in the returned string. Release note: None --- pkg/cmd/roachprod/install/cluster_synced.go | 1 + pkg/cmd/roachprod/install/cockroach.go | 19 +++++++++++++------ pkg/cmd/roachprod/main.go | 4 ++++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/roachprod/install/cluster_synced.go b/pkg/cmd/roachprod/install/cluster_synced.go index 1c7b34652061..6e0e7d0ee56e 100644 --- a/pkg/cmd/roachprod/install/cluster_synced.go +++ b/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 diff --git a/pkg/cmd/roachprod/install/cockroach.go b/pkg/cmd/roachprod/install/cockroach.go index 0095f3a3ab96..2bfa54e0cbe9 100644 --- a/pkg/cmd/roachprod/install/cockroach.go +++ b/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. diff --git a/pkg/cmd/roachprod/main.go b/pkg/cmd/roachprod/main.go index 0b26e2618b34..b17abcbece94 100644 --- a/pkg/cmd/roachprod/main.go +++ b/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") From 5368b10aa29297c7106583fc13d9db58f653d07d Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Tue, 27 Jul 2021 11:25:51 +0200 Subject: [PATCH 5/8] roachtest: support using Conn() against secure clusters roachtest does not offer real support for secure clusters. In light of https://github.com/cockroachdb/cockroach/issues/65830 this needs to change. It will be a larger project to make this truly seamless. This is just a first important step: roachtest, when starting a secure cluster, will download the certs so that it can generate the proper SQL connection string for use by `c.Conn`. Importantly, the test (mostly) doesn't have to care whether it's operating against a secure cluster, as long as it doesn't do anything fancy. Parts of roachtest's artifact collection assumes an insecure cluster, so artifacts will be degraded when the cluster is secure. The logs will still be collected, though. Release note: None --- pkg/cmd/roachtest/cluster.go | 42 +++++++++++++++++++-- pkg/cmd/roachtest/tests/BUILD.bazel | 1 + pkg/cmd/roachtest/tests/registry.go | 1 + pkg/cmd/roachtest/tests/smoketest_secure.go | 41 ++++++++++++++++++++ 4 files changed, 82 insertions(+), 3 deletions(-) create mode 100644 pkg/cmd/roachtest/tests/smoketest_secure.go diff --git a/pkg/cmd/roachtest/cluster.go b/pkg/cmd/roachtest/cluster.go index 4c6dba4eeab5..6f6ba1438370 100644 --- a/pkg/cmd/roachtest/cluster.go +++ b/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...) diff --git a/pkg/cmd/roachtest/tests/BUILD.bazel b/pkg/cmd/roachtest/tests/BUILD.bazel index 825273cb7301..1f1dfebc5344 100644 --- a/pkg/cmd/roachtest/tests/BUILD.bazel +++ b/pkg/cmd/roachtest/tests/BUILD.bazel @@ -101,6 +101,7 @@ go_library( "scrub.go", "secondary_indexes.go", "sequelize.go", + "smoketest_secure.go", "split.go", "sqlalchemy.go", "sqlalchemy_blocklist.go", diff --git a/pkg/cmd/roachtest/tests/registry.go b/pkg/cmd/roachtest/tests/registry.go index 36b8e38f2ed4..3e852c6e140c 100644 --- a/pkg/cmd/roachtest/tests/registry.go +++ b/pkg/cmd/roachtest/tests/registry.go @@ -94,6 +94,7 @@ func RegisterTests(r registry.Registry) { registerScrubAllChecksTPCC(r) registerScrubIndexOnlyTPCC(r) registerSecondaryIndexesMultiVersionCluster(r) + registerSecure(r) registerSequelize(r) registerSQLAlchemy(r) registerSQLSmith(r) diff --git a/pkg/cmd/roachtest/tests/smoketest_secure.go b/pkg/cmd/roachtest/tests/smoketest_secure.go new file mode 100644 index 000000000000..b2a99d0f4960 --- /dev/null +++ b/pkg/cmd/roachtest/tests/smoketest_secure.go @@ -0,0 +1,41 @@ +// Copyright 2021 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package tests + +import ( + "context" + "fmt" + + "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster" + "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option" + "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry" + "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test" + "github.com/stretchr/testify/require" +) + +func registerSecure(r registry.Registry) { + for _, numNodes := range []int{1, 3} { + r.Add(registry.TestSpec{ + Name: fmt.Sprintf("smoketest/secure/nodes=%d", numNodes), + Tags: []string{"smoketest", "weekly"}, + Owner: registry.OwnerKV, // TODO: OwnerTestEng once the open PR that introduces it has merged + Cluster: r.MakeClusterSpec(numNodes), + Run: func(ctx context.Context, t test.Test, c cluster.Cluster) { + c.Put(ctx, t.Cockroach(), "./cockroach") + c.Start(ctx, option.StartArgs("--secure")) + db := c.Conn(ctx, 1) + defer db.Close() + _, err := db.QueryContext(ctx, `SELECT 1`) + require.NoError(t, err) + }, + }) + } +} From cac044af882861fa80159306b1293d9e601f6193 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Tue, 27 Jul 2021 11:31:44 +0200 Subject: [PATCH 6/8] roachtest: remove unused method from Cluster interface The impl on clusterImpl is still used, so it isn't removed yet. Release note: None --- pkg/cmd/roachtest/cluster/cluster_interface.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/cmd/roachtest/cluster/cluster_interface.go b/pkg/cmd/roachtest/cluster/cluster_interface.go index 4bb567291976..d407b3b759e5 100644 --- a/pkg/cmd/roachtest/cluster/cluster_interface.go +++ b/pkg/cmd/roachtest/cluster/cluster_interface.go @@ -66,9 +66,6 @@ type Cluster interface { InternalPGUrl(ctx context.Context, node option.NodeListOption) ([]string, error) ExternalPGUrl(ctx context.Context, node option.NodeListOption) ([]string, error) - ExternalPGUrlSecure( - ctx context.Context, node option.NodeListOption, user string, certsDir string, port int, - ) ([]string, error) // SQL clients to nodes. From 05fee699fdeb9301059180b4b7b476274c929648 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Tue, 27 Jul 2021 11:49:46 +0200 Subject: [PATCH 7/8] roachtest: remove ad-hoc methods for secure clusters A number of ad-hoc mechanisms for handling secure clusters was added a while back for the node-postgres roachtest. Now that roachprod and roachtest have rudimentary support for dealing with secure clusters, we get to remove all of that. I verified that node-postgres passes with this commit. Release note: None --- pkg/cmd/roachprod/install/cluster_synced.go | 1 + pkg/cmd/roachtest/cluster.go | 32 ------------ .../roachtest/cluster/cluster_interface.go | 3 -- pkg/cmd/roachtest/tests/activerecord.go | 6 +-- pkg/cmd/roachtest/tests/canary.go | 46 ++--------------- pkg/cmd/roachtest/tests/django.go | 6 +-- pkg/cmd/roachtest/tests/gopg.go | 6 +-- pkg/cmd/roachtest/tests/gorm.go | 4 +- pkg/cmd/roachtest/tests/hibernate.go | 6 +-- pkg/cmd/roachtest/tests/libpq.go | 6 +-- pkg/cmd/roachtest/tests/liquibase.go | 4 +- pkg/cmd/roachtest/tests/nodejs_postgres.go | 49 +++---------------- pkg/cmd/roachtest/tests/orm_helpers.go | 25 ++-------- pkg/cmd/roachtest/tests/pgjdbc.go | 6 +-- pkg/cmd/roachtest/tests/pgx.go | 6 +-- pkg/cmd/roachtest/tests/pop.go | 4 +- pkg/cmd/roachtest/tests/psycopg.go | 6 +-- pkg/cmd/roachtest/tests/ruby_pg.go | 4 +- pkg/cmd/roachtest/tests/sequelize.go | 4 +- pkg/cmd/roachtest/tests/sqlalchemy.go | 6 +-- pkg/cmd/roachtest/tests/typeorm.go | 6 +-- 21 files changed, 46 insertions(+), 190 deletions(-) diff --git a/pkg/cmd/roachprod/install/cluster_synced.go b/pkg/cmd/roachprod/install/cluster_synced.go index 6e0e7d0ee56e..148652f19727 100644 --- a/pkg/cmd/roachprod/install/cluster_synced.go +++ b/pkg/cmd/roachprod/install/cluster_synced.go @@ -895,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, " ")) diff --git a/pkg/cmd/roachtest/cluster.go b/pkg/cmd/roachtest/cluster.go index 6f6ba1438370..90b356f4e7e5 100644 --- a/pkg/cmd/roachtest/cluster.go +++ b/pkg/cmd/roachtest/cluster.go @@ -2056,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 { @@ -2261,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 { diff --git a/pkg/cmd/roachtest/cluster/cluster_interface.go b/pkg/cmd/roachtest/cluster/cluster_interface.go index d407b3b759e5..1ec97c76d0a8 100644 --- a/pkg/cmd/roachtest/cluster/cluster_interface.go +++ b/pkg/cmd/roachtest/cluster/cluster_interface.go @@ -71,9 +71,6 @@ type Cluster interface { Conn(ctx context.Context, node int) *gosql.DB ConnE(ctx context.Context, node int) (*gosql.DB, error) - ConnSecure( - ctx context.Context, node int, user string, certsDir string, port int, - ) (*gosql.DB, error) // URLs for the Admin UI. diff --git a/pkg/cmd/roachtest/tests/activerecord.go b/pkg/cmd/roachtest/tests/activerecord.go index 43419a872526..dd7469d25dda 100644 --- a/pkg/cmd/roachtest/tests/activerecord.go +++ b/pkg/cmd/roachtest/tests/activerecord.go @@ -46,14 +46,12 @@ func registerActiveRecord(r registry.Registry) { } c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings( - ctx, version, c, node[0], nil, - ); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/canary.go b/pkg/cmd/roachtest/tests/canary.go index f09dda6a71d3..ebff0a176e03 100644 --- a/pkg/cmd/roachtest/tests/canary.go +++ b/pkg/cmd/roachtest/tests/canary.go @@ -12,7 +12,6 @@ package tests import ( "context" - gosql "database/sql" "encoding/json" "fmt" "net/http" @@ -54,26 +53,6 @@ type blocklistForVersion struct { type blocklistsForVersion []blocklistForVersion -// SecureDBConnectionParams contains information used to create a secure -// connection to CRDB. -type SecureDBConnectionParams struct { - username string - certsDir string - port int -} - -// NewSecureDBConnectionParams creates a SecureDBConnectionParams struct for creating -// a secure connection. -func NewSecureDBConnectionParams( - username string, certsDir string, port int, -) *SecureDBConnectionParams { - return &SecureDBConnectionParams{ - username: username, - certsDir: certsDir, - port: port, - } -} - // getLists returns the appropriate blocklist and ignorelist based on the // cockroach version. This check only looks to ensure that the prefix that // matches. @@ -86,27 +65,10 @@ func (b blocklistsForVersion) getLists(version string) (string, blocklist, strin return "", nil, "", nil } -func fetchCockroachVersion( - ctx context.Context, - c cluster.Cluster, - nodeIndex int, - dbConnectionParams *SecureDBConnectionParams, -) (string, error) { - var db *gosql.DB - var err error - if dbConnectionParams != nil { - db, err = c.ConnSecure( - ctx, nodeIndex, dbConnectionParams.username, - dbConnectionParams.certsDir, dbConnectionParams.port, - ) - if err != nil { - return "", err - } - } else { - db, err = c.ConnE(ctx, nodeIndex) - if err != nil { - return "", err - } +func fetchCockroachVersion(ctx context.Context, c cluster.Cluster, nodeIndex int) (string, error) { + db, err := c.ConnE(ctx, nodeIndex) + if err != nil { + return "", err } defer db.Close() var version string diff --git a/pkg/cmd/roachtest/tests/django.go b/pkg/cmd/roachtest/tests/django.go index 2f9818ad2653..80e86f92d170 100644 --- a/pkg/cmd/roachtest/tests/django.go +++ b/pkg/cmd/roachtest/tests/django.go @@ -41,14 +41,12 @@ func registerDjango(r registry.Registry) { c.Put(ctx, t.Cockroach(), "./cockroach", c.All()) c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - err = alterZoneConfigAndClusterSettings( - ctx, version, c, node[0], nil, - ) + err = alterZoneConfigAndClusterSettings(ctx, version, c, node[0]) if err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/gopg.go b/pkg/cmd/roachtest/tests/gopg.go index 79e538ec269c..b260d2ac66f6 100644 --- a/pkg/cmd/roachtest/tests/gopg.go +++ b/pkg/cmd/roachtest/tests/gopg.go @@ -49,14 +49,12 @@ func registerGopg(r registry.Registry) { t.Status("setting up cockroach") c.Put(ctx, t.Cockroach(), "./cockroach", c.All()) c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings( - ctx, version, c, node[0], nil, - ); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/gorm.go b/pkg/cmd/roachtest/tests/gorm.go index 5ccd168cee6a..ea6066ed693c 100644 --- a/pkg/cmd/roachtest/tests/gorm.go +++ b/pkg/cmd/roachtest/tests/gorm.go @@ -33,11 +33,11 @@ func registerGORM(r registry.Registry) { t.Status("setting up cockroach") c.Put(ctx, t.Cockroach(), "./cockroach", c.All()) c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0], nil); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/hibernate.go b/pkg/cmd/roachtest/tests/hibernate.go index 39b630e30174..c377dd25b31c 100644 --- a/pkg/cmd/roachtest/tests/hibernate.go +++ b/pkg/cmd/roachtest/tests/hibernate.go @@ -87,14 +87,12 @@ func registerHibernate(r registry.Registry, opt hibernateOptions) { opt.dbSetupFunc(ctx, t, c) } - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings( - ctx, version, c, node[0], nil, - ); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/libpq.go b/pkg/cmd/roachtest/tests/libpq.go index ded4c5f0caa9..c3752be0be94 100644 --- a/pkg/cmd/roachtest/tests/libpq.go +++ b/pkg/cmd/roachtest/tests/libpq.go @@ -35,13 +35,11 @@ func registerLibPQ(r registry.Registry) { c.Put(ctx, t.Cockroach(), "./cockroach", c.All()) c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings( - ctx, version, c, node[0], nil, - ); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/liquibase.go b/pkg/cmd/roachtest/tests/liquibase.go index b4af3a6f1e7f..ed80d33d3b6c 100644 --- a/pkg/cmd/roachtest/tests/liquibase.go +++ b/pkg/cmd/roachtest/tests/liquibase.go @@ -36,12 +36,12 @@ func registerLiquibase(r registry.Registry) { c.Put(ctx, t.Cockroach(), "./cockroach", c.All()) c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0], nil); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/nodejs_postgres.go b/pkg/cmd/roachtest/tests/nodejs_postgres.go index dceddc603dbb..f8e99516ff0c 100644 --- a/pkg/cmd/roachtest/tests/nodejs_postgres.go +++ b/pkg/cmd/roachtest/tests/nodejs_postgres.go @@ -13,8 +13,6 @@ package tests import ( "context" "fmt" - "os" - "path/filepath" "strings" "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster" @@ -46,56 +44,23 @@ func registerNodeJSPostgres(r registry.Registry) { err = c.StartE(ctx, option.StartArgs("--secure")) require.NoError(t, err) - user := "testuser" - certsDir := "/home/ubuntu/certs" - localCertsDir, err := filepath.Abs("./certs") - require.NoError(t, err) + const user = "testuser" // certs auto-generated by roachprod start --secure err = repeatRunE(ctx, t, c, node, "create sql user", fmt.Sprintf( - `./cockroach sql --certs-dir %s -e "CREATE USER %s CREATEDB"`, - certsDir, user, + `./cockroach sql --certs-dir certs -e "CREATE USER %s CREATEDB"`, user, )) require.NoError(t, err) - err = repeatRunE(ctx, t, c, c.All(), "create user certs", - fmt.Sprintf(`./cockroach cert create-client testuser --certs-dir %s --ca-key=%s/ca.key`, - certsDir, certsDir)) - require.NoError(t, err) - err = repeatRunE(ctx, t, c, node, "create test database", - fmt.Sprintf(`./cockroach sql --certs-dir %s -e "CREATE DATABASE postgres_node_test"`, certsDir), + `./cockroach sql --certs-dir certs -e "CREATE DATABASE postgres_node_test"`, ) require.NoError(t, err) - err = os.RemoveAll(localCertsDir) - require.NoError(t, err) - - err = c.Get(ctx, t.L(), certsDir, localCertsDir) - require.NoError(t, err) - - // Certs can have at max 0600 privilege. - err = filepath.Walk(localCertsDir, func(path string, info os.FileInfo, err error) error { - // Don't change permissions for the certs directory. - if path == localCertsDir { - return nil - } - if err != nil { - return err - } - return os.Chmod(path, os.FileMode(0600)) - }) + version, err := fetchCockroachVersion(ctx, c, node[0]) require.NoError(t, err) - version, err := fetchCockroachVersion( - ctx, c, node[0], NewSecureDBConnectionParams(user, localCertsDir, 26257), - ) - require.NoError(t, err) - - err = alterZoneConfigAndClusterSettings( - ctx, version, c, node[0], - NewSecureDBConnectionParams("root", localCertsDir, 26257), - ) + err = alterZoneConfigAndClusterSettings(ctx, version, c, node[0]) require.NoError(t, err) err = repeatRunE( @@ -160,8 +125,8 @@ func registerNodeJSPostgres(r registry.Registry) { fmt.Sprintf( `cd /mnt/data1/node-postgres/ && sudo \ PGPORT=26257 PGUSER=%s PGSSLMODE=require PGDATABASE=postgres_node_test \ -PGSSLCERT=%s/client.%s.crt PGSSLKEY=%s/client.%s.key PGSSLROOTCERT=%s/ca.crt yarn test`, - user, certsDir, user, certsDir, user, certsDir, +PGSSLCERT=$HOME/certs/client.%s.crt PGSSLKEY=$HOME/certs/client.%s.key PGSSLROOTCERT=$HOME/certs/ca.crt yarn test`, + user, user, user, ), ) rawResultsStr := string(rawResults) diff --git a/pkg/cmd/roachtest/tests/orm_helpers.go b/pkg/cmd/roachtest/tests/orm_helpers.go index fee0a1d0deb8..ed115539f990 100644 --- a/pkg/cmd/roachtest/tests/orm_helpers.go +++ b/pkg/cmd/roachtest/tests/orm_helpers.go @@ -12,7 +12,6 @@ package tests import ( "context" - gosql "database/sql" "fmt" "sort" "strings" @@ -27,27 +26,11 @@ import ( // cause thousands of table descriptors and schema change jobs to accumulate // rapidly, thereby decreasing performance. func alterZoneConfigAndClusterSettings( - ctx context.Context, - version string, - c cluster.Cluster, - nodeIdx int, - dbConnectionParams *SecureDBConnectionParams, + ctx context.Context, version string, c cluster.Cluster, nodeIdx int, ) error { - var db *gosql.DB - var err error - if dbConnectionParams != nil { - db, err = c.ConnSecure( - ctx, nodeIdx, dbConnectionParams.username, - dbConnectionParams.certsDir, dbConnectionParams.port, - ) - if err != nil { - return err - } - } else { - db, err = c.ConnE(ctx, nodeIdx) - if err != nil { - return err - } + db, err := c.ConnE(ctx, nodeIdx) + if err != nil { + return err } defer db.Close() diff --git a/pkg/cmd/roachtest/tests/pgjdbc.go b/pkg/cmd/roachtest/tests/pgjdbc.go index a92e8b1b3b93..db06137f989c 100644 --- a/pkg/cmd/roachtest/tests/pgjdbc.go +++ b/pkg/cmd/roachtest/tests/pgjdbc.go @@ -39,14 +39,12 @@ func registerPgjdbc(r registry.Registry) { c.Put(ctx, t.Cockroach(), "./cockroach", c.All()) c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings( - ctx, version, c, node[0], nil, - ); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/pgx.go b/pkg/cmd/roachtest/tests/pgx.go index 44fb657511d2..1a0d6e568a10 100644 --- a/pkg/cmd/roachtest/tests/pgx.go +++ b/pkg/cmd/roachtest/tests/pgx.go @@ -39,14 +39,12 @@ func registerPgx(r registry.Registry) { c.Put(ctx, t.Cockroach(), "./cockroach", c.All()) c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings( - ctx, version, c, node[0], nil, - ); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/pop.go b/pkg/cmd/roachtest/tests/pop.go index 688763f7eefb..75d2b8826e4a 100644 --- a/pkg/cmd/roachtest/tests/pop.go +++ b/pkg/cmd/roachtest/tests/pop.go @@ -33,11 +33,11 @@ func registerPop(r registry.Registry) { t.Status("setting up cockroach") c.Put(ctx, t.Cockroach(), "./cockroach", c.All()) c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0], nil); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/psycopg.go b/pkg/cmd/roachtest/tests/psycopg.go index 38e6f8383830..7fe501342085 100644 --- a/pkg/cmd/roachtest/tests/psycopg.go +++ b/pkg/cmd/roachtest/tests/psycopg.go @@ -37,14 +37,12 @@ func registerPsycopg(r registry.Registry) { c.Put(ctx, t.Cockroach(), "./cockroach", c.All()) c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings( - ctx, version, c, node[0], nil, - ); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/ruby_pg.go b/pkg/cmd/roachtest/tests/ruby_pg.go index 98f6b4915a87..edeff8373e60 100644 --- a/pkg/cmd/roachtest/tests/ruby_pg.go +++ b/pkg/cmd/roachtest/tests/ruby_pg.go @@ -45,12 +45,12 @@ func registerRubyPG(r registry.Registry) { } c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0], nil); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/sequelize.go b/pkg/cmd/roachtest/tests/sequelize.go index 1eed70b5aeb9..f2cacc445641 100644 --- a/pkg/cmd/roachtest/tests/sequelize.go +++ b/pkg/cmd/roachtest/tests/sequelize.go @@ -41,12 +41,12 @@ func registerSequelize(r registry.Registry) { } c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0], nil); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/sqlalchemy.go b/pkg/cmd/roachtest/tests/sqlalchemy.go index 34797dc17fca..1673bbb375ec 100644 --- a/pkg/cmd/roachtest/tests/sqlalchemy.go +++ b/pkg/cmd/roachtest/tests/sqlalchemy.go @@ -170,14 +170,12 @@ func runSQLAlchemy(ctx context.Context, t test.Test, c cluster.Cluster) { c.Put(ctx, t.Cockroach(), "./cockroach", c.All()) c.Start(ctx, c.All()) - version, err := fetchCockroachVersion(ctx, c, node[0], nil) + version, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings( - ctx, version, c, node[0], nil, - ); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/roachtest/tests/typeorm.go b/pkg/cmd/roachtest/tests/typeorm.go index 13203f7a17c4..cca198d563e3 100644 --- a/pkg/cmd/roachtest/tests/typeorm.go +++ b/pkg/cmd/roachtest/tests/typeorm.go @@ -39,14 +39,12 @@ func registerTypeORM(r registry.Registry) { c.Put(ctx, t.Cockroach(), "./cockroach", c.All()) c.Start(ctx, c.All()) - cockroachVersion, err := fetchCockroachVersion(ctx, c, node[0], nil) + cockroachVersion, err := fetchCockroachVersion(ctx, c, node[0]) if err != nil { t.Fatal(err) } - if err := alterZoneConfigAndClusterSettings( - ctx, cockroachVersion, c, node[0], nil, - ); err != nil { + if err := alterZoneConfigAndClusterSettings(ctx, cockroachVersion, c, node[0]); err != nil { t.Fatal(err) } From f5885dfa95afd3321763ee0eef7e136f7df4bc99 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Thu, 29 Jul 2021 15:47:36 +0200 Subject: [PATCH 8/8] roachtest: fix connection_latency roachtest Now that `testuser` certs are created by `roachprod start --secure` we need to update this test to stop doing it itself. Release note: None --- pkg/cmd/roachtest/tests/connection_latency.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/cmd/roachtest/tests/connection_latency.go b/pkg/cmd/roachtest/tests/connection_latency.go index 283707cf88eb..019b63db89e5 100644 --- a/pkg/cmd/roachtest/tests/connection_latency.go +++ b/pkg/cmd/roachtest/tests/connection_latency.go @@ -71,11 +71,9 @@ func runConnectionLatencyTest( } urlString = strings.Join(urls, " ") + // NB: certs for `testuser` are created by `roachprod start --secure`. err = c.RunE(ctx, c.Node(1), `./cockroach sql --certs-dir certs -e "CREATE USER testuser CREATEDB"`) require.NoError(t, err) - err = c.RunE(ctx, c.All(), - fmt.Sprintf(`./cockroach cert create-client testuser --certs-dir %s --ca-key=%s/ca.key`, - certsDir, certsDir)) require.NoError(t, err) err = c.RunE(ctx, c.Node(1), "./workload init connectionlatency --user testuser --secure") require.NoError(t, err)