diff --git a/build/bazelutil/nogo_config.json b/build/bazelutil/nogo_config.json index 4f6f5b2f979a..34a2c9a23424 100644 --- a/build/bazelutil/nogo_config.json +++ b/build/bazelutil/nogo_config.json @@ -739,7 +739,14 @@ "pkg/sql/execinfra/processorsbase.go$": "temporary exclusion until deprecatedContext is removed from eval.Context", "pkg/sql/importer/import_table_creation.go$": "temporary exclusion until deprecatedContext is removed from eval.Context", "pkg/sql/row/expr_walker_test.go$": "temporary exclusion until deprecatedContext is removed from eval.Context", - "pkg/sql/schemachanger/scbuild/tree_context_builder.go$": "temporary exclusion until deprecatedContext is removed from eval.Context" + "pkg/sql/schemachanger/scbuild/tree_context_builder.go$": "temporary exclusion until deprecatedContext is removed from eval.Context", + "pkg/security/securityassets/security_assets.go$": "excluded until all uses of io/ioutil are replaced", + "pkg/roachprod/install/cluster_synced.go$": "excluded until all uses of io/ioutil are replaced", + "pkg/security/securitytest/securitytest.go$": "excluded until all uses of io/ioutil are replaced", + "pkg/server/dumpstore/dumpstore.go$": "excluded until all uses of io/ioutil are replaced", + "pkg/server/dumpstore/dumpstore_test.go$": "excluded until all uses of io/ioutil are replaced", + "pkg/server/heapprofiler/profilestore_test.go$": "excluded until all uses of io/ioutil are replaced", + "pkg/util/log/file_api.go$": "excluded until all uses of io/ioutil are replaced" }, "only_files": { "cockroach/pkg/.*$": "first-party code" diff --git a/build/patches/co_honnef_go_tools.patch b/build/patches/co_honnef_go_tools.patch index ea4e0b73946e..8dee2b6bb8cf 100644 --- a/build/patches/co_honnef_go_tools.patch +++ b/build/patches/co_honnef_go_tools.patch @@ -1,5 +1,5 @@ diff --git a/staticcheck/lint.go b/staticcheck/lint.go -index 85bcb21..e91e81c 100644 +index 85bcb21..9fb4cc0 100644 --- a/staticcheck/lint.go +++ b/staticcheck/lint.go @@ -3080,6 +3080,9 @@ func CheckDeprecated(pass *analysis.Pass) (interface{}, error) { @@ -44,3 +44,12 @@ index 85bcb21..e91e81c 100644 } return true } +@@ -3209,6 +3214,8 @@ func CheckDeprecated(pass *analysis.Pass) (interface{}, error) { + } + + handleDeprecation(depr, spec.Path, path, path, nil) ++ } else if _, ok := knowledge.StdlibDeprecations[path]; ok { ++ handleDeprecation(nil, spec.Path, path, path, nil) + } + } + pass.ResultOf[inspect.Analyzer].(*inspector.Inspector).Nodes(nil, fn) diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index 9a87a76c90b4..01b472a145d1 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -4674,3 +4674,39 @@ order by attnum; attnum attname 1 x 2 y + +# Regression test for not projecting away looked up columns by the left semi +# virtual lookup join (#91012). +statement ok +CREATE TABLE t91012 (id INT, a_id INT); + +query I +SELECT + count(*) +FROM + pg_class AS t INNER JOIN pg_attribute AS a ON t.oid = a.attrelid +WHERE + a.attnotnull = 'f' + AND a.attname = 'a_id' + AND t.relname = 't91012' + AND a.atttypid + IN (SELECT oid FROM pg_type WHERE typname = ANY (ARRAY['int8'])); +---- +1 + +# Regression test for incorrectly handling left anti virtual lookup joins +# (#88096). +statement ok +CREATE TYPE mytype AS enum('hello') + +query I +SELECT + count(*) +FROM + pg_type AS t +WHERE + t.typrelid = 0 + AND NOT EXISTS(SELECT 1 FROM pg_type AS el WHERE el.oid = t.typelem AND el.typarray = t.oid) + AND t.typname LIKE 'myt%' +---- +1 diff --git a/pkg/sql/opt/xform/join_funcs.go b/pkg/sql/opt/xform/join_funcs.go index 8260993153e3..f2e58c36df5a 100644 --- a/pkg/sql/opt/xform/join_funcs.go +++ b/pkg/sql/opt/xform/join_funcs.go @@ -292,6 +292,7 @@ func (c *CustomFuncs) GenerateLookupJoins( // // It should be possible to support semi- and anti- joins. Left joins may be // possible with additional complexity. +// TODO(mgartner): update this comment. // // It should also be possible to support cases where all the virtual columns are // not covered by a single index by wrapping the lookup join in a Project that diff --git a/pkg/sql/opt_exec_factory.go b/pkg/sql/opt_exec_factory.go index bb85c5491ada..0f1987abb886 100644 --- a/pkg/sql/opt_exec_factory.go +++ b/pkg/sql/opt_exec_factory.go @@ -790,10 +790,18 @@ func (ef *execFactory) constructVirtualTableLookupJoin( tableScan.index = idx vtableCols := colinfo.ResultColumnsFromColumns(tableDesc.GetID(), tableDesc.PublicColumns()) projectedVtableCols := planColumns(&tableScan) - outputCols := make(colinfo.ResultColumns, 0, len(inputCols)+len(projectedVtableCols)) - outputCols = append(outputCols, inputCols...) - outputCols = append(outputCols, projectedVtableCols...) - // joinType is either INNER or LEFT_OUTER. + var outputCols colinfo.ResultColumns + switch joinType { + case descpb.InnerJoin, descpb.LeftOuterJoin: + outputCols = make(colinfo.ResultColumns, 0, len(inputCols)+len(projectedVtableCols)) + outputCols = append(outputCols, inputCols...) + outputCols = append(outputCols, projectedVtableCols...) + case descpb.LeftSemiJoin, descpb.LeftAntiJoin: + outputCols = make(colinfo.ResultColumns, 0, len(inputCols)) + outputCols = append(outputCols, inputCols...) + default: + return nil, errors.AssertionFailedf("unexpected join type for virtual lookup join: %s", joinType.String()) + } pred := makePredicate(joinType, inputCols, projectedVtableCols) pred.onCond = pred.iVarHelper.Rebind(onCond) n := &vTableLookupJoinNode{ diff --git a/pkg/sql/rowcontainer/BUILD.bazel b/pkg/sql/rowcontainer/BUILD.bazel index 44db8cc14039..90d87cf8de36 100644 --- a/pkg/sql/rowcontainer/BUILD.bazel +++ b/pkg/sql/rowcontainer/BUILD.bazel @@ -28,6 +28,7 @@ go_library( "//pkg/sql/sqlerrors", "//pkg/sql/types", "//pkg/util", + "//pkg/util/bufalloc", "//pkg/util/cancelchecker", "//pkg/util/encoding", "//pkg/util/hlc", diff --git a/pkg/sql/rowcontainer/disk_row_container.go b/pkg/sql/rowcontainer/disk_row_container.go index d79479501806..88cc9aad7524 100644 --- a/pkg/sql/rowcontainer/disk_row_container.go +++ b/pkg/sql/rowcontainer/disk_row_container.go @@ -22,6 +22,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/rowenc" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" + "github.com/cockroachdb/cockroach/pkg/util/bufalloc" "github.com/cockroachdb/cockroach/pkg/util/encoding" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/mon" @@ -394,8 +395,9 @@ func (d *DiskRowContainer) Close(ctx context.Context) { } // keyValToRow decodes a key and a value byte slice stored with AddRow() into -// a sqlbase.EncDatumRow. The returned EncDatumRow is only valid until the next -// call to keyValToRow(). +// a rowenc.EncDatumRow. The returned EncDatumRow is only valid until the next +// call to keyValToRow(). The passed in byte slices are used directly, and it is +// the caller's responsibility to make sure they don't get modified. func (d *DiskRowContainer) keyValToRow(k []byte, v []byte) (rowenc.EncDatumRow, error) { for i, orderInfo := range d.ordering { // Types with composite key encodings are decoded from the value. @@ -430,7 +432,7 @@ func (d *DiskRowContainer) keyValToRow(k []byte, v []byte) (rowenc.EncDatumRow, // diskRowIterator iterates over the rows in a DiskRowContainer. type diskRowIterator struct { rowContainer *DiskRowContainer - rowBuf []byte + rowBuf bufalloc.ByteAllocator diskmap.SortedDiskMapIterator } @@ -452,7 +454,7 @@ func (d *DiskRowContainer) NewIterator(ctx context.Context) RowIterator { return &i } -// Row returns the current row. The returned sqlbase.EncDatumRow is only valid +// Row returns the current row. The returned rowenc.EncDatumRow is only valid // until the next call to Row(). func (r *diskRowIterator) Row() (rowenc.EncDatumRow, error) { if ok, err := r.Valid(); err != nil { @@ -463,19 +465,12 @@ func (r *diskRowIterator) Row() (rowenc.EncDatumRow, error) { k := r.UnsafeKey() v := r.UnsafeValue() - // TODO(asubiotto): the "true ||" should not be necessary. We should be to - // reuse rowBuf, yet doing so causes - // TestDiskBackedIndexedRowContainer/ReorderingOnDisk, TestHashJoiner, and - // TestSorter to fail. Some caller of Row() is presumably not making a copy - // of the return value. - if true || cap(r.rowBuf) < len(k)+len(v) { - r.rowBuf = make([]byte, 0, len(k)+len(v)) - } - r.rowBuf = r.rowBuf[:len(k)+len(v)] - copy(r.rowBuf, k) - copy(r.rowBuf[len(k):], v) - k = r.rowBuf[:len(k)] - v = r.rowBuf[len(k):] + // keyValToRow will use the encoded key and value bytes as is by shoving + // them directly into the EncDatum, so we need to make a copy here. We + // cannot reuse the same byte slice across Row() calls because it would lead + // to modification of the EncDatums (which is not allowed). + r.rowBuf, k = r.rowBuf.Copy(k, len(v)) + r.rowBuf, v = r.rowBuf.Copy(v, 0 /* extraCap */) return r.rowContainer.keyValToRow(k, v) } diff --git a/pkg/sql/rowcontainer/row_container.go b/pkg/sql/rowcontainer/row_container.go index fdd7ecab2eb5..2d16b21f5f07 100644 --- a/pkg/sql/rowcontainer/row_container.go +++ b/pkg/sql/rowcontainer/row_container.go @@ -132,7 +132,8 @@ type RowIterator interface { // Next advances the iterator to the next row in the iteration. Next() // Row returns the current row. The returned row is only valid until the - // next call to Rewind() or Next(). + // next call to Rewind() or Next(). However, datums in the row won't be + // modified, so shallow copying is sufficient. Row() (rowenc.EncDatumRow, error) // Close frees up resources held by the iterator. diff --git a/pkg/sql/virtual_table.go b/pkg/sql/virtual_table.go index f38054cd3584..3c7fa3790aa2 100644 --- a/pkg/sql/virtual_table.go +++ b/pkg/sql/virtual_table.go @@ -235,9 +235,13 @@ type vTableLookupJoinNode struct { // run contains the runtime state of this planNode. run struct { + // matched indicates whether the current input row had at least one + // match. + matched bool // row contains the next row to output. row tree.Datums - // rows contains the next rows to output, except for row. + // rows contains the next rows to output, except for row. Only allocated + // for inner and left outer joins. rows *rowcontainer.RowContainer keyCtx constraint.KeyContext @@ -257,10 +261,14 @@ var _ rowPusher = &vTableLookupJoinNode{} // startExec implements the planNode interface. func (v *vTableLookupJoinNode) startExec(params runParams) error { v.run.keyCtx = constraint.KeyContext{EvalCtx: params.EvalContext()} - v.run.rows = rowcontainer.NewRowContainer( - params.p.Mon().MakeBoundAccount(), - colinfo.ColTypeInfoFromResCols(v.columns), - ) + if v.joinType == descpb.InnerJoin || v.joinType == descpb.LeftOuterJoin { + v.run.rows = rowcontainer.NewRowContainer( + params.p.Mon().MakeBoundAccount(), + colinfo.ColTypeInfoFromResCols(v.columns), + ) + } else if v.joinType != descpb.LeftSemiJoin && v.joinType != descpb.LeftAntiJoin { + return errors.AssertionFailedf("unexpected join type for virtual lookup join: %s", v.joinType.String()) + } v.run.indexKeyDatums = make(tree.Datums, len(v.columns)) var err error db, err := params.p.Descriptors().GetImmutableDatabaseByName( @@ -285,7 +293,7 @@ func (v *vTableLookupJoinNode) Next(params runParams) (bool, error) { v.run.params = ¶ms for { // Check if there are any rows left to emit from the last input row. - if v.run.rows.Len() > 0 { + if v.run.rows != nil && v.run.rows.Len() > 0 { copy(v.run.row, v.run.rows.At(0)) v.run.rows.PopFirst(params.ctx) return true, nil @@ -315,18 +323,38 @@ func (v *vTableLookupJoinNode) Next(params runParams) (bool, error) { ) // Add the input row to the left of the scratch row. v.run.row = append(v.run.row[:0], inputRow...) + v.run.matched = false // Finally, we're ready to do the lookup. This invocation will push all of // the looked-up rows into v.run.rows. if err := genFunc(params.ctx, v); err != nil { return false, err } - if v.run.rows.Len() == 0 && v.joinType == descpb.LeftOuterJoin { - // No matches - construct an outer match. - v.run.row = v.run.row[:len(v.inputCols)] - for i := len(inputRow); i < len(v.columns); i++ { - v.run.row = append(v.run.row, tree.DNull) + switch v.joinType { + case descpb.LeftOuterJoin: + if !v.run.matched { + // No matches - construct an outer match. + v.run.row = v.run.row[:len(v.inputCols)] + for i := len(inputRow); i < len(v.columns); i++ { + v.run.row = append(v.run.row, tree.DNull) + } + return true, nil + } + case descpb.LeftSemiJoin: + if v.run.matched { + // This input row had a match, so it should be emitted. + // + // Reset our output row to just the contents of the input row. + v.run.row = v.run.row[:len(v.inputCols)] + return true, nil + } + case descpb.LeftAntiJoin: + if !v.run.matched { + // This input row didn't have a match, so it should be emitted. + // + // Reset our output row to just the contents of the input row. + v.run.row = v.run.row[:len(v.inputCols)] + return true, nil } - return true, nil } } } @@ -350,6 +378,12 @@ func (v *vTableLookupJoinNode) pushRow(lookedUpRow ...tree.Datum) error { ); !ok || err != nil { return err } + v.run.matched = true + if v.joinType == descpb.LeftSemiJoin || v.joinType == descpb.LeftAntiJoin { + // Avoid adding the row into the container since for left semi and left + // anti joins we only care to know whether there was a match or not. + return nil + } _, err := v.run.rows.AddRow(v.run.params.ctx, v.run.row) return err } @@ -362,5 +396,7 @@ func (v *vTableLookupJoinNode) Values() tree.Datums { // Close implements the planNode interface. func (v *vTableLookupJoinNode) Close(ctx context.Context) { v.input.Close(ctx) - v.run.rows.Close(ctx) + if v.run.rows != nil { + v.run.rows.Close(ctx) + } }