From 86ebe2b5eccbbe03540899c3a4d160384890f8ac Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Sun, 2 Jul 2023 02:42:38 -0400 Subject: [PATCH 1/5] sql/sqlstats: return iterators by value from constructors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit switches to returning sqlstats iterations by value from their constructors, instead of by pointer. This avoids causing the iterators to escape and needing to allocate on the heap. Instead, they can be kept on the stack, avoiding heap allocations in methods like `IterateStatementStats`. ``` ➜ benchdiff --run='BenchmarkKV/./SQL/rows=1$$' --count=25 ./pkg/sql/tests name old time/op new time/op delta KV/Insert/SQL/rows=1-10 173µs ±13% 172µs ±15% ~ (p=0.939 n=25+25) KV/Update/SQL/rows=1-10 285µs ±16% 280µs ±11% ~ (p=0.356 n=25+23) KV/Delete/SQL/rows=1-10 212µs ±25% 221µs ±19% ~ (p=0.345 n=25+25) KV/Scan/SQL/rows=1-10 127µs ±11% 127µs ±11% ~ (p=0.878 n=25+24) name old alloc/op new alloc/op delta KV/Scan/SQL/rows=1-10 32.2kB ± 1% 32.0kB ± 1% -0.36% (p=0.006 n=24+24) KV/Insert/SQL/rows=1-10 59.0kB ± 1% 58.9kB ± 1% ~ (p=0.140 n=25+25) KV/Update/SQL/rows=1-10 70.6kB ± 1% 70.4kB ± 1% ~ (p=0.050 n=24+24) KV/Delete/SQL/rows=1-10 84.9kB ± 1% 84.9kB ± 1% ~ (p=0.859 n=25+25) name old allocs/op new allocs/op delta KV/Scan/SQL/rows=1-10 368 ± 1% 366 ± 1% -0.62% (p=0.001 n=24+24) KV/Update/SQL/rows=1-10 726 ± 1% 723 ± 2% -0.45% (p=0.013 n=23+24) KV/Insert/SQL/rows=1-10 492 ± 2% 490 ± 1% -0.41% (p=0.044 n=25+25) KV/Delete/SQL/rows=1-10 613 ± 3% 613 ± 3% ~ (p=0.179 n=25+25) ``` Epic: None Release note: None --- .../persistedsqlstats/combined_iterator.go | 16 +++++++-------- .../persistedsqlstats/mem_iterator.go | 12 +++++------ pkg/sql/sqlstats/sslocal/sslocal_iterator.go | 16 +++++++-------- pkg/sql/sqlstats/sslocal/sslocal_provider.go | 4 ++-- .../sqlstats/ssmemstorage/ss_mem_iterator.go | 20 +++++++++++++------ .../sqlstats/ssmemstorage/ss_mem_storage.go | 4 ++-- 6 files changed, 40 insertions(+), 32 deletions(-) diff --git a/pkg/sql/sqlstats/persistedsqlstats/combined_iterator.go b/pkg/sql/sqlstats/persistedsqlstats/combined_iterator.go index 99f49dbd01e0..90e7a4535071 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/combined_iterator.go +++ b/pkg/sql/sqlstats/persistedsqlstats/combined_iterator.go @@ -29,7 +29,7 @@ type CombinedStmtStatsIterator struct { mem struct { canBeAdvanced bool paused bool - it *memStmtStatsIterator + it memStmtStatsIterator } disk struct { @@ -42,9 +42,9 @@ type CombinedStmtStatsIterator struct { // NewCombinedStmtStatsIterator returns a new instance of // CombinedStmtStatsIterator. func NewCombinedStmtStatsIterator( - memIter *memStmtStatsIterator, diskIter isql.Rows, expectedColCnt int, -) *CombinedStmtStatsIterator { - c := &CombinedStmtStatsIterator{ + memIter memStmtStatsIterator, diskIter isql.Rows, expectedColCnt int, +) CombinedStmtStatsIterator { + c := CombinedStmtStatsIterator{ expectedColCnt: expectedColCnt, } @@ -215,7 +215,7 @@ type CombinedTxnStatsIterator struct { mem struct { canBeAdvanced bool paused bool - it *memTxnStatsIterator + it memTxnStatsIterator } disk struct { @@ -228,9 +228,9 @@ type CombinedTxnStatsIterator struct { // NewCombinedTxnStatsIterator returns a new instance of // CombinedTxnStatsIterator. func NewCombinedTxnStatsIterator( - memIter *memTxnStatsIterator, diskIter isql.Rows, expectedColCnt int, -) *CombinedTxnStatsIterator { - c := &CombinedTxnStatsIterator{ + memIter memTxnStatsIterator, diskIter isql.Rows, expectedColCnt int, +) CombinedTxnStatsIterator { + c := CombinedTxnStatsIterator{ expectedColCnt: expectedColCnt, } diff --git a/pkg/sql/sqlstats/persistedsqlstats/mem_iterator.go b/pkg/sql/sqlstats/persistedsqlstats/mem_iterator.go index 4e74c7ec907b..75cef70e889a 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/mem_iterator.go +++ b/pkg/sql/sqlstats/persistedsqlstats/mem_iterator.go @@ -25,7 +25,7 @@ import ( // and aggregation_interval fields on the returning // appstatspb.CollectedStatementStatistics. type memStmtStatsIterator struct { - *sslocal.StmtStatsIterator + sslocal.StmtStatsIterator aggregatedTs time.Time aggInterval time.Duration } @@ -35,8 +35,8 @@ func newMemStmtStatsIterator( options *sqlstats.IteratorOptions, aggregatedTS time.Time, aggInterval time.Duration, -) *memStmtStatsIterator { - return &memStmtStatsIterator{ +) memStmtStatsIterator { + return memStmtStatsIterator{ StmtStatsIterator: stats.StmtStatsIterator(options), aggregatedTs: aggregatedTS, aggInterval: aggInterval, @@ -59,7 +59,7 @@ func (m *memStmtStatsIterator) Cur() *appstatspb.CollectedStatementStatistics { // aggregatoin_interval fields fields on the returning // appstatspb.CollectedTransactionStatistics. type memTxnStatsIterator struct { - *sslocal.TxnStatsIterator + sslocal.TxnStatsIterator aggregatedTs time.Time aggInterval time.Duration } @@ -69,8 +69,8 @@ func newMemTxnStatsIterator( options *sqlstats.IteratorOptions, aggregatedTS time.Time, aggInterval time.Duration, -) *memTxnStatsIterator { - return &memTxnStatsIterator{ +) memTxnStatsIterator { + return memTxnStatsIterator{ TxnStatsIterator: stats.TxnStatsIterator(options), aggregatedTs: aggregatedTS, aggInterval: aggInterval, diff --git a/pkg/sql/sqlstats/sslocal/sslocal_iterator.go b/pkg/sql/sqlstats/sslocal/sslocal_iterator.go index c704c5d56095..08ad90e4f085 100644 --- a/pkg/sql/sqlstats/sslocal/sslocal_iterator.go +++ b/pkg/sql/sqlstats/sslocal/sslocal_iterator.go @@ -27,14 +27,14 @@ type baseIterator struct { // statement statistics stored in SQLStats. type StmtStatsIterator struct { baseIterator - curIter *ssmemstorage.StmtStatsIterator + curIter ssmemstorage.StmtStatsIterator } // NewStmtStatsIterator returns a new instance of the StmtStatsIterator. -func NewStmtStatsIterator(s *SQLStats, options *sqlstats.IteratorOptions) *StmtStatsIterator { +func NewStmtStatsIterator(s *SQLStats, options *sqlstats.IteratorOptions) StmtStatsIterator { appNames := s.getAppNames(options.SortedAppNames) - return &StmtStatsIterator{ + return StmtStatsIterator{ baseIterator: baseIterator{ sqlStats: s, idx: -1, @@ -53,7 +53,7 @@ func NewStmtStatsIterator(s *SQLStats, options *sqlstats.IteratorOptions) *StmtS func (s *StmtStatsIterator) Next() bool { // If we haven't called Next() for the first time or our current child // iterator has finished iterator, then we increment s.idx. - if s.curIter == nil || !s.curIter.Next() { + if !s.curIter.Initialized() || !s.curIter.Next() { s.idx++ if s.idx >= len(s.appNames) { return false @@ -78,14 +78,14 @@ func (s *StmtStatsIterator) Cur() *appstatspb.CollectedStatementStatistics { // statement statistics stored in SQLStats. type TxnStatsIterator struct { baseIterator - curIter *ssmemstorage.TxnStatsIterator + curIter ssmemstorage.TxnStatsIterator } // NewTxnStatsIterator returns a new instance of the TxnStatsIterator. -func NewTxnStatsIterator(s *SQLStats, options *sqlstats.IteratorOptions) *TxnStatsIterator { +func NewTxnStatsIterator(s *SQLStats, options *sqlstats.IteratorOptions) TxnStatsIterator { appNames := s.getAppNames(options.SortedAppNames) - return &TxnStatsIterator{ + return TxnStatsIterator{ baseIterator: baseIterator{ sqlStats: s, idx: -1, @@ -104,7 +104,7 @@ func NewTxnStatsIterator(s *SQLStats, options *sqlstats.IteratorOptions) *TxnSta func (t *TxnStatsIterator) Next() bool { // If we haven't called Next() for the first time or our current child // iterator has finished iterator, then we increment s.idx. - if t.curIter == nil || !t.curIter.Next() { + if !t.curIter.Initialized() || !t.curIter.Next() { t.idx++ if t.idx >= len(t.appNames) { return false diff --git a/pkg/sql/sqlstats/sslocal/sslocal_provider.go b/pkg/sql/sqlstats/sslocal/sslocal_provider.go index a81e03c9b37c..9e68b2a66162 100644 --- a/pkg/sql/sqlstats/sslocal/sslocal_provider.go +++ b/pkg/sql/sqlstats/sslocal/sslocal_provider.go @@ -145,7 +145,7 @@ func (s *SQLStats) IterateStatementStats( // StmtStatsIterator returns an instance of sslocal.StmtStatsIterator for // the current SQLStats. -func (s *SQLStats) StmtStatsIterator(options *sqlstats.IteratorOptions) *StmtStatsIterator { +func (s *SQLStats) StmtStatsIterator(options *sqlstats.IteratorOptions) StmtStatsIterator { return NewStmtStatsIterator(s, options) } @@ -167,7 +167,7 @@ func (s *SQLStats) IterateTransactionStats( // TxnStatsIterator returns an instance of sslocal.TxnStatsIterator for // the current SQLStats. -func (s *SQLStats) TxnStatsIterator(options *sqlstats.IteratorOptions) *TxnStatsIterator { +func (s *SQLStats) TxnStatsIterator(options *sqlstats.IteratorOptions) TxnStatsIterator { return NewTxnStatsIterator(s, options) } diff --git a/pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go b/pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go index d50715209daf..55b05a7028f1 100644 --- a/pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go +++ b/pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go @@ -33,7 +33,7 @@ type StmtStatsIterator struct { // NewStmtStatsIterator returns a StmtStatsIterator. func NewStmtStatsIterator( container *Container, options *sqlstats.IteratorOptions, -) *StmtStatsIterator { +) StmtStatsIterator { var stmtKeys stmtList container.mu.Lock() for k := range container.mu.stmts { @@ -44,7 +44,7 @@ func NewStmtStatsIterator( sort.Sort(stmtKeys) } - return &StmtStatsIterator{ + return StmtStatsIterator{ baseIterator: baseIterator{ container: container, idx: -1, @@ -53,6 +53,11 @@ func NewStmtStatsIterator( } } +// Initialized returns true if the iterator has been initialized, false otherwise. +func (s *StmtStatsIterator) Initialized() bool { + return s.container != nil +} + // Next updates the current value returned by the subsequent Cur() call. Next() // returns true if the following Cur() call is valid, false otherwise. func (s *StmtStatsIterator) Next() bool { @@ -119,9 +124,7 @@ type TxnStatsIterator struct { } // NewTxnStatsIterator returns a new instance of TxnStatsIterator. -func NewTxnStatsIterator( - container *Container, options *sqlstats.IteratorOptions, -) *TxnStatsIterator { +func NewTxnStatsIterator(container *Container, options *sqlstats.IteratorOptions) TxnStatsIterator { var txnKeys txnList container.mu.Lock() for k := range container.mu.txns { @@ -132,7 +135,7 @@ func NewTxnStatsIterator( sort.Sort(txnKeys) } - return &TxnStatsIterator{ + return TxnStatsIterator{ baseIterator: baseIterator{ container: container, idx: -1, @@ -141,6 +144,11 @@ func NewTxnStatsIterator( } } +// Initialized returns true if the iterator has been initialized, false otherwise. +func (t *TxnStatsIterator) Initialized() bool { + return t.container != nil +} + // Next updates the current value returned by the subsequent Cur() call. Next() // returns true if the following Cur() call is valid, false otherwise. func (t *TxnStatsIterator) Next() bool { diff --git a/pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go b/pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go index 2fc46dbbde36..d8bfe6d97c63 100644 --- a/pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go +++ b/pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go @@ -184,12 +184,12 @@ func (s *Container) IterateAggregatedTransactionStats( } // StmtStatsIterator returns an instance of StmtStatsIterator. -func (s *Container) StmtStatsIterator(options *sqlstats.IteratorOptions) *StmtStatsIterator { +func (s *Container) StmtStatsIterator(options *sqlstats.IteratorOptions) StmtStatsIterator { return NewStmtStatsIterator(s, options) } // TxnStatsIterator returns an instance of TxnStatsIterator. -func (s *Container) TxnStatsIterator(options *sqlstats.IteratorOptions) *TxnStatsIterator { +func (s *Container) TxnStatsIterator(options *sqlstats.IteratorOptions) TxnStatsIterator { return NewTxnStatsIterator(s, options) } From b7dd1035c337c6705d5d25ae49b3d6370ccd3e90 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Sun, 2 Jul 2023 03:01:30 -0400 Subject: [PATCH 2/5] sql/sqlstats: pass `sqlstats.IteratorOptions` by value, not pointer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit switches from passing the `sqlstats.IteratorOptions` around by pointer to passing it by value. The struct is 2 bytes large (8 when padded), so there's little benefit to passing it around by pointer. Meanwhile, passing the object by pointer through interface methods prevents escape analysis from keeping it on the stack, forcing a heap allocation. ``` ➜ benchdiff --run='BenchmarkKV/./SQL/rows=1$$' --count=25 ./pkg/sql/tests name old time/op new time/op delta KV/Insert/SQL/rows=1-10 169µs ±13% 168µs ±17% ~ (p=0.603 n=25+25) KV/Update/SQL/rows=1-10 282µs ± 9% 282µs ± 8% ~ (p=0.788 n=25+25) KV/Delete/SQL/rows=1-10 227µs ±26% 206µs ±27% ~ (p=0.074 n=25+25) KV/Scan/SQL/rows=1-10 126µs ±12% 127µs ±16% ~ (p=0.908 n=25+25) name old alloc/op new alloc/op delta KV/Delete/SQL/rows=1-10 84.9kB ± 1% 84.5kB ± 1% -0.50% (p=0.008 n=25+21) KV/Insert/SQL/rows=1-10 58.9kB ± 1% 58.7kB ± 1% -0.23% (p=0.020 n=25+23) KV/Update/SQL/rows=1-10 70.5kB ± 1% 70.5kB ± 1% ~ (p=0.894 n=24+24) KV/Scan/SQL/rows=1-10 32.0kB ± 1% 32.0kB ± 1% ~ (p=0.631 n=25+24) name old allocs/op new allocs/op delta KV/Delete/SQL/rows=1-10 613 ± 3% 604 ± 1% -1.52% (p=0.000 n=25+21) KV/Insert/SQL/rows=1-10 489 ± 1% 486 ± 1% -0.69% (p=0.001 n=25+23) KV/Scan/SQL/rows=1-10 365 ± 1% 363 ± 1% -0.42% (p=0.024 n=25+25) KV/Update/SQL/rows=1-10 724 ± 2% 722 ± 2% ~ (p=0.302 n=25+24) ``` Epic: None Release note: None --- pkg/sql/conn_executor.go | 9 +++------ pkg/sql/crdb_internal.go | 10 +++++----- pkg/sql/instrumentation_test.go | 4 ++-- pkg/sql/sqlstats/persistedsqlstats/flush.go | 4 ++-- pkg/sql/sqlstats/persistedsqlstats/flush_test.go | 4 ++-- pkg/sql/sqlstats/persistedsqlstats/mem_iterator.go | 4 ++-- pkg/sql/sqlstats/persistedsqlstats/reader_test.go | 8 ++++---- pkg/sql/sqlstats/persistedsqlstats/stmt_reader.go | 6 +++--- pkg/sql/sqlstats/persistedsqlstats/txn_reader.go | 6 +++--- pkg/sql/sqlstats/sslocal/iterator_test.go | 6 +++--- pkg/sql/sqlstats/sslocal/sql_stats_test.go | 8 ++++---- pkg/sql/sqlstats/sslocal/sslocal_iterator.go | 6 +++--- pkg/sql/sqlstats/sslocal/sslocal_provider.go | 10 +++++----- .../sqlstats/sslocal/sslocal_stats_collector.go | 2 +- pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go | 4 ++-- pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go | 14 +++++++------- pkg/sql/sqlstats/ssprovider.go | 6 +++--- 17 files changed, 54 insertions(+), 57 deletions(-) diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index ead2a081d4c3..fbe21162c6d5 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -678,8 +678,7 @@ func (s *Server) GetUnscrubbedStmtStats( stmtStats = append(stmtStats, *stat) return nil } - err := - s.sqlStats.GetLocalMemProvider().IterateStatementStats(ctx, &sqlstats.IteratorOptions{}, stmtStatsVisitor) + err := s.sqlStats.GetLocalMemProvider().IterateStatementStats(ctx, sqlstats.IteratorOptions{}, stmtStatsVisitor) if err != nil { return nil, errors.Wrap(err, "failed to fetch statement stats") @@ -698,8 +697,7 @@ func (s *Server) GetUnscrubbedTxnStats( txnStats = append(txnStats, *stat) return nil } - err := - s.sqlStats.GetLocalMemProvider().IterateTransactionStats(ctx, &sqlstats.IteratorOptions{}, txnStatsVisitor) + err := s.sqlStats.GetLocalMemProvider().IterateTransactionStats(ctx, sqlstats.IteratorOptions{}, txnStatsVisitor) if err != nil { return nil, errors.Wrap(err, "failed to fetch statement stats") @@ -748,8 +746,7 @@ func (s *Server) getScrubbedStmtStats( return nil } - err := - statsProvider.IterateStatementStats(ctx, &sqlstats.IteratorOptions{}, stmtStatsVisitor) + err := statsProvider.IterateStatementStats(ctx, sqlstats.IteratorOptions{}, stmtStatsVisitor) if err != nil { return nil, errors.Wrap(err, "failed to fetch scrubbed statement stats") diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 4adc544dcedb..cb3fc2865425 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -1659,7 +1659,7 @@ CREATE TABLE crdb_internal.node_statement_statistics ( return nil } - return sqlStats.GetLocalMemProvider().IterateStatementStats(ctx, &sqlstats.IteratorOptions{ + return sqlStats.GetLocalMemProvider().IterateStatementStats(ctx, sqlstats.IteratorOptions{ SortedAppNames: true, SortedKey: true, }, statementVisitor) @@ -1817,7 +1817,7 @@ CREATE TABLE crdb_internal.node_transaction_statistics ( return nil } - return sqlStats.GetLocalMemProvider().IterateTransactionStats(ctx, &sqlstats.IteratorOptions{ + return sqlStats.GetLocalMemProvider().IterateTransactionStats(ctx, sqlstats.IteratorOptions{ SortedAppNames: true, SortedKey: true, }, transactionVisitor) @@ -1857,7 +1857,7 @@ CREATE TABLE crdb_internal.node_txn_stats ( ) } - return sqlStats.IterateAggregatedTransactionStats(ctx, &sqlstats.IteratorOptions{ + return sqlStats.IterateAggregatedTransactionStats(ctx, sqlstats.IteratorOptions{ SortedAppNames: true, }, appTxnStatsVisitor) }, @@ -6396,7 +6396,7 @@ CREATE TABLE crdb_internal.cluster_statement_statistics ( row := make(tree.Datums, 9 /* number of columns for this virtual table */) worker := func(ctx context.Context, pusher rowPusher) error { - return memSQLStats.IterateStatementStats(ctx, &sqlstats.IteratorOptions{ + return memSQLStats.IterateStatementStats(ctx, sqlstats.IteratorOptions{ SortedAppNames: true, SortedKey: true, }, func(ctx context.Context, statistics *appstatspb.CollectedStatementStatistics) error { @@ -6798,7 +6798,7 @@ CREATE TABLE crdb_internal.cluster_transaction_statistics ( row := make(tree.Datums, 5 /* number of columns for this virtual table */) worker := func(ctx context.Context, pusher rowPusher) error { - return memSQLStats.IterateTransactionStats(ctx, &sqlstats.IteratorOptions{ + return memSQLStats.IterateTransactionStats(ctx, sqlstats.IteratorOptions{ SortedAppNames: true, SortedKey: true, }, func( diff --git a/pkg/sql/instrumentation_test.go b/pkg/sql/instrumentation_test.go index a2aba5c80ad5..f33d75b415e9 100644 --- a/pkg/sql/instrumentation_test.go +++ b/pkg/sql/instrumentation_test.go @@ -60,7 +60,7 @@ func TestSampledStatsCollection(t *testing.T) { GetLocalMemProvider(). IterateStatementStats( ctx, - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func(ctx context.Context, statistics *appstatspb.CollectedStatementStatistics) error { if statistics.Key.Query == key.Query && statistics.Key.ImplicitTxn == key.ImplicitTxn && @@ -89,7 +89,7 @@ func TestSampledStatsCollection(t *testing.T) { GetLocalMemProvider(). IterateTransactionStats( ctx, - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func(ctx context.Context, statistics *appstatspb.CollectedTransactionStatistics) error { if statistics.TransactionFingerprintID == key { stats = statistics diff --git a/pkg/sql/sqlstats/persistedsqlstats/flush.go b/pkg/sql/sqlstats/persistedsqlstats/flush.go index 8345c315cfce..836262880548 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/flush.go +++ b/pkg/sql/sqlstats/persistedsqlstats/flush.go @@ -145,7 +145,7 @@ FROM func (s *PersistedSQLStats) flushStmtStats(ctx context.Context, aggregatedTs time.Time) { // s.doFlush directly logs errors if they are encountered. Therefore, // no error is returned here. - _ = s.SQLStats.IterateStatementStats(ctx, &sqlstats.IteratorOptions{}, + _ = s.SQLStats.IterateStatementStats(ctx, sqlstats.IteratorOptions{}, func(ctx context.Context, statistics *appstatspb.CollectedStatementStatistics) error { s.doFlush(ctx, func() error { return s.doFlushSingleStmtStats(ctx, statistics, aggregatedTs) @@ -160,7 +160,7 @@ func (s *PersistedSQLStats) flushStmtStats(ctx context.Context, aggregatedTs tim } func (s *PersistedSQLStats) flushTxnStats(ctx context.Context, aggregatedTs time.Time) { - _ = s.SQLStats.IterateTransactionStats(ctx, &sqlstats.IteratorOptions{}, + _ = s.SQLStats.IterateTransactionStats(ctx, sqlstats.IteratorOptions{}, func(ctx context.Context, statistics *appstatspb.CollectedTransactionStatistics) error { s.doFlush(ctx, func() error { return s.doFlushSingleTxnStats(ctx, statistics, aggregatedTs) diff --git a/pkg/sql/sqlstats/persistedsqlstats/flush_test.go b/pkg/sql/sqlstats/persistedsqlstats/flush_test.go index cd7fcff0bdd5..6fdbd65c5572 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/flush_test.go +++ b/pkg/sql/sqlstats/persistedsqlstats/flush_test.go @@ -651,7 +651,7 @@ func verifyInMemoryStatsCorrectness( t *testing.T, tcs []testCase, statsProvider *persistedsqlstats.PersistedSQLStats, ) { for _, tc := range tcs { - err := statsProvider.SQLStats.IterateStatementStats(context.Background(), &sqlstats.IteratorOptions{}, func(ctx context.Context, statistics *appstatspb.CollectedStatementStatistics) error { + err := statsProvider.SQLStats.IterateStatementStats(context.Background(), sqlstats.IteratorOptions{}, func(ctx context.Context, statistics *appstatspb.CollectedStatementStatistics) error { if tc.fingerprint == statistics.Key.Query { require.Equal(t, tc.count, statistics.Stats.Count, "fingerprint: %s", tc.fingerprint) } @@ -678,7 +678,7 @@ func verifyInMemoryStatsEmpty( t *testing.T, tcs []testCase, statsProvider *persistedsqlstats.PersistedSQLStats, ) { for _, tc := range tcs { - err := statsProvider.SQLStats.IterateStatementStats(context.Background(), &sqlstats.IteratorOptions{}, func(ctx context.Context, statistics *appstatspb.CollectedStatementStatistics) error { + err := statsProvider.SQLStats.IterateStatementStats(context.Background(), sqlstats.IteratorOptions{}, func(ctx context.Context, statistics *appstatspb.CollectedStatementStatistics) error { if tc.fingerprint == statistics.Key.Query { require.Equal(t, 0 /* expected */, statistics.Stats.Count, "fingerprint: %s", tc.fingerprint) } diff --git a/pkg/sql/sqlstats/persistedsqlstats/mem_iterator.go b/pkg/sql/sqlstats/persistedsqlstats/mem_iterator.go index 75cef70e889a..80e542057f36 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/mem_iterator.go +++ b/pkg/sql/sqlstats/persistedsqlstats/mem_iterator.go @@ -32,7 +32,7 @@ type memStmtStatsIterator struct { func newMemStmtStatsIterator( stats *sslocal.SQLStats, - options *sqlstats.IteratorOptions, + options sqlstats.IteratorOptions, aggregatedTS time.Time, aggInterval time.Duration, ) memStmtStatsIterator { @@ -66,7 +66,7 @@ type memTxnStatsIterator struct { func newMemTxnStatsIterator( stats *sslocal.SQLStats, - options *sqlstats.IteratorOptions, + options sqlstats.IteratorOptions, aggregatedTS time.Time, aggInterval time.Duration, ) memTxnStatsIterator { diff --git a/pkg/sql/sqlstats/persistedsqlstats/reader_test.go b/pkg/sql/sqlstats/persistedsqlstats/reader_test.go index 5e90dbd48a5a..41e89e3fea1c 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/reader_test.go +++ b/pkg/sql/sqlstats/persistedsqlstats/reader_test.go @@ -93,7 +93,7 @@ func TestPersistedSQLStatsRead(t *testing.T) { require.NoError(t, sqlStats.IterateStatementStats( context.Background(), - &sqlstats.IteratorOptions{ + sqlstats.IteratorOptions{ SortedKey: true, SortedAppNames: true, }, @@ -115,7 +115,7 @@ func TestPersistedSQLStatsRead(t *testing.T) { require.NoError(t, sqlStats.IterateTransactionStats( context.Background(), - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func( ctx context.Context, statistics *appstatspb.CollectedTransactionStatistics, @@ -215,7 +215,7 @@ func verifyStoredStmtFingerprints( require.NoError(t, sqlStats.IterateStatementStats( context.Background(), - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func(ctx context.Context, statistics *appstatspb.CollectedStatementStatistics) error { if expectedExecCount, ok := expectedStmtFingerprints[statistics.Key.Query]; ok { foundQueries[statistics.Key.Query] = struct{}{} @@ -228,7 +228,7 @@ func verifyStoredStmtFingerprints( require.NoError(t, sqlStats.IterateTransactionStats( context.Background(), - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func( ctx context.Context, statistics *appstatspb.CollectedTransactionStatistics, diff --git a/pkg/sql/sqlstats/persistedsqlstats/stmt_reader.go b/pkg/sql/sqlstats/persistedsqlstats/stmt_reader.go index f4b024c38289..b55a3322f66e 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/stmt_reader.go +++ b/pkg/sql/sqlstats/persistedsqlstats/stmt_reader.go @@ -28,7 +28,7 @@ import ( // IterateStatementStats implements sqlstats.Provider interface. func (s *PersistedSQLStats) IterateStatementStats( - ctx context.Context, options *sqlstats.IteratorOptions, visitor sqlstats.StatementVisitor, + ctx context.Context, options sqlstats.IteratorOptions, visitor sqlstats.StatementVisitor, ) (err error) { // We override the sorting options since otherwise we would need to implement // sorted and unsorted merge separately. We can revisit this decision if @@ -79,7 +79,7 @@ func (s *PersistedSQLStats) IterateStatementStats( } func (s *PersistedSQLStats) persistedStmtStatsIter( - ctx context.Context, options *sqlstats.IteratorOptions, + ctx context.Context, options sqlstats.IteratorOptions, ) (iter isql.Rows, expectedColCnt int, err error) { query, expectedColCnt := s.getFetchQueryForStmtStatsTable(ctx, options) @@ -99,7 +99,7 @@ func (s *PersistedSQLStats) persistedStmtStatsIter( } func (s *PersistedSQLStats) getFetchQueryForStmtStatsTable( - ctx context.Context, options *sqlstats.IteratorOptions, + ctx context.Context, options sqlstats.IteratorOptions, ) (query string, colCnt int) { selectedColumns := []string{ "aggregated_ts", diff --git a/pkg/sql/sqlstats/persistedsqlstats/txn_reader.go b/pkg/sql/sqlstats/persistedsqlstats/txn_reader.go index 71b6921c3ea9..1f73de1a6490 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/txn_reader.go +++ b/pkg/sql/sqlstats/persistedsqlstats/txn_reader.go @@ -27,7 +27,7 @@ import ( // IterateTransactionStats implements sqlstats.Provider interface. func (s *PersistedSQLStats) IterateTransactionStats( - ctx context.Context, options *sqlstats.IteratorOptions, visitor sqlstats.TransactionVisitor, + ctx context.Context, options sqlstats.IteratorOptions, visitor sqlstats.TransactionVisitor, ) (err error) { // We override the sorting options since otherwise we would need to implement // sorted and unsorted merge separately. We can revisit this decision if @@ -78,7 +78,7 @@ func (s *PersistedSQLStats) IterateTransactionStats( } func (s *PersistedSQLStats) persistedTxnStatsIter( - ctx context.Context, options *sqlstats.IteratorOptions, + ctx context.Context, options sqlstats.IteratorOptions, ) (iter isql.Rows, expectedColCnt int, err error) { query, expectedColCnt := s.getFetchQueryForTxnStatsTable(options) exec := s.cfg.DB.Executor() @@ -96,7 +96,7 @@ func (s *PersistedSQLStats) persistedTxnStatsIter( } func (s *PersistedSQLStats) getFetchQueryForTxnStatsTable( - options *sqlstats.IteratorOptions, + options sqlstats.IteratorOptions, ) (query string, colCnt int) { selectedColumns := []string{ "aggregated_ts", diff --git a/pkg/sql/sqlstats/sslocal/iterator_test.go b/pkg/sql/sqlstats/sslocal/iterator_test.go index 602cb7705c0a..ff434f65e6bd 100644 --- a/pkg/sql/sqlstats/sslocal/iterator_test.go +++ b/pkg/sql/sqlstats/sslocal/iterator_test.go @@ -51,7 +51,7 @@ func TestSQLStatsIteratorWithTelemetryFlush(t *testing.T) { // transaction stats later. fingerprintIDs := make(map[appstatspb.StmtFingerprintID]struct{}) require.NoError(t, - sqlStats.IterateStatementStats(ctx, &sqlstats.IteratorOptions{}, + sqlStats.IterateStatementStats(ctx, sqlstats.IteratorOptions{}, func(_ context.Context, statistics *appstatspb.CollectedStatementStatistics) error { fingerprintIDs[statistics.ID] = struct{}{} return nil @@ -61,7 +61,7 @@ func TestSQLStatsIteratorWithTelemetryFlush(t *testing.T) { require.NoError(t, sqlStats.IterateStatementStats( ctx, - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func(_ context.Context, statistics *appstatspb.CollectedStatementStatistics) error { require.NotNil(t, statistics) // If we are running our test case, we reset the SQL Stats. The iterator @@ -80,7 +80,7 @@ func TestSQLStatsIteratorWithTelemetryFlush(t *testing.T) { require.NoError(t, sqlStats.IterateTransactionStats( ctx, - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func( ctx context.Context, statistics *appstatspb.CollectedTransactionStatistics, diff --git a/pkg/sql/sqlstats/sslocal/sql_stats_test.go b/pkg/sql/sqlstats/sslocal/sql_stats_test.go index 1479047f0045..a474ab5eefd8 100644 --- a/pkg/sql/sqlstats/sslocal/sql_stats_test.go +++ b/pkg/sql/sqlstats/sslocal/sql_stats_test.go @@ -77,7 +77,7 @@ func TestStmtStatsBulkIngestWithRandomMetadata(t *testing.T) { require.NoError(t, sqlStats.IterateStatementStats( context.Background(), - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func( ctx context.Context, statistics *appstatspb.CollectedStatementStatistics, @@ -196,7 +196,7 @@ func TestSQLStatsStmtStatsBulkIngest(t *testing.T) { require.NoError(t, sqlStats.IterateStatementStats( context.Background(), - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func( ctx context.Context, statistics *appstatspb.CollectedStatementStatistics, @@ -294,7 +294,7 @@ func TestSQLStatsTxnStatsBulkIngest(t *testing.T) { require.NoError(t, sqlStats.IterateTransactionStats( context.Background(), - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func( ctx context.Context, statistics *appstatspb.CollectedTransactionStatistics, @@ -621,7 +621,7 @@ func TestAssociatingStmtStatsWithTxnFingerprint(t *testing.T) { var stats []*appstatspb.CollectedStatementStatistics err = statsCollector.IterateStatementStats( ctx, - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func(_ context.Context, s *appstatspb.CollectedStatementStatistics) error { stats = append(stats, s) return nil diff --git a/pkg/sql/sqlstats/sslocal/sslocal_iterator.go b/pkg/sql/sqlstats/sslocal/sslocal_iterator.go index 08ad90e4f085..f5077e93fa2c 100644 --- a/pkg/sql/sqlstats/sslocal/sslocal_iterator.go +++ b/pkg/sql/sqlstats/sslocal/sslocal_iterator.go @@ -20,7 +20,7 @@ type baseIterator struct { sqlStats *SQLStats idx int appNames []string - options *sqlstats.IteratorOptions + options sqlstats.IteratorOptions } // StmtStatsIterator is an iterator that can be used to iterate over the @@ -31,7 +31,7 @@ type StmtStatsIterator struct { } // NewStmtStatsIterator returns a new instance of the StmtStatsIterator. -func NewStmtStatsIterator(s *SQLStats, options *sqlstats.IteratorOptions) StmtStatsIterator { +func NewStmtStatsIterator(s *SQLStats, options sqlstats.IteratorOptions) StmtStatsIterator { appNames := s.getAppNames(options.SortedAppNames) return StmtStatsIterator{ @@ -82,7 +82,7 @@ type TxnStatsIterator struct { } // NewTxnStatsIterator returns a new instance of the TxnStatsIterator. -func NewTxnStatsIterator(s *SQLStats, options *sqlstats.IteratorOptions) TxnStatsIterator { +func NewTxnStatsIterator(s *SQLStats, options sqlstats.IteratorOptions) TxnStatsIterator { appNames := s.getAppNames(options.SortedAppNames) return TxnStatsIterator{ diff --git a/pkg/sql/sqlstats/sslocal/sslocal_provider.go b/pkg/sql/sqlstats/sslocal/sslocal_provider.go index 9e68b2a66162..ea0f0fbbbba4 100644 --- a/pkg/sql/sqlstats/sslocal/sslocal_provider.go +++ b/pkg/sql/sqlstats/sslocal/sslocal_provider.go @@ -130,7 +130,7 @@ func (s *SQLStats) GetLastReset() time.Time { // IterateStatementStats implements sqlstats.Provider interface. func (s *SQLStats) IterateStatementStats( - ctx context.Context, options *sqlstats.IteratorOptions, visitor sqlstats.StatementVisitor, + ctx context.Context, options sqlstats.IteratorOptions, visitor sqlstats.StatementVisitor, ) error { iter := s.StmtStatsIterator(options) @@ -145,13 +145,13 @@ func (s *SQLStats) IterateStatementStats( // StmtStatsIterator returns an instance of sslocal.StmtStatsIterator for // the current SQLStats. -func (s *SQLStats) StmtStatsIterator(options *sqlstats.IteratorOptions) StmtStatsIterator { +func (s *SQLStats) StmtStatsIterator(options sqlstats.IteratorOptions) StmtStatsIterator { return NewStmtStatsIterator(s, options) } // IterateTransactionStats implements sqlstats.Provider interface. func (s *SQLStats) IterateTransactionStats( - ctx context.Context, options *sqlstats.IteratorOptions, visitor sqlstats.TransactionVisitor, + ctx context.Context, options sqlstats.IteratorOptions, visitor sqlstats.TransactionVisitor, ) error { iter := s.TxnStatsIterator(options) @@ -167,14 +167,14 @@ func (s *SQLStats) IterateTransactionStats( // TxnStatsIterator returns an instance of sslocal.TxnStatsIterator for // the current SQLStats. -func (s *SQLStats) TxnStatsIterator(options *sqlstats.IteratorOptions) TxnStatsIterator { +func (s *SQLStats) TxnStatsIterator(options sqlstats.IteratorOptions) TxnStatsIterator { return NewTxnStatsIterator(s, options) } // IterateAggregatedTransactionStats implements sqlstats.Provider interface. func (s *SQLStats) IterateAggregatedTransactionStats( ctx context.Context, - options *sqlstats.IteratorOptions, + options sqlstats.IteratorOptions, visitor sqlstats.AggregatedTransactionVisitor, ) error { appNames := s.getAppNames(options.SortedAppNames) diff --git a/pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go b/pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go index 0acc363ddd2a..34e5db02d6ed 100644 --- a/pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go +++ b/pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go @@ -134,7 +134,7 @@ func (s *StatsCollector) ShouldSample( // UpgradeImplicitTxn implements sqlstats.StatsCollector interface. func (s *StatsCollector) UpgradeImplicitTxn(ctx context.Context) error { - err := s.ApplicationStats.IterateStatementStats(ctx, &sqlstats.IteratorOptions{}, + err := s.ApplicationStats.IterateStatementStats(ctx, sqlstats.IteratorOptions{}, func(_ context.Context, statistics *appstatspb.CollectedStatementStatistics) error { statistics.Key.ImplicitTxn = false return nil diff --git a/pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go b/pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go index 55b05a7028f1..21b5650f7c31 100644 --- a/pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go +++ b/pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go @@ -32,7 +32,7 @@ type StmtStatsIterator struct { // NewStmtStatsIterator returns a StmtStatsIterator. func NewStmtStatsIterator( - container *Container, options *sqlstats.IteratorOptions, + container *Container, options sqlstats.IteratorOptions, ) StmtStatsIterator { var stmtKeys stmtList container.mu.Lock() @@ -124,7 +124,7 @@ type TxnStatsIterator struct { } // NewTxnStatsIterator returns a new instance of TxnStatsIterator. -func NewTxnStatsIterator(container *Container, options *sqlstats.IteratorOptions) TxnStatsIterator { +func NewTxnStatsIterator(container *Container, options sqlstats.IteratorOptions) TxnStatsIterator { var txnKeys txnList container.mu.Lock() for k := range container.mu.txns { diff --git a/pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go b/pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go index d8bfe6d97c63..8d8f57b87b0a 100644 --- a/pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go +++ b/pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go @@ -167,7 +167,7 @@ func New( // IterateAggregatedTransactionStats implements sqlstats.ApplicationStats // interface. func (s *Container) IterateAggregatedTransactionStats( - _ context.Context, _ *sqlstats.IteratorOptions, visitor sqlstats.AggregatedTransactionVisitor, + _ context.Context, _ sqlstats.IteratorOptions, visitor sqlstats.AggregatedTransactionVisitor, ) error { txnStat := func() appstatspb.TxnStats { s.txnCounts.mu.Lock() @@ -184,18 +184,18 @@ func (s *Container) IterateAggregatedTransactionStats( } // StmtStatsIterator returns an instance of StmtStatsIterator. -func (s *Container) StmtStatsIterator(options *sqlstats.IteratorOptions) StmtStatsIterator { +func (s *Container) StmtStatsIterator(options sqlstats.IteratorOptions) StmtStatsIterator { return NewStmtStatsIterator(s, options) } // TxnStatsIterator returns an instance of TxnStatsIterator. -func (s *Container) TxnStatsIterator(options *sqlstats.IteratorOptions) TxnStatsIterator { +func (s *Container) TxnStatsIterator(options sqlstats.IteratorOptions) TxnStatsIterator { return NewTxnStatsIterator(s, options) } // IterateStatementStats implements sqlstats.Provider interface. func (s *Container) IterateStatementStats( - ctx context.Context, options *sqlstats.IteratorOptions, visitor sqlstats.StatementVisitor, + ctx context.Context, options sqlstats.IteratorOptions, visitor sqlstats.StatementVisitor, ) error { iter := s.StmtStatsIterator(options) @@ -210,7 +210,7 @@ func (s *Container) IterateStatementStats( // IterateTransactionStats implements sqlstats.Provider interface. func (s *Container) IterateTransactionStats( - ctx context.Context, options *sqlstats.IteratorOptions, visitor sqlstats.TransactionVisitor, + ctx context.Context, options sqlstats.IteratorOptions, visitor sqlstats.TransactionVisitor, ) error { iter := s.TxnStatsIterator(options) @@ -686,7 +686,7 @@ func (s *Container) MergeApplicationStatementStats( ) (discardedStats uint64) { if err := other.IterateStatementStats( ctx, - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func(ctx context.Context, statistics *appstatspb.CollectedStatementStatistics) error { if transformer != nil { transformer(statistics) @@ -737,7 +737,7 @@ func (s *Container) MergeApplicationTransactionStats( ) (discardedStats uint64) { if err := other.IterateTransactionStats( ctx, - &sqlstats.IteratorOptions{}, + sqlstats.IteratorOptions{}, func(ctx context.Context, statistics *appstatspb.CollectedTransactionStatistics) error { txnStats, _, throttled := s.getStatsForTxnWithKey( diff --git a/pkg/sql/sqlstats/ssprovider.go b/pkg/sql/sqlstats/ssprovider.go index e11b99fd5047..19ff5c12e00f 100644 --- a/pkg/sql/sqlstats/ssprovider.go +++ b/pkg/sql/sqlstats/ssprovider.go @@ -56,16 +56,16 @@ type Reader interface { // as ordering, through IteratorOptions argument. StatementVisitor can return // error, if an error is returned after the execution of the visitor, the // iteration is aborted. - IterateStatementStats(context.Context, *IteratorOptions, StatementVisitor) error + IterateStatementStats(context.Context, IteratorOptions, StatementVisitor) error // IterateTransactionStats iterates through all the collected transaction // statistics by using TransactionVisitor. It behaves similarly to // IterateStatementStats. - IterateTransactionStats(context.Context, *IteratorOptions, TransactionVisitor) error + IterateTransactionStats(context.Context, IteratorOptions, TransactionVisitor) error // IterateAggregatedTransactionStats iterates through all the collected app-level // transactions statistics. It behaves similarly to IterateStatementStats. - IterateAggregatedTransactionStats(context.Context, *IteratorOptions, AggregatedTransactionVisitor) error + IterateAggregatedTransactionStats(context.Context, IteratorOptions, AggregatedTransactionVisitor) error } // ApplicationStats is an interface to read from or write to the statistics From eadaa0e1e9e06369724aa3bdc66431d83d8a9d28 Mon Sep 17 00:00:00 2001 From: Evan Wall Date: Fri, 30 Jun 2023 15:40:22 -0400 Subject: [PATCH 3/5] test: add query to output of SQLRunner.ExpectErr Add incorrectly succeeding query to output of SQLRunner.ExpectErr to make debugging easier when used in a loop. It now matches the behavior of SQLRunner.Exec. Release note: None --- pkg/testutils/sqlutils/sql_runner.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/testutils/sqlutils/sql_runner.go b/pkg/testutils/sqlutils/sql_runner.go index 41c52bb69b6b..4847309dffdc 100644 --- a/pkg/testutils/sqlutils/sql_runner.go +++ b/pkg/testutils/sqlutils/sql_runner.go @@ -148,17 +148,17 @@ func (sr *SQLRunner) ExecRowsAffected( func (sr *SQLRunner) ExpectErr(t Fataler, errRE string, query string, args ...interface{}) { helperOrNoop(t)() _, err := sr.DB.ExecContext(context.Background(), query, args...) - sr.expectErr(t, err, errRE) + sr.expectErr(t, query, err, errRE) } -func (sr *SQLRunner) expectErr(t Fataler, err error, errRE string) { +func (sr *SQLRunner) expectErr(t Fataler, query string, err error, errRE string) { helperOrNoop(t)() if !testutils.IsError(err, errRE) { s := "nil" if err != nil { s = pgerror.FullError(err) } - t.Fatalf("expected error '%s', got: %s", errRE, s) + t.Fatalf("expected query '%s' error '%s', got: %s", query, errRE, s) } } @@ -170,7 +170,7 @@ func (sr *SQLRunner) ExpectErrWithHint( helperOrNoop(t)() _, err := sr.DB.ExecContext(context.Background(), query, args...) - sr.expectErr(t, err, errRE) + sr.expectErr(t, query, err, errRE) if pqErr := (*pq.Error)(nil); errors.As(err, &pqErr) { matched, merr := regexp.MatchString(hintRE, pqErr.Hint) From b3c2312f933ad09ff8394d7df67017c38c0f3cb3 Mon Sep 17 00:00:00 2001 From: Evan Wall Date: Fri, 30 Jun 2023 16:01:17 -0400 Subject: [PATCH 4/5] sql: exclude anonymous database from pg_catalog and pg_extension schemas in TableName.FQString() Fixes #105929 `"".pg_extension.spatial_ref_sys` is the output of `TableName.FQString()` for `spatial_ref_sys` which results in an error when trying to use the value: ``` demo@127.0.0.1:26257/movr> SELECT count(*) FROM pg_extension.spatial_ref_sys; count --------- 6139 (1 row) Time: 17ms total (execution 17ms / network 0ms) demo@127.0.0.1:26257/movr> SELECT count(*) FROM "".pg_extension.spatial_ref_sys; ERROR: cannot access virtual schema in anonymous database SQLSTATE: 42704 HINT: verify that the current database is set ``` This PR fixes this by removing the `"".` prefix. Release note: None --- pkg/sql/sem/tree/table_name.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/sql/sem/tree/table_name.go b/pkg/sql/sem/tree/table_name.go index bbcc40af19c7..8831542cf799 100644 --- a/pkg/sql/sem/tree/table_name.go +++ b/pkg/sql/sem/tree/table_name.go @@ -10,6 +10,8 @@ package tree +import "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" + // TableName corresponds to the name of a table in a FROM clause, // INSERT or UPDATE statement, etc. // @@ -46,8 +48,13 @@ func (t *TableName) objectName() {} // schema and catalog names. Suitable for logging, etc. func (t *TableName) FQString() string { ctx := NewFmtCtx(FmtSimple) - ctx.FormatNode(&t.CatalogName) - ctx.WriteByte('.') + schemaName := t.SchemaName.String() + // The pg_catalog and pg_extension schemas cannot be referenced from inside + // an anonymous ("") database. This makes their FQ string always relative. + if schemaName != catconstants.PgCatalogName && schemaName != catconstants.PgExtensionSchemaName { + ctx.FormatNode(&t.CatalogName) + ctx.WriteByte('.') + } ctx.FormatNode(&t.SchemaName) ctx.WriteByte('.') ctx.FormatNode(&t.ObjectName) From 68a9c3869d454ba4d8e9640d5af7b34cd7d624e6 Mon Sep 17 00:00:00 2001 From: Evan Wall Date: Fri, 30 Jun 2023 14:11:26 -0400 Subject: [PATCH 5/5] sql: add tests for CTAS, CMVAS with every vtable Informs #105895 Adds tests for CREATE TABLE AS, CREATE MATERIALIZED VIEW AS sourcing from all vtables. Release note: None --- pkg/sql/BUILD.bazel | 1 + pkg/sql/create_as_test.go | 126 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 pkg/sql/create_as_test.go diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel index 473c79658fc8..c79a50cf5dfa 100644 --- a/pkg/sql/BUILD.bazel +++ b/pkg/sql/BUILD.bazel @@ -616,6 +616,7 @@ go_test( "copy_test.go", "copy_to_test.go", "crdb_internal_test.go", + "create_as_test.go", "create_function_test.go", "create_stats_test.go", "create_test.go", diff --git a/pkg/sql/create_as_test.go b/pkg/sql/create_as_test.go new file mode 100644 index 000000000000..ebbd3c3418e9 --- /dev/null +++ b/pkg/sql/create_as_test.go @@ -0,0 +1,126 @@ +// Copyright 2023 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 sql + +import ( + "context" + "fmt" + "strings" + "testing" + + "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/sql/parser" + "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/types" + "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" + "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/stretchr/testify/require" +) + +// TestCreateAsVTable verifies that all vtables can be used as the source of +// CREATE TABLE AS and CREATE MATERIALIZED VIEW AS. +func TestCreateAsVTable(t *testing.T) { + defer leaktest.AfterTest(t)() + + // These are the vtables that need to be fixed. + // The map should be empty if all vtables are supported. + brokenTables := map[string]struct{}{ + // TODO(sql-foundations): Fix nil pointer dereference. + // See https://github.com/cockroachdb/cockroach/issues/106166. + `pg_catalog.pg_prepared_statements`: {}, + // TODO(sql-foundations): Fix nil pointer dereference. + // See https://github.com/cockroachdb/cockroach/issues/106167. + `pg_catalog.pg_cursors`: {}, + // TODO(sql-foundations): Fix nil pointer dereference. + // See https://github.com/cockroachdb/cockroach/issues/106168. + `"".crdb_internal.create_statements`: {}, + } + + ctx := context.Background() + testCluster := serverutils.StartNewTestCluster(t, 1, base.TestClusterArgs{}) + defer testCluster.Stopper().Stop(ctx) + sqlRunner := sqlutils.MakeSQLRunner(testCluster.ServerConn(0)) + var p parser.Parser + + i := 0 + for _, vSchema := range virtualSchemas { + for _, vSchemaDef := range vSchema.tableDefs { + if vSchemaDef.isUnimplemented() { + continue + } + + var name tree.TableName + var ctasColumns []string + schema := vSchemaDef.getSchema() + statements, err := p.Parse(schema) + require.NoErrorf(t, err, schema) + require.Lenf(t, statements, 1, schema) + switch stmt := statements[0].AST.(type) { + case *tree.CreateTable: + name = stmt.Table + for _, def := range stmt.Defs { + if colDef, ok := def.(*tree.ColumnTableDef); ok { + if colDef.Hidden { + continue + } + // Filter out vector columns to prevent error in CTAS: + // "VECTOR column types are unsupported". + if colDef.Type == types.Int2Vector || colDef.Type == types.OidVector { + continue + } + ctasColumns = append(ctasColumns, colDef.Name.String()) + } + } + case *tree.CreateView: + name = stmt.Name + ctasColumns = []string{"*"} + default: + require.Failf(t, "missing case", "unexpected type %T for schema %s", stmt, schema) + } + + fqName := name.FQString() + if _, ok := brokenTables[fqName]; ok { + continue + } + + // Filter by trace_id to prevent error when selecting from + // crdb_internal.cluster_inflight_traces: + // "pq: a trace_id value needs to be specified". + var where string + if fqName == `"".crdb_internal.cluster_inflight_traces` { + where = " WHERE trace_id = 1" + } + + createTableStmt := fmt.Sprintf( + "CREATE TABLE test_table_%d AS SELECT %s FROM %s%s", + i, strings.Join(ctasColumns, ", "), fqName, where, + ) + sqlRunner.Exec(t, createTableStmt) + createViewStmt := fmt.Sprintf( + "CREATE MATERIALIZED VIEW test_view_%d AS SELECT * FROM %s%s", + i, fqName, where, + ) + sqlRunner.Exec(t, createViewStmt) + i++ + } + } + + waitForJobsSuccess(t, sqlRunner) +} + +func waitForJobsSuccess(t *testing.T, sqlRunner *sqlutils.SQLRunner) { + query := `SELECT job_id, status, error, description +FROM [SHOW JOBS] +WHERE job_type IN ('SCHEMA CHANGE', 'NEW SCHEMA CHANGE') +AND status != 'succeeded'` + sqlRunner.CheckQueryResultsRetry(t, query, [][]string{}) +}