From 9cde0a62e2b1a54913c02010e1af11b18116e62a Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Thu, 27 Jul 2023 12:50:14 -0400 Subject: [PATCH 01/14] server: deflake TestServerShutdownReleasesSession The tenant was not being fully stopped, so the test could encounter flakes. Release note: None --- pkg/server/drain_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/server/drain_test.go b/pkg/server/drain_test.go index 35b6650c5244..981e7291e045 100644 --- a/pkg/server/drain_test.go +++ b/pkg/server/drain_test.go @@ -346,6 +346,7 @@ func TestServerShutdownReleasesSession(t *testing.T) { require.True(t, sessionExists(*session)) require.NoError(t, tmpTenant.DrainClients(context.Background())) + tmpTenant.Stopper().Stop(ctx) require.False(t, sessionExists(*session), "expected session %s to be deleted from the sqlliveness table, but it still exists", *session) require.Nil(t, queryOwner(tmpSQLInstance), "expected sql_instance %d to have no owning session_id", tmpSQLInstance) From 18b6a6a4f2328b545fc4f5cbab34f69904264602 Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Thu, 27 Jul 2023 10:33:32 -0400 Subject: [PATCH 02/14] server/profiler: remove `server.cpu_profile.enabled` setting Cpu profiling can be enabled by setting the cluster setting `server.cpu_profile.cpu_usage_combined_threshold`. This makes `server.cpu_profile.enabled` redundant and makes it more difficult and confusing to enable cpu profiling. This commit removes the `server.cpu_profile.enabled` setting entirely. Note that both jdefault values for the cluster settings set profiling off. Closes: #102024 Release note (sql change): The cluster setting `server.cpu_profile.enabled` has been removed. `server.cpu_profile.cpu_usage_combined_threshold` can enable and disable cpu profiling. --- pkg/server/profiler/cpuprofile_test.go | 1 - pkg/server/profiler/cpuprofiler.go | 12 ------------ pkg/settings/registry.go | 1 + 3 files changed, 1 insertion(+), 13 deletions(-) diff --git a/pkg/server/profiler/cpuprofile_test.go b/pkg/server/profiler/cpuprofile_test.go index ab87ce0e0694..f059057c2021 100644 --- a/pkg/server/profiler/cpuprofile_test.go +++ b/pkg/server/profiler/cpuprofile_test.go @@ -31,7 +31,6 @@ func TestCPUProfiler(t *testing.T) { sv.Init(ctx, s.Version) cpuProfileInterval.Override(ctx, sv, time.Hour) cpuUsageCombined.Override(ctx, sv, 80) - cpuProfileEnabled.Override(ctx, sv, true) pastTime := time.Date(2023, 1, 1, 1, 1, 1, 1, time.UTC) cases := []struct { name string diff --git a/pkg/server/profiler/cpuprofiler.go b/pkg/server/profiler/cpuprofiler.go index a10edbdd1214..9d49bc4a723d 100644 --- a/pkg/server/profiler/cpuprofiler.go +++ b/pkg/server/profiler/cpuprofiler.go @@ -61,15 +61,6 @@ var cpuProfileDuration = settings.RegisterDurationSetting( 10*time.Second, settings.PositiveDuration, ) -var cpuProfileEnabled = settings.RegisterBoolSetting( - settings.TenantWritable, - "server.cpu_profile.enabled", - "a bool which indicates whether cpu profiles should be taken by the cpu profiler. "+ - "in order to have the profiler function, server.cpu_profile.cpu_usage_combined_threshold "+ - "must also be set to a realistic value", - false, -) - const cpuProfFileNamePrefix = "cpuprof" // CPUProfiler is used to take CPU profiles. @@ -116,9 +107,6 @@ func (cp *CPUProfiler) MaybeTakeProfile(ctx context.Context, currentCpuUsage int logcrash.ReportPanic(ctx, &cp.st.SV, p, 1) } }() - if !cpuProfileEnabled.Get(&cp.st.SV) { - return - } cp.profiler.maybeTakeProfile(ctx, currentCpuUsage, cp.takeCPUProfile) } diff --git a/pkg/settings/registry.go b/pkg/settings/registry.go index ac47c5ba0ad7..35cad8aa946c 100644 --- a/pkg/settings/registry.go +++ b/pkg/settings/registry.go @@ -173,6 +173,7 @@ var retiredSettings = map[string]struct{}{ "changefeed.replan_flow_threshold": {}, "jobs.trace.force_dump_mode": {}, "timeseries.storage.30m_resolution_ttl": {}, + "server.cpu_profile.enabled": {}, } // sqlDefaultSettings is the list of "grandfathered" existing sql.defaults From 44480bb69632d5b0ea9f0ea3052ca68be8c9c109 Mon Sep 17 00:00:00 2001 From: Austen McClernon Date: Thu, 27 Jul 2023 19:34:37 +0000 Subject: [PATCH 03/14] kvserver: fix test merge queue when grunning unsupported `TestMergeQueue/load-based-merging/switch...below-threshold` asserts that switching the split objective between CPU and QPS will not cause range's to merge, even if their pre-switch load qualified them for merging. This test was broken when `grunning` was unsupported, as the objective never actually switches to anything other than QPS. Add a check for `grunning` support, and assert that a merge occurs if unsupported. Fixes: #106937 Epic: none Release note: None --- pkg/kv/kvserver/client_merge_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/pkg/kv/kvserver/client_merge_test.go b/pkg/kv/kvserver/client_merge_test.go index df7152e7d2e0..5d90291e769b 100644 --- a/pkg/kv/kvserver/client_merge_test.go +++ b/pkg/kv/kvserver/client_merge_test.go @@ -55,6 +55,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" "github.com/cockroachdb/cockroach/pkg/util/ctxgroup" + "github.com/cockroachdb/cockroach/pkg/util/grunning" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -4525,7 +4526,14 @@ func TestMergeQueue(t *testing.T) { clearRange(t, lhsStartKey, rhsEndKey) setSplitObjective(secondSplitObjective) - verifyUnmergedSoon(t, store, lhsStartKey, rhsStartKey) + if !grunning.Supported() { + // CPU isn't a supported split objective when grunning isn't + // supported. Switching the dimension will have no effect, as the + // objective gets overridden in such cases to always be QPS. + verifyMergedSoon(t, store, lhsStartKey, rhsStartKey) + } else { + verifyUnmergedSoon(t, store, lhsStartKey, rhsStartKey) + } }) } } From 31e9b590051221d16c3088884acb30938c50c2d6 Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Thu, 27 Jul 2023 14:20:59 -0400 Subject: [PATCH 04/14] ui: show txn fingerprint details page with unspecified app Previously, when the app was not specified in the url search params for the txn details fingerprint page, the page would fail to load. This commit allows the page to load when there is no app specified but a fingerprint id that matches the requested page in the payload. The first matching fingerprint id is loaded. Additionally, the TransactionDetailsLink will not include the appNames search param unless the provided prop is non-nullish. Fixes: #107731 Release note (bug fix): Txn fingerprint details page in the console UI should load with the fingerprint details even if no app is specified in the URL. --- .../transactionDetailsUtils.spec.tsx | 83 ++++++++++++++----- .../transactionDetailsUtils.tsx | 4 +- .../transactionsTable/transactionsTable.tsx | 9 +- 3 files changed, 69 insertions(+), 27 deletions(-) diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetailsUtils.spec.tsx b/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetailsUtils.spec.tsx index 0e4551d22a9f..2c2d7ac11543 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetailsUtils.spec.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetailsUtils.spec.tsx @@ -18,28 +18,68 @@ import { shuffle } from "lodash"; import Long from "long"; describe("getTxnFromSqlStatsTxns", () => { - //TODO - it("should return the first txn with the fingerprint ID and app name specified", () => { - const app = "cockroach"; - const txns = [ - { id: 1, app: "hello_world" }, - { id: 2, app }, - { id: 3, app: "cockrooch" }, - { id: 3, app }, - { id: 4, app: "my_app" }, - ].map(txn => - mockTxnStats({ - stats_data: { - transaction_fingerprint_id: Long.fromInt(txn.id), - app: txn.app, - }, - }), - ); + it.each([ + [ + [ + { id: 1, app: "hello_world" }, + { id: 2, app: "cockroach" }, + { id: 3, app: "" }, + { id: 3, app: "cockroach" }, + { id: 3, app: "cockroach" }, + { id: 3, app: "my_app" }, + { id: 4, app: "my_app" }, + ], + "3", // fingerprint id + ["cockroach", "my_app"], // app name + 3, // Expected idx. + ], + [ + [ + { id: 1, app: "hello_world" }, + { id: 2, app: "cockroach_app" }, + { id: 3, app: "" }, + { id: 3, app: "cockroach" }, + { id: 3, app: "my_app" }, + { id: 4, app: "my_app" }, + ], + "3", // fingerprint id + ["cockroach", "my_app"], // app name + 3, // Expected idx. + ], + [ + [ + { id: 1, app: "hello_world" }, + { id: 2, app: "cockroach" }, + { id: 2, app: "cockrooch" }, + { id: 3, app: "cockroach" }, + { id: 4, app: "my_app" }, + ], + "2", // fingerprint id + null, // app names + 1, // Expected idx. + ], + ])( + "should return the first txn with the fingerprint ID and app name specified", + ( + txnsToMock, + fingerprintID: string, + apps: string[] | null, + expectedIdx: number, + ) => { + const txns = txnsToMock.map((txn: { id: number; app: string }) => + mockTxnStats({ + stats_data: { + transaction_fingerprint_id: Long.fromInt(txn.id), + app: txn.app, + }, + }), + ); - const expectedTxn = txns[3]; - const txn = getTxnFromSqlStatsTxns(txns, "3", [app]); - expect(txn).toEqual(expectedTxn); - }); + const expectedTxn = txns[expectedIdx]; + const txn = getTxnFromSqlStatsTxns(txns, fingerprintID, apps); + expect(txn).toEqual(expectedTxn); + }, + ); it("should return null if no txn can be found", () => { const txns = [1, 2, 3, 4, 5, 6].map(txn => @@ -60,7 +100,6 @@ describe("getTxnFromSqlStatsTxns", () => { [null, "123", null], [null, null, ["app"]], [[mockTxnStats()], null, null], - [[mockTxnStats()], "123", null], [[mockTxnStats()], "123", []], [[mockTxnStats()], null, ["app"]], [[mockTxnStats()], "", ["app"]], diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetailsUtils.tsx b/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetailsUtils.tsx index 623d2f46644f..000f32944c06 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetailsUtils.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetailsUtils.tsx @@ -41,7 +41,7 @@ export const getTxnFromSqlStatsTxns = ( txnFingerprintID: string | null, apps: string[] | null, ): Transaction | null => { - if (!txns?.length || !apps?.length || !txnFingerprintID) { + if (!txns?.length || !txnFingerprintID) { return null; } @@ -49,7 +49,7 @@ export const getTxnFromSqlStatsTxns = ( txn => txn.stats_data.transaction_fingerprint_id.toString() === txnFingerprintID && - (apps.length ? apps.includes(txn.stats_data.app ?? unset) : true), + (apps?.length ? apps.includes(txn.stats_data.app ?? unset) : true), ); }; diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsTable/transactionsTable.tsx b/pkg/ui/workspaces/cluster-ui/src/transactionsTable/transactionsTable.tsx index 37eeffe715cb..a9d1d1dc34c8 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionsTable/transactionsTable.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/transactionsTable/transactionsTable.tsx @@ -82,9 +82,12 @@ interface TransactionLinkTargetProps { export const TransactionLinkTarget = ( props: TransactionLinkTargetProps, ): string => { - const searchParams = propsToQueryString({ - [appNamesAttr]: [props.application], - }); + let searchParams = ""; + if (props.application != null) { + searchParams = propsToQueryString({ + [appNamesAttr]: [props.application], + }); + } return `/transaction/${props.transactionFingerprintId}?${searchParams}`; }; From 4f1e340e1d03be8337bac199155cc7af9ad1c0f0 Mon Sep 17 00:00:00 2001 From: Jayant Shrivastava Date: Thu, 27 Jul 2023 16:20:38 -0400 Subject: [PATCH 05/14] changefeedccl: prevent deadlock in TestChangefeedKafkaMessageTooLarge Previously, this test would deadlock due to kafka retrying messages too many times. These messages are stored in a buffer of size 1024 created by the CDC testing infra: https://github.com/cockroachdb/cockroach/blob/5c3f96d38cdc3a2d953ca3ffb1e39e97d7e5110e/pkg/ccl/changefeedccl/testfeed_test.go#L1819 The test asserts that 2000 messages pass through the buffer. When the test finishes, it stops reading from the buffer. The problem is that due to retries, there may be more messages sent to the buffer than that are read out of the buffer. Even after the 2000 messages are read and the test is shutting down, the sink may be blocked trying to put resolved messages (plus retries) in the buffer. If this happens, the changefeed resumer (same goroutine as the kafka sink) gets blocked and does not terminate when the job is cancelled at the end of the test. This change caps the number of retries at 200 for this test, so there should be no more than 200 extra messages plus a few resolved messages during this test. This is far less than the buffer size of 1024. See detailed explanation in #107591. Fixes: https://github.com/cockroachdb/cockroach/issues/107591 Epic: none Release note: None --- pkg/ccl/changefeedccl/changefeed_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/ccl/changefeedccl/changefeed_test.go b/pkg/ccl/changefeedccl/changefeed_test.go index b4ccb2d9c6bf..34863e8558da 100644 --- a/pkg/ccl/changefeedccl/changefeed_test.go +++ b/pkg/ccl/changefeedccl/changefeed_test.go @@ -7702,8 +7702,10 @@ func TestChangefeedKafkaMessageTooLarge(t *testing.T) { rnd, _ := randutil.NewPseudoRand() + maxFailures := int32(200) + var numFailures atomic.Int32 knobs.kafkaInterceptor = func(m *sarama.ProducerMessage, client kafkaClient) error { - if client.Config().Producer.Flush.MaxMessages > 1 && rnd.Int()%5 == 0 { + if client.Config().Producer.Flush.MaxMessages > 1 && numFailures.Add(1) < maxFailures && rnd.Int()%10 == 0 { return sarama.ErrMessageSizeTooLarge } return nil From dd65d45b88ccfe1f54ea4c822f5fe6ea75fb399a Mon Sep 17 00:00:00 2001 From: Michael Erickson Date: Thu, 27 Jul 2023 12:46:27 -0700 Subject: [PATCH 06/14] opt: add enable_durable_locking_for_serializable session variable Follow-up from #105857 This commit ammends 6a3e43d76a024c1de0bb33284b9c3f1d6b6236ed to add a session variable to control whether guaranteed-durable locks are used under serializable isolation. Informs: #100194 Epic: CRDB-25322 Release note (sql change): Add a new session variable, `enable_durable_locking_for_serializable`, which controls locking durability under serializable isolation. With this set to true, SELECT FOR UPDATE locks, SELECT FOR SHARED locks, and constraint check locks (e.g. locks acquired during foreign key checks if `enable_implicit_fk_locking_for_serializable` is set to true) will be guaranteed-durable under serializable isolation, meaning they will always be held to transaction commit. (These locks are always guaranteed-durable under weaker isolation levels.) By default, under serializable isolation these locks are best-effort rather than guaranteed-durable, meaning in some cases (e.g. leaseholder transfer, node loss, etc.) they could be released before transaction commit. Serializable isolation does not rely on locking for correctness, only using it to improve performance under contention, so this default is a deliberate choice to avoid the performance overhead of lock replication. --- pkg/sql/exec_util.go | 4 + .../testdata/logic_test/information_schema | 1 + .../logictest/testdata/logic_test/pg_catalog | 3 + .../logictest/testdata/logic_test/show_source | 1 + pkg/sql/opt/exec/execbuilder/testdata/fk | 47 +++++++++++ .../execbuilder/testdata/select_for_update | 81 +++++++++++++++++++ pkg/sql/opt/memo/memo.go | 3 + pkg/sql/opt/memo/memo_test.go | 6 ++ pkg/sql/opt/optbuilder/select.go | 16 ++-- .../local_only_session_data.proto | 6 ++ pkg/sql/vars.go | 17 ++++ 11 files changed, 180 insertions(+), 5 deletions(-) diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 7be6ebc245c8..7bac5de6c986 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -3616,6 +3616,10 @@ func (m *sessionDataMutator) SetImplicitFKLockingForSerializable(val bool) { m.data.ImplicitFKLockingForSerializable = val } +func (m *sessionDataMutator) SetDurableLockingForSerializable(val bool) { + m.data.DurableLockingForSerializable = val +} + // Utility functions related to scrubbing sensitive information on SQL Stats. // quantizeCounts ensures that the Count field in the diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema index 817f8b1277d4..69a4185aafb2 100644 --- a/pkg/sql/logictest/testdata/logic_test/information_schema +++ b/pkg/sql/logictest/testdata/logic_test/information_schema @@ -5288,6 +5288,7 @@ disallow_full_table_scans off enable_auto_rehoming off enable_create_stats_using_extremes off enable_drop_enum_value on +enable_durable_locking_for_serializable off enable_experimental_alter_column_type_general off enable_implicit_fk_locking_for_serializable off enable_implicit_select_for_update on diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index cd02b4355027..081c87aaf28c 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -2726,6 +2726,7 @@ disallow_full_table_scans off N distsql off NULL NULL NULL string enable_auto_rehoming off NULL NULL NULL string enable_create_stats_using_extremes off NULL NULL NULL string +enable_durable_locking_for_serializable off NULL NULL NULL string enable_experimental_alter_column_type_general off NULL NULL NULL string enable_implicit_fk_locking_for_serializable off NULL NULL NULL string enable_implicit_select_for_update on NULL NULL NULL string @@ -2887,6 +2888,7 @@ disallow_full_table_scans off N distsql off NULL user NULL off off enable_auto_rehoming off NULL user NULL off off enable_create_stats_using_extremes off NULL user NULL off off +enable_durable_locking_for_serializable off NULL user NULL off off enable_experimental_alter_column_type_general off NULL user NULL off off enable_implicit_fk_locking_for_serializable off NULL user NULL off off enable_implicit_select_for_update on NULL user NULL on on @@ -3045,6 +3047,7 @@ distsql NULL NULL NULL distsql_workmem NULL NULL NULL NULL NULL enable_auto_rehoming NULL NULL NULL NULL NULL enable_create_stats_using_extremes NULL NULL NULL NULL NULL +enable_durable_locking_for_serializable NULL NULL NULL NULL NULL enable_experimental_alter_column_type_general NULL NULL NULL NULL NULL enable_implicit_fk_locking_for_serializable NULL NULL NULL NULL NULL enable_implicit_select_for_update NULL NULL NULL NULL NULL diff --git a/pkg/sql/logictest/testdata/logic_test/show_source b/pkg/sql/logictest/testdata/logic_test/show_source index c71e86ef5be3..df3857d1a5c1 100644 --- a/pkg/sql/logictest/testdata/logic_test/show_source +++ b/pkg/sql/logictest/testdata/logic_test/show_source @@ -61,6 +61,7 @@ disallow_full_table_scans off distsql off enable_auto_rehoming off enable_create_stats_using_extremes off +enable_durable_locking_for_serializable off enable_experimental_alter_column_type_general off enable_implicit_fk_locking_for_serializable off enable_implicit_select_for_update on diff --git a/pkg/sql/opt/exec/execbuilder/testdata/fk b/pkg/sql/opt/exec/execbuilder/testdata/fk index 5ea04f8abef4..50811f307971 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/fk +++ b/pkg/sql/opt/exec/execbuilder/testdata/fk @@ -77,6 +77,29 @@ vectorized: true estimated row count: 2 label: buffer 1 +# Try again with durable locking enabled. +statement ok +SET enable_durable_locking_for_serializable = true + +# TODO(michae2, 100194): Change this from EXPLAIN (OPT) to EXPLAIN. +query T +EXPLAIN (OPT) INSERT INTO child VALUES (1,1), (2,2) +---- +insert child + ├── values + │ ├── (1, 1) + │ └── (2, 2) + └── f-k-checks + └── f-k-checks-item: child(p) -> parent(p) + └── anti-join (lookup parent) + ├── lookup columns are key + ├── locking: for-share,durability-guaranteed + ├── with-scan &1 + └── filters (true) + +statement ok +RESET enable_durable_locking_for_serializable + statement ok RESET enable_implicit_fk_locking_for_serializable @@ -644,6 +667,30 @@ vectorized: true spans: FULL SCAN locking strength: for share +# Try again with durable locking enabled. +statement ok +SET enable_durable_locking_for_serializable = true + +# TODO(michae2, 100194): Change this from EXPLAIN (OPT) to EXPLAIN. +query T +EXPLAIN (OPT) UPDATE child SET p = 4 +---- +update child + ├── project + │ ├── scan child + │ └── projections + │ └── 4 + └── f-k-checks + └── f-k-checks-item: child(p) -> parent(p) + └── anti-join (merge) + ├── with-scan &1 + ├── scan parent + │ └── locking: for-share,durability-guaranteed + └── filters (true) + +statement ok +RESET enable_durable_locking_for_serializable + statement ok RESET enable_implicit_fk_locking_for_serializable diff --git a/pkg/sql/opt/exec/execbuilder/testdata/select_for_update b/pkg/sql/opt/exec/execbuilder/testdata/select_for_update index e36bae6c8e19..de5c532f7801 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/select_for_update +++ b/pkg/sql/opt/exec/execbuilder/testdata/select_for_update @@ -2560,3 +2560,84 @@ vectorized: true spans: /2-/3 locking strength: for update locking wait policy: skip locked + +# ------------------------------------------------------------------------------ +# Tests with durable locking. +# ------------------------------------------------------------------------------ + +statement ok +SET enable_durable_locking_for_serializable = true + +# TODO(michae2, 100194): Change these from EXPLAIN (OPT) to EXPLAIN (VERBOSE). + +query T +EXPLAIN (OPT) SELECT * FROM t FOR UPDATE +---- +scan t + └── locking: for-update,durability-guaranteed + +query T +EXPLAIN (OPT) SELECT * FROM t FOR NO KEY UPDATE +---- +scan t + └── locking: for-no-key-update,durability-guaranteed + +query T +EXPLAIN (OPT) SELECT * FROM t FOR SHARE +---- +scan t + └── locking: for-share,durability-guaranteed + +query T +EXPLAIN (OPT) SELECT * FROM t FOR KEY SHARE +---- +scan t + └── locking: for-key-share,durability-guaranteed + +query T +EXPLAIN (OPT) SELECT * FROM t FOR KEY SHARE FOR SHARE +---- +scan t + └── locking: for-share,durability-guaranteed + +query T +EXPLAIN (OPT) SELECT * FROM t FOR KEY SHARE FOR SHARE FOR NO KEY UPDATE +---- +scan t + └── locking: for-no-key-update,durability-guaranteed + +query T +EXPLAIN (OPT) SELECT * FROM t FOR KEY SHARE FOR SHARE FOR NO KEY UPDATE FOR UPDATE +---- +scan t + └── locking: for-update,durability-guaranteed + +query T +EXPLAIN (OPT) SELECT * FROM t FOR UPDATE OF t +---- +scan t + └── locking: for-update,durability-guaranteed + +query T +EXPLAIN (OPT) SELECT (SELECT a FROM t FOR UPDATE OF t) +---- +values + └── tuple + └── subquery + └── max1-row + └── scan t + └── locking: for-update,durability-guaranteed + +query T +EXPLAIN (OPT) SELECT * FROM t WHERE a IN (SELECT b FROM t FOR UPDATE) +---- +project + └── inner-join (lookup t) + ├── lookup columns are key + ├── distinct-on + │ └── scan t + │ └── locking: for-update,durability-guaranteed + └── filters (true) + +statement ok +RESET enable_durable_locking_for_serializable diff --git a/pkg/sql/opt/memo/memo.go b/pkg/sql/opt/memo/memo.go index 9aceb441f354..c15054b5dfd4 100644 --- a/pkg/sql/opt/memo/memo.go +++ b/pkg/sql/opt/memo/memo.go @@ -170,6 +170,7 @@ type Memo struct { useImprovedComputedColumnFiltersDerivation bool useImprovedJoinElimination bool implicitFKLockingForSerializable bool + durableLockingForSerializable bool // txnIsoLevel is the isolation level under which the plan was created. This // affects the planning of some locking operations, so it must be included in @@ -238,6 +239,7 @@ func (m *Memo) Init(ctx context.Context, evalCtx *eval.Context) { useImprovedComputedColumnFiltersDerivation: evalCtx.SessionData().OptimizerUseImprovedComputedColumnFiltersDerivation, useImprovedJoinElimination: evalCtx.SessionData().OptimizerUseImprovedJoinElimination, implicitFKLockingForSerializable: evalCtx.SessionData().ImplicitFKLockingForSerializable, + durableLockingForSerializable: evalCtx.SessionData().DurableLockingForSerializable, txnIsoLevel: evalCtx.TxnIsoLevel, } m.metadata.Init() @@ -386,6 +388,7 @@ func (m *Memo) IsStale( m.useImprovedComputedColumnFiltersDerivation != evalCtx.SessionData().OptimizerUseImprovedComputedColumnFiltersDerivation || m.useImprovedJoinElimination != evalCtx.SessionData().OptimizerUseImprovedJoinElimination || m.implicitFKLockingForSerializable != evalCtx.SessionData().ImplicitFKLockingForSerializable || + m.durableLockingForSerializable != evalCtx.SessionData().DurableLockingForSerializable || m.txnIsoLevel != evalCtx.TxnIsoLevel { return true, nil } diff --git a/pkg/sql/opt/memo/memo_test.go b/pkg/sql/opt/memo/memo_test.go index aac46822b054..45300216d53a 100644 --- a/pkg/sql/opt/memo/memo_test.go +++ b/pkg/sql/opt/memo/memo_test.go @@ -386,6 +386,12 @@ func TestMemoIsStale(t *testing.T) { evalCtx.SessionData().ImplicitFKLockingForSerializable = false notStale() + // Stale enable_durable_locking_for_serializable. + evalCtx.SessionData().DurableLockingForSerializable = true + stale() + evalCtx.SessionData().DurableLockingForSerializable = false + notStale() + // Stale txn isolation level. evalCtx.TxnIsoLevel = isolation.ReadCommitted stale() diff --git a/pkg/sql/opt/optbuilder/select.go b/pkg/sql/opt/optbuilder/select.go index 870eaa0ad6f1..70867917946e 100644 --- a/pkg/sql/opt/optbuilder/select.go +++ b/pkg/sql/opt/optbuilder/select.go @@ -697,16 +697,22 @@ func (b *Builder) buildScan( } if locking.isSet() { private.Locking = locking.get() - if b.evalCtx.TxnIsoLevel != isolation.Serializable { + if b.evalCtx.TxnIsoLevel != isolation.Serializable || + b.evalCtx.SessionData().DurableLockingForSerializable { // Under weaker isolation levels we use fully-durable locks for SELECT FOR - // UPDATE statements, SELECT FOR SHARE statements, and all other locked - // scans (e.g. FK checks), regardless of locking strength and wait - // policy. Unlike mutation statements, SELECT FOR UPDATE statements do not - // lay down intents, so we cannot rely on the durability of intents to + // UPDATE statements, SELECT FOR SHARE statements, and constraint checks + // (e.g. FK checks), regardless of locking strength and wait policy. + // Unlike mutation statements, SELECT FOR UPDATE statements do not lay + // down intents, so we cannot rely on the durability of intents to // guarantee exclusion until commit as we do for mutation statements. And // unlike serializable isolation, weaker isolation levels do not perform // read refreshing, so we cannot rely on read refreshing to guarantee // exclusion. + // + // Under serializable isolation we only use fully-durable locks if + // enable_durable_locking_for_serializable is set. (Serializable isolation + // does not require locking for correctness, so by default we use + // best-effort locks for better performance.) private.Locking.Durability = tree.LockDurabilityGuaranteed } if private.Locking.WaitPolicy == tree.LockWaitSkipLocked && tab.FamilyCount() > 1 { diff --git a/pkg/sql/sessiondatapb/local_only_session_data.proto b/pkg/sql/sessiondatapb/local_only_session_data.proto index 19d62af2189c..0c7dbb7ff603 100644 --- a/pkg/sql/sessiondatapb/local_only_session_data.proto +++ b/pkg/sql/sessiondatapb/local_only_session_data.proto @@ -417,6 +417,12 @@ message LocalOnlySessionData { bool implicit_fk_locking_for_serializable = 108 [ (gogoproto.customname) = "ImplicitFKLockingForSerializable" ]; + // DurableLockingForSerializable is true if we should use durable locking for + // SELECT FOR UPDATE statements, SELECT FOR SHARE statements, and constraint + // checks under serializable isolation. (Serializable isolation does not + // require locking for correctness, so by default we use best-effor locks for + // better performance.) Weaker isolation levels always use durable locking. + bool durable_locking_for_serializable = 109; /////////////////////////////////////////////////////////////////////////// // WARNING: consider whether a session parameter you're adding needs to // diff --git a/pkg/sql/vars.go b/pkg/sql/vars.go index f51a9a84e19a..1ab24352fc25 100644 --- a/pkg/sql/vars.go +++ b/pkg/sql/vars.go @@ -2834,6 +2834,23 @@ var varGen = map[string]sessionVar{ }, GlobalDefault: globalFalse, }, + + // CockroachDB extension. + `enable_durable_locking_for_serializable`: { + GetStringVal: makePostgresBoolGetStringValFn(`enable_durable_locking_for_serializable`), + Set: func(_ context.Context, m sessionDataMutator, s string) error { + b, err := paramparse.ParseBoolVar("enable_durable_locking_for_serializable", s) + if err != nil { + return err + } + m.SetDurableLockingForSerializable(b) + return nil + }, + Get: func(evalCtx *extendedEvalContext, _ *kv.Txn) (string, error) { + return formatBoolAsPostgresSetting(evalCtx.SessionData().DurableLockingForSerializable), nil + }, + GlobalDefault: globalFalse, + }, } func ReplicationModeFromString(s string) (sessiondatapb.ReplicationMode, error) { From 8cdb073652ddca58decdd30a6ff340db3d92ed48 Mon Sep 17 00:00:00 2001 From: j82w Date: Thu, 27 Jul 2023 10:54:38 -0400 Subject: [PATCH 07/14] cli: add probe_range in debug.zip PR #79546 introduces `crdb_internal.probe_range`. This PR adds the `crdb_internal.probe_range` to the debug.zip. The LIMIT gives a very approximately ~1000ms*100 target on how long this can take, so that running debug.zip against an unavailable cluster won't take too long. closes: #80360 Release note (cli change): The debug.zip now includes the `crdb_internal.probe_range` table with a limit of 100 rows to avoid the query from taking to long. --- pkg/cli/testdata/zip/partial1 | 1 + pkg/cli/testdata/zip/partial1_excluded | 1 + pkg/cli/testdata/zip/partial2 | 1 + pkg/cli/testdata/zip/testzip | 1 + pkg/cli/testdata/zip/testzip_concurrent | 3 +++ pkg/cli/testdata/zip/testzip_external_process_virtualization | 1 + pkg/cli/testdata/zip/testzip_include_range_info | 1 + pkg/cli/testdata/zip/testzip_shared_process_virtualization | 2 ++ pkg/cli/zip_table_registry.go | 4 ++++ pkg/cli/zip_test.go | 1 + 10 files changed, 16 insertions(+) diff --git a/pkg/cli/testdata/zip/partial1 b/pkg/cli/testdata/zip/partial1 index ad300118f4e6..5f44c04c36b9 100644 --- a/pkg/cli/testdata/zip/partial1 +++ b/pkg/cli/testdata/zip/partial1 @@ -32,6 +32,7 @@ debug zip --concurrency=1 --cpu-profile-duration=0s /dev/null [cluster] retrieving SQL data for crdb_internal.kv_store_status... writing output: debug/crdb_internal.kv_store_status.txt... done [cluster] retrieving SQL data for crdb_internal.kv_system_privileges... writing output: debug/crdb_internal.kv_system_privileges.txt... done [cluster] retrieving SQL data for crdb_internal.partitions... writing output: debug/crdb_internal.partitions.txt... done +[cluster] retrieving SQL data for crdb_internal.probe_ranges_1s_write_limit_100... writing output: debug/crdb_internal.probe_ranges_1s_write_limit_100.txt... done [cluster] retrieving SQL data for crdb_internal.regions... writing output: debug/crdb_internal.regions.txt... done [cluster] retrieving SQL data for crdb_internal.schema_changes... writing output: debug/crdb_internal.schema_changes.txt... done [cluster] retrieving SQL data for crdb_internal.super_regions... writing output: debug/crdb_internal.super_regions.txt... done diff --git a/pkg/cli/testdata/zip/partial1_excluded b/pkg/cli/testdata/zip/partial1_excluded index 4912aa33ea52..a7b8567cef16 100644 --- a/pkg/cli/testdata/zip/partial1_excluded +++ b/pkg/cli/testdata/zip/partial1_excluded @@ -32,6 +32,7 @@ debug zip /dev/null --concurrency=1 --exclude-nodes=2 --cpu-profile-duration=0 [cluster] retrieving SQL data for crdb_internal.kv_store_status... writing output: debug/crdb_internal.kv_store_status.txt... done [cluster] retrieving SQL data for crdb_internal.kv_system_privileges... writing output: debug/crdb_internal.kv_system_privileges.txt... done [cluster] retrieving SQL data for crdb_internal.partitions... writing output: debug/crdb_internal.partitions.txt... done +[cluster] retrieving SQL data for crdb_internal.probe_ranges_1s_write_limit_100... writing output: debug/crdb_internal.probe_ranges_1s_write_limit_100.txt... done [cluster] retrieving SQL data for crdb_internal.regions... writing output: debug/crdb_internal.regions.txt... done [cluster] retrieving SQL data for crdb_internal.schema_changes... writing output: debug/crdb_internal.schema_changes.txt... done [cluster] retrieving SQL data for crdb_internal.super_regions... writing output: debug/crdb_internal.super_regions.txt... done diff --git a/pkg/cli/testdata/zip/partial2 b/pkg/cli/testdata/zip/partial2 index b6de0d1ca379..719fceb75cba 100644 --- a/pkg/cli/testdata/zip/partial2 +++ b/pkg/cli/testdata/zip/partial2 @@ -32,6 +32,7 @@ debug zip --concurrency=1 --cpu-profile-duration=0 /dev/null [cluster] retrieving SQL data for crdb_internal.kv_store_status... writing output: debug/crdb_internal.kv_store_status.txt... done [cluster] retrieving SQL data for crdb_internal.kv_system_privileges... writing output: debug/crdb_internal.kv_system_privileges.txt... done [cluster] retrieving SQL data for crdb_internal.partitions... writing output: debug/crdb_internal.partitions.txt... done +[cluster] retrieving SQL data for crdb_internal.probe_ranges_1s_write_limit_100... writing output: debug/crdb_internal.probe_ranges_1s_write_limit_100.txt... done [cluster] retrieving SQL data for crdb_internal.regions... writing output: debug/crdb_internal.regions.txt... done [cluster] retrieving SQL data for crdb_internal.schema_changes... writing output: debug/crdb_internal.schema_changes.txt... done [cluster] retrieving SQL data for crdb_internal.super_regions... writing output: debug/crdb_internal.super_regions.txt... done diff --git a/pkg/cli/testdata/zip/testzip b/pkg/cli/testdata/zip/testzip index 45bffc0addb1..f23adb06daf9 100644 --- a/pkg/cli/testdata/zip/testzip +++ b/pkg/cli/testdata/zip/testzip @@ -32,6 +32,7 @@ debug zip --concurrency=1 --cpu-profile-duration=1s /dev/null [cluster] retrieving SQL data for crdb_internal.kv_store_status... writing output: debug/crdb_internal.kv_store_status.txt... done [cluster] retrieving SQL data for crdb_internal.kv_system_privileges... writing output: debug/crdb_internal.kv_system_privileges.txt... done [cluster] retrieving SQL data for crdb_internal.partitions... writing output: debug/crdb_internal.partitions.txt... done +[cluster] retrieving SQL data for crdb_internal.probe_ranges_1s_write_limit_100... writing output: debug/crdb_internal.probe_ranges_1s_write_limit_100.txt... done [cluster] retrieving SQL data for crdb_internal.regions... writing output: debug/crdb_internal.regions.txt... done [cluster] retrieving SQL data for crdb_internal.schema_changes... writing output: debug/crdb_internal.schema_changes.txt... done [cluster] retrieving SQL data for crdb_internal.super_regions... writing output: debug/crdb_internal.super_regions.txt... done diff --git a/pkg/cli/testdata/zip/testzip_concurrent b/pkg/cli/testdata/zip/testzip_concurrent index 0a94ef203d3f..66a198d83694 100644 --- a/pkg/cli/testdata/zip/testzip_concurrent +++ b/pkg/cli/testdata/zip/testzip_concurrent @@ -110,6 +110,9 @@ zip [cluster] retrieving SQL data for crdb_internal.partitions... [cluster] retrieving SQL data for crdb_internal.partitions: done [cluster] retrieving SQL data for crdb_internal.partitions: writing output: debug/crdb_internal.partitions.txt... +[cluster] retrieving SQL data for crdb_internal.probe_ranges_1s_write_limit_100... +[cluster] retrieving SQL data for crdb_internal.probe_ranges_1s_write_limit_100: done +[cluster] retrieving SQL data for crdb_internal.probe_ranges_1s_write_limit_100: writing output: debug/crdb_internal.probe_ranges_1s_write_limit_100.txt... [cluster] retrieving SQL data for crdb_internal.regions... [cluster] retrieving SQL data for crdb_internal.regions: done [cluster] retrieving SQL data for crdb_internal.regions: writing output: debug/crdb_internal.regions.txt... diff --git a/pkg/cli/testdata/zip/testzip_external_process_virtualization b/pkg/cli/testdata/zip/testzip_external_process_virtualization index b346bb38db46..ed0eb99ecf8f 100644 --- a/pkg/cli/testdata/zip/testzip_external_process_virtualization +++ b/pkg/cli/testdata/zip/testzip_external_process_virtualization @@ -40,6 +40,7 @@ debug zip --concurrency=1 --cpu-profile-duration=1s /dev/null [cluster] retrieving SQL data for crdb_internal.kv_store_status: creating error output: debug/crdb_internal.kv_store_status.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.kv_system_privileges... writing output: debug/crdb_internal.kv_system_privileges.txt... done [cluster] retrieving SQL data for crdb_internal.partitions... writing output: debug/crdb_internal.partitions.txt... done +[cluster] retrieving SQL data for crdb_internal.probe_ranges_1s_write_limit_100... writing output: debug/crdb_internal.probe_ranges_1s_write_limit_100.txt... done [cluster] retrieving SQL data for crdb_internal.regions... writing output: debug/crdb_internal.regions.txt... done [cluster] retrieving SQL data for crdb_internal.schema_changes... writing output: debug/crdb_internal.schema_changes.txt... done [cluster] retrieving SQL data for crdb_internal.super_regions... writing output: debug/crdb_internal.super_regions.txt... done diff --git a/pkg/cli/testdata/zip/testzip_include_range_info b/pkg/cli/testdata/zip/testzip_include_range_info index 57db1103b6bf..5d427e50eb23 100644 --- a/pkg/cli/testdata/zip/testzip_include_range_info +++ b/pkg/cli/testdata/zip/testzip_include_range_info @@ -32,6 +32,7 @@ debug zip --concurrency=1 --cpu-profile-duration=1s --include-range-info /dev/nu [cluster] retrieving SQL data for crdb_internal.kv_store_status... writing output: debug/crdb_internal.kv_store_status.txt... done [cluster] retrieving SQL data for crdb_internal.kv_system_privileges... writing output: debug/crdb_internal.kv_system_privileges.txt... done [cluster] retrieving SQL data for crdb_internal.partitions... writing output: debug/crdb_internal.partitions.txt... done +[cluster] retrieving SQL data for crdb_internal.probe_ranges_1s_write_limit_100... writing output: debug/crdb_internal.probe_ranges_1s_write_limit_100.txt... done [cluster] retrieving SQL data for crdb_internal.regions... writing output: debug/crdb_internal.regions.txt... done [cluster] retrieving SQL data for crdb_internal.schema_changes... writing output: debug/crdb_internal.schema_changes.txt... done [cluster] retrieving SQL data for crdb_internal.super_regions... writing output: debug/crdb_internal.super_regions.txt... done diff --git a/pkg/cli/testdata/zip/testzip_shared_process_virtualization b/pkg/cli/testdata/zip/testzip_shared_process_virtualization index 5592b84ae295..3f4a45377579 100644 --- a/pkg/cli/testdata/zip/testzip_shared_process_virtualization +++ b/pkg/cli/testdata/zip/testzip_shared_process_virtualization @@ -32,6 +32,7 @@ debug zip --concurrency=1 --cpu-profile-duration=1s /dev/null [cluster] retrieving SQL data for crdb_internal.kv_store_status... writing output: debug/crdb_internal.kv_store_status.txt... done [cluster] retrieving SQL data for crdb_internal.kv_system_privileges... writing output: debug/crdb_internal.kv_system_privileges.txt... done [cluster] retrieving SQL data for crdb_internal.partitions... writing output: debug/crdb_internal.partitions.txt... done +[cluster] retrieving SQL data for crdb_internal.probe_ranges_1s_write_limit_100... writing output: debug/crdb_internal.probe_ranges_1s_write_limit_100.txt... done [cluster] retrieving SQL data for crdb_internal.regions... writing output: debug/crdb_internal.regions.txt... done [cluster] retrieving SQL data for crdb_internal.schema_changes... writing output: debug/crdb_internal.schema_changes.txt... done [cluster] retrieving SQL data for crdb_internal.super_regions... writing output: debug/crdb_internal.super_regions.txt... done @@ -159,6 +160,7 @@ debug zip --concurrency=1 --cpu-profile-duration=1s /dev/null [cluster] retrieving SQL data for crdb_internal.kv_store_status: creating error output: debug/cluster/test-tenant/crdb_internal.kv_store_status.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.kv_system_privileges... writing output: debug/cluster/test-tenant/crdb_internal.kv_system_privileges.txt... done [cluster] retrieving SQL data for crdb_internal.partitions... writing output: debug/cluster/test-tenant/crdb_internal.partitions.txt... done +[cluster] retrieving SQL data for crdb_internal.probe_ranges_1s_write_limit_100... writing output: debug/cluster/test-tenant/crdb_internal.probe_ranges_1s_write_limit_100.txt... done [cluster] retrieving SQL data for crdb_internal.regions... writing output: debug/cluster/test-tenant/crdb_internal.regions.txt... done [cluster] retrieving SQL data for crdb_internal.schema_changes... writing output: debug/cluster/test-tenant/crdb_internal.schema_changes.txt... done [cluster] retrieving SQL data for crdb_internal.super_regions... writing output: debug/cluster/test-tenant/crdb_internal.super_regions.txt... done diff --git a/pkg/cli/zip_table_registry.go b/pkg/cli/zip_table_registry.go index 7280b2917cbb..513f2e246b92 100644 --- a/pkg/cli/zip_table_registry.go +++ b/pkg/cli/zip_table_registry.go @@ -228,6 +228,10 @@ var zipInternalTablesPerCluster = DebugZipTableRegistry{ origin FROM crdb_internal.cluster_settings`, }, + "crdb_internal.probe_ranges_1s_write_limit_100": { + customQueryRedacted: `SELECT * FROM crdb_internal.probe_ranges(INTERVAL '1000ms', 'write') WHERE error != '' ORDER BY end_to_end_latency_ms DESC LIMIT 100;`, + customQueryUnredacted: `SELECT * FROM crdb_internal.probe_ranges(INTERVAL '1000ms', 'write') WHERE error != '' ORDER BY end_to_end_latency_ms DESC LIMIT 100;`, + }, "crdb_internal.cluster_transactions": { // `last_auto_retry_reason` contains error text that may contain // sensitive data. diff --git a/pkg/cli/zip_test.go b/pkg/cli/zip_test.go index 763c455c33bc..bc0e7fe4bfe7 100644 --- a/pkg/cli/zip_test.go +++ b/pkg/cli/zip_test.go @@ -122,6 +122,7 @@ ORDER BY name ASC`) assert.NoError(t, rows.Scan(&table)) tables = append(tables, table) } + tables = append(tables, "crdb_internal.probe_ranges_1s_write_limit_100") sort.Strings(tables) var exp []string From 5f384b3af1a66478069f2370c58bb51184a5a7dd Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Fri, 7 Jul 2023 12:21:33 +0000 Subject: [PATCH 08/14] DEPS: bump golang.org/x/exp Needed to pull in `constraints.Ordered`. Epic: none Release note: None --- DEPS.bzl | 12 ++++++------ build/bazelutil/distdir_files.bzl | 4 ++-- go.mod | 4 ++-- go.sum | 8 ++++---- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/DEPS.bzl b/DEPS.bzl index 723975a50e39..e6c5d017480f 100644 --- a/DEPS.bzl +++ b/DEPS.bzl @@ -11160,10 +11160,10 @@ def go_deps(): name = "org_golang_x_exp", build_file_proto_mode = "disable_global", importpath = "golang.org/x/exp", - sha256 = "d22b8623bef1ead959b1c6449512dfdbf9624e1ec6c4bc443e91eee170d4d7af", - strip_prefix = "golang.org/x/exp@v0.0.0-20220827204233-334a2380cb91", + sha256 = "af32025a065aa599a3e5b01048602a53e2b6e3938b12d33fa2a5f057be9759fa", + strip_prefix = "golang.org/x/exp@v0.0.0-20230626212559-97b1e661b5df", urls = [ - "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/exp/org_golang_x_exp-v0.0.0-20220827204233-334a2380cb91.zip", + "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/exp/org_golang_x_exp-v0.0.0-20230626212559-97b1e661b5df.zip", ], ) go_repository( @@ -11210,10 +11210,10 @@ def go_deps(): name = "org_golang_x_mod", build_file_proto_mode = "disable_global", importpath = "golang.org/x/mod", - sha256 = "ad0b72a29c07f57d92612fdbc2da6891b3988dd98b31d2fdcbe8ddb9c8cc2d52", - strip_prefix = "golang.org/x/mod@v0.9.0", + sha256 = "364198930cc7f46ba5bb1c0987d089a557aa0b406f8efec0490744a454df00a5", + strip_prefix = "golang.org/x/mod@v0.11.0", urls = [ - "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/mod/org_golang_x_mod-v0.9.0.zip", + "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/mod/org_golang_x_mod-v0.11.0.zip", ], ) go_repository( diff --git a/build/bazelutil/distdir_files.bzl b/build/bazelutil/distdir_files.bzl index d2ec400f1978..b7657e76c13d 100644 --- a/build/bazelutil/distdir_files.bzl +++ b/build/bazelutil/distdir_files.bzl @@ -1051,12 +1051,12 @@ DISTDIR_FILES = { "https://storage.googleapis.com/cockroach-godeps/gomod/goji.io/io_goji-v2.0.2+incompatible.zip": "1ea69b28e356cb91381ce2339004fcf144ad1b268c9e3497c9ef304751ae0bb3", "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/arch/org_golang_x_arch-v0.0.0-20180920145803-b19384d3c130.zip": "9f67b677a3fefc503111d9aa7df8bacd2677411b0fcb982eb1654aa6d14cc3f8", "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/crypto/org_golang_x_crypto-v0.9.0.zip": "672ebc916740a040d5f5472b477e1d1898e06b0c6c0a5a820b65495bbb133e82", - "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/exp/org_golang_x_exp-v0.0.0-20220827204233-334a2380cb91.zip": "d22b8623bef1ead959b1c6449512dfdbf9624e1ec6c4bc443e91eee170d4d7af", + "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/exp/org_golang_x_exp-v0.0.0-20230626212559-97b1e661b5df.zip": "af32025a065aa599a3e5b01048602a53e2b6e3938b12d33fa2a5f057be9759fa", "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/exp/typeparams/org_golang_x_exp_typeparams-v0.0.0-20221208152030-732eee02a75a.zip": "9bd73f186851c6229484f486981f608d16e2b86acbbef6f4f7cc0480a508a4a4", "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/image/org_golang_x_image-v0.0.0-20210628002857-a66eb6448b8d.zip": "70cf423fad9be160a88fbf01bc1897efd888f915a6d7ba0dd41ca7085f75e06e", "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/lint/org_golang_x_lint-v0.0.0-20210508222113-6edffad5e616.zip": "0a4a5ebd2b1d79e7f480cbf5a54b45a257ae1ec9d11f01688efc5c35268d4603", "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/mobile/org_golang_x_mobile-v0.0.0-20190719004257-d2bd2a29d028.zip": "6b946c7da47acf3b6195336fd071bfc73d543cefab73f2d27528c5dc1dc829ec", - "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/mod/org_golang_x_mod-v0.9.0.zip": "ad0b72a29c07f57d92612fdbc2da6891b3988dd98b31d2fdcbe8ddb9c8cc2d52", + "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/mod/org_golang_x_mod-v0.11.0.zip": "364198930cc7f46ba5bb1c0987d089a557aa0b406f8efec0490744a454df00a5", "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/net/org_golang_x_net-v0.10.0.zip": "f92f9b2655226a6d015af7a76279a11fb55678e410b851b158fc846546f80733", "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/oauth2/org_golang_x_oauth2-v0.5.0.zip": "4d9d9ce8d2f55b7d2ffb3697ed811a54606e881bb7fbbd84d3d9c42fc404ae3c", "https://storage.googleapis.com/cockroach-godeps/gomod/golang.org/x/perf/org_golang_x_perf-v0.0.0-20230113213139-801c7ef9e5c5.zip": "bc1b902e645fdd5d210b7db8f3280833af225b131dab5842d7a6d32a676f80f5", diff --git a/go.mod b/go.mod index bc7208ec65b4..c824ae226d79 100644 --- a/go.mod +++ b/go.mod @@ -17,9 +17,9 @@ require ( github.com/google/pprof v0.0.0-20210827144239-02619b876842 github.com/google/uuid v1.3.0 golang.org/x/crypto v0.9.0 - golang.org/x/exp v0.0.0-20220827204233-334a2380cb91 + golang.org/x/exp v0.0.0-20230626212559-97b1e661b5df golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a // indirect - golang.org/x/mod v0.9.0 // indirect + golang.org/x/mod v0.11.0 // indirect golang.org/x/net v0.10.0 golang.org/x/oauth2 v0.5.0 golang.org/x/sync v0.1.0 diff --git a/go.sum b/go.sum index 76e50526b38a..a09fd3a6520f 100644 --- a/go.sum +++ b/go.sum @@ -2485,8 +2485,8 @@ golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u0 golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4= golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM= golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU= -golang.org/x/exp v0.0.0-20220827204233-334a2380cb91 h1:tnebWN09GYg9OLPss1KXj8txwZc6X6uMr6VFdcGNbHw= -golang.org/x/exp v0.0.0-20220827204233-334a2380cb91/go.mod h1:cyybsKvd6eL0RnXn6p/Grxp8F5bW7iYuBgsNCOHpMYE= +golang.org/x/exp v0.0.0-20230626212559-97b1e661b5df h1:UA2aFVmmsIlefxMk29Dp2juaUSth8Pyn3Tq5Y5mJGME= +golang.org/x/exp v0.0.0-20230626212559-97b1e661b5df/go.mod h1:FXUEEKJgO7OQYeo8N01OfiKP8RXMtf6e8aTskBGqWdc= golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a h1:Jw5wfR+h9mnIYH+OtGT2im5wV1YGGDora5vTv/aa5bE= golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a/go.mod h1:AbB0pIl9nAr9wVwH+Z2ZpaocVmF5I4GyWCDIsVjR0bk= golang.org/x/image v0.0.0-20180708004352-c73c2afc3b81/go.mod h1:ux5Hcp/YLpHSI86hEcLt0YII63i6oz57MZXIpbrjZUs= @@ -2525,8 +2525,8 @@ golang.org/x/mod v0.4.1/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.5.1/go.mod h1:5OXOZSfqPIIbmVBIIKWRFfZjPR0E5r58TLhUjH0a2Ro= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= -golang.org/x/mod v0.9.0 h1:KENHtAZL2y3NLMYZeHY9DW8HW8V+kQyJsY/V9JlKvCs= -golang.org/x/mod v0.9.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= +golang.org/x/mod v0.11.0 h1:bUO06HqtnRcc/7l71XBe4WcqTZ+3AH1J59zWDDwLKgU= +golang.org/x/mod v0.11.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/net v0.0.0-20170114055629-f2499483f923/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180218175443-cbe0f9307d01/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180530234432-1e491301e022/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= From 4ffc54c9940f96fcecc6a8a493fa76b88fa863d0 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Sat, 8 Jul 2023 11:57:26 +0000 Subject: [PATCH 09/14] logcrash: add `ReportTypeAssertionFailure` Epic: none Release note: None --- pkg/util/log/logcrash/crash_reporting.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/pkg/util/log/logcrash/crash_reporting.go b/pkg/util/log/logcrash/crash_reporting.go index ead3913e3a47..5c35fae3a7b0 100644 --- a/pkg/util/log/logcrash/crash_reporting.go +++ b/pkg/util/log/logcrash/crash_reporting.go @@ -317,6 +317,9 @@ const ( // ReportTypeLogFatal signifies that this is an error report that // was generated via a log.Fatal call. ReportTypeLogFatal + // ReportTypeAssertionFailure signifies that an assertion was violated (see + // expect package). + ReportTypeAssertionFailure ) // sendCrashReport posts to sentry. @@ -376,6 +379,8 @@ func SendReport( event.Tags["report_type"] = "error" case ReportTypeLogFatal: event.Tags["report_type"] = "log_fatal" + case ReportTypeAssertionFailure: + event.Tags["report_type"] = "assertion" } for _, f := range tagFns { @@ -437,13 +442,15 @@ func RegisterTagFn(key string, value func(context.Context) string) { tagFns = append(tagFns, tagFn{key, value}) } -func maybeSendCrashReport(ctx context.Context, err error) { +func maybeSendCrashReport(ctx context.Context, err error, reportType ReportType) { // We load the ReportingSettings from global singleton in this call path. if sv := getGlobalSettings(); sv != nil { - sendCrashReport(ctx, sv, err, ReportTypeLogFatal) + sendCrashReport(ctx, sv, err, reportType) } } func init() { - log.MaybeSendCrashReport = maybeSendCrashReport + log.MaybeSendCrashReport = func(ctx context.Context, err error) { + maybeSendCrashReport(ctx, err, ReportTypeLogFatal) + } } From 32504778bdc173d4b6ce3e446b681b5d8c0baba2 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Mon, 10 Jul 2023 10:37:29 +0000 Subject: [PATCH 10/14] util/must: add runtime assertion API This patch adds a convenient and canonical API for runtime assertions, inspired by the Testify package used for Go test assertions. It is intended to encourage liberal use of runtime assertions throughout the code base, by making it as easy as possible to write assertions that follow best practices. It does not attempt to reinvent the wheel, but instead builds on existing infrastructure. Assertion failures are fatal in all non-release builds, including roachprod clusters and roachtests, to ensure they will be noticed. In release builds, they instead log the failure and report it to Sentry (if enabled), and return an assertion error to the caller for propagation. This avoids excessive disruption in production environments, where an assertion failure is often scoped to an individual RPC request, transaction, or range, and crashing the node can turn a minor problem into a full-blown outage. It is still possible to kill the node when appropriate via `log.Fatalf`, but this should be the exception rather than the norm. It also supports expensive assertions that must be compiled out of normal dev/test/release builds for performance reasons. These are instead enabled in special test builds. This is intended to be used instead of other existing assertion mechanisms, which have various shortcomings: * `log.Fatalf`: kills the node even in release builds, which can cause severe disruption over often minor issues. * `errors.AssertionFailedf`: only suitable when we have an error return path, does not fatal in non-release builds, and are not always notified in release builds. * `logcrash.ReportOrPanic`: panics rather than fatals, which can leave the node limping along. Requires the caller to implement separate assertion handling in release builds, which is easy to forget. Also requires propagating cluster settings, which aren't always available. * `buildutil.CrdbTestBuild`: only enabled in Go tests, not roachtests, roachprod clusters, or production clusters. * `util.RaceEnabled`: only enabled in race builds. Expensive assertions should be possible to run without the additional overhead of the race detector. For more details and examples, see the `must` package documentation. Epic: none Release note: None --- pkg/BUILD.bazel | 3 + pkg/testutils/lint/gcassert_paths.txt | 1 + .../lint/passes/fmtsafe/functions.go | 29 + pkg/util/log/logcrash/BUILD.bazel | 1 + pkg/util/log/logcrash/crash_reporting.go | 6 +- pkg/util/must/BUILD.bazel | 36 ++ pkg/util/must/must.go | 579 ++++++++++++++++++ pkg/util/must/must_test.go | 426 +++++++++++++ .../must/testdata/TestAssertions/Contains | 6 + pkg/util/must/testdata/TestAssertions/Empty | 3 + pkg/util/must/testdata/TestAssertions/Equal | 9 + .../must/testdata/TestAssertions/EqualBytes | 8 + pkg/util/must/testdata/TestAssertions/Error | 3 + pkg/util/must/testdata/TestAssertions/False | 3 + pkg/util/must/testdata/TestAssertions/Greater | 8 + .../testdata/TestAssertions/GreaterOrEqual | 5 + pkg/util/must/testdata/TestAssertions/Len | 3 + pkg/util/must/testdata/TestAssertions/Less | 8 + .../must/testdata/TestAssertions/LessOrEqual | 5 + pkg/util/must/testdata/TestAssertions/Nil | 3 + pkg/util/must/testdata/TestAssertions/NoError | 3 + .../must/testdata/TestAssertions/NotContains | 4 + .../must/testdata/TestAssertions/NotEmpty | 4 + .../must/testdata/TestAssertions/NotEqual | 9 + .../testdata/TestAssertions/NotEqualBytes | 7 + pkg/util/must/testdata/TestAssertions/NotNil | 3 + .../testdata/TestAssertions/NotPrefixBytes | 12 + pkg/util/must/testdata/TestAssertions/NotSame | 4 + pkg/util/must/testdata/TestAssertions/NotZero | 8 + .../must/testdata/TestAssertions/PrefixBytes | 8 + pkg/util/must/testdata/TestAssertions/Same | 8 + pkg/util/must/testdata/TestAssertions/True | 3 + pkg/util/must/testdata/TestAssertions/Zero | 8 + 33 files changed, 1225 insertions(+), 1 deletion(-) create mode 100644 pkg/util/must/BUILD.bazel create mode 100644 pkg/util/must/must.go create mode 100644 pkg/util/must/must_test.go create mode 100644 pkg/util/must/testdata/TestAssertions/Contains create mode 100644 pkg/util/must/testdata/TestAssertions/Empty create mode 100644 pkg/util/must/testdata/TestAssertions/Equal create mode 100644 pkg/util/must/testdata/TestAssertions/EqualBytes create mode 100644 pkg/util/must/testdata/TestAssertions/Error create mode 100644 pkg/util/must/testdata/TestAssertions/False create mode 100644 pkg/util/must/testdata/TestAssertions/Greater create mode 100644 pkg/util/must/testdata/TestAssertions/GreaterOrEqual create mode 100644 pkg/util/must/testdata/TestAssertions/Len create mode 100644 pkg/util/must/testdata/TestAssertions/Less create mode 100644 pkg/util/must/testdata/TestAssertions/LessOrEqual create mode 100644 pkg/util/must/testdata/TestAssertions/Nil create mode 100644 pkg/util/must/testdata/TestAssertions/NoError create mode 100644 pkg/util/must/testdata/TestAssertions/NotContains create mode 100644 pkg/util/must/testdata/TestAssertions/NotEmpty create mode 100644 pkg/util/must/testdata/TestAssertions/NotEqual create mode 100644 pkg/util/must/testdata/TestAssertions/NotEqualBytes create mode 100644 pkg/util/must/testdata/TestAssertions/NotNil create mode 100644 pkg/util/must/testdata/TestAssertions/NotPrefixBytes create mode 100644 pkg/util/must/testdata/TestAssertions/NotSame create mode 100644 pkg/util/must/testdata/TestAssertions/NotZero create mode 100644 pkg/util/must/testdata/TestAssertions/PrefixBytes create mode 100644 pkg/util/must/testdata/TestAssertions/Same create mode 100644 pkg/util/must/testdata/TestAssertions/True create mode 100644 pkg/util/must/testdata/TestAssertions/Zero diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index bd682f7a1445..37cc89062a08 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -661,6 +661,7 @@ ALL_TESTS = [ "//pkg/util/metric/aggmetric:aggmetric_test", "//pkg/util/metric:metric_test", "//pkg/util/mon:mon_test", + "//pkg/util/must:must_test", "//pkg/util/netutil/addr:addr_test", "//pkg/util/netutil:netutil_test", "//pkg/util/optional:optional_test", @@ -2337,6 +2338,8 @@ GO_TARGETS = [ "//pkg/util/metric:metric_test", "//pkg/util/mon:mon", "//pkg/util/mon:mon_test", + "//pkg/util/must:must", + "//pkg/util/must:must_test", "//pkg/util/netutil/addr:addr", "//pkg/util/netutil/addr:addr_test", "//pkg/util/netutil:netutil", diff --git a/pkg/testutils/lint/gcassert_paths.txt b/pkg/testutils/lint/gcassert_paths.txt index 94288e2cf952..b38225ce4ee9 100644 --- a/pkg/testutils/lint/gcassert_paths.txt +++ b/pkg/testutils/lint/gcassert_paths.txt @@ -29,3 +29,4 @@ util util/admission util/hlc util/intsets +util/must diff --git a/pkg/testutils/lint/passes/fmtsafe/functions.go b/pkg/testutils/lint/passes/fmtsafe/functions.go index ac4339ed4d8e..5f7f364911ab 100644 --- a/pkg/testutils/lint/passes/fmtsafe/functions.go +++ b/pkg/testutils/lint/passes/fmtsafe/functions.go @@ -162,6 +162,35 @@ var requireConstFmt = map[string]bool{ "(github.com/cockroachdb/cockroach/pkg/kv/kvpb.TestPrinter).Printf": true, + // must assertions. + "github.com/cockroachdb/cockroach/pkg/util/must.Fail": true, + "github.com/cockroachdb/cockroach/pkg/util/must.failDepth": true, + "github.com/cockroachdb/cockroach/pkg/util/must.True": true, + "github.com/cockroachdb/cockroach/pkg/util/must.False": true, + "github.com/cockroachdb/cockroach/pkg/util/must.Equal": true, + "github.com/cockroachdb/cockroach/pkg/util/must.NotEqual": true, + "github.com/cockroachdb/cockroach/pkg/util/must.Greater": true, + "github.com/cockroachdb/cockroach/pkg/util/must.GreaterOrEqual": true, + "github.com/cockroachdb/cockroach/pkg/util/must.Less": true, + "github.com/cockroachdb/cockroach/pkg/util/must.LessOrEqual": true, + "github.com/cockroachdb/cockroach/pkg/util/must.EqualBytes": true, + "github.com/cockroachdb/cockroach/pkg/util/must.NotEqualBytes": true, + "github.com/cockroachdb/cockroach/pkg/util/must.PrefixBytes": true, + "github.com/cockroachdb/cockroach/pkg/util/must.NotPrefixBytes": true, + "github.com/cockroachdb/cockroach/pkg/util/must.Len": true, + "github.com/cockroachdb/cockroach/pkg/util/must.Contains": true, + "github.com/cockroachdb/cockroach/pkg/util/must.NotContains": true, + "github.com/cockroachdb/cockroach/pkg/util/must.Empty": true, + "github.com/cockroachdb/cockroach/pkg/util/must.NotEmpty": true, + "github.com/cockroachdb/cockroach/pkg/util/must.Nil": true, + "github.com/cockroachdb/cockroach/pkg/util/must.NotNil": true, + "github.com/cockroachdb/cockroach/pkg/util/must.Same": true, + "github.com/cockroachdb/cockroach/pkg/util/must.NotSame": true, + "github.com/cockroachdb/cockroach/pkg/util/must.Zero": true, + "github.com/cockroachdb/cockroach/pkg/util/must.NotZero": true, + "github.com/cockroachdb/cockroach/pkg/util/must.Error": true, + "github.com/cockroachdb/cockroach/pkg/util/must.NoError": true, + // Error things are populated in the init() message. } diff --git a/pkg/util/log/logcrash/BUILD.bazel b/pkg/util/log/logcrash/BUILD.bazel index 83b592fdc5fb..8211df8f75a7 100644 --- a/pkg/util/log/logcrash/BUILD.bazel +++ b/pkg/util/log/logcrash/BUILD.bazel @@ -14,6 +14,7 @@ go_library( "//pkg/util/envutil", "//pkg/util/log", "//pkg/util/log/severity", + "//pkg/util/must", "//pkg/util/timeutil", "@com_github_cockroachdb_errors//:errors", "@com_github_getsentry_sentry_go//:sentry-go", diff --git a/pkg/util/log/logcrash/crash_reporting.go b/pkg/util/log/logcrash/crash_reporting.go index 5c35fae3a7b0..522b8a9947b4 100644 --- a/pkg/util/log/logcrash/crash_reporting.go +++ b/pkg/util/log/logcrash/crash_reporting.go @@ -22,6 +22,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/envutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/log/severity" + "github.com/cockroachdb/cockroach/pkg/util/must" "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/errors" sentry "github.com/getsentry/sentry-go" @@ -318,7 +319,7 @@ const ( // was generated via a log.Fatal call. ReportTypeLogFatal // ReportTypeAssertionFailure signifies that an assertion was violated (see - // expect package). + // must package). ReportTypeAssertionFailure ) @@ -453,4 +454,7 @@ func init() { log.MaybeSendCrashReport = func(ctx context.Context, err error) { maybeSendCrashReport(ctx, err, ReportTypeLogFatal) } + must.MaybeSendReport = func(ctx context.Context, err error) { + maybeSendCrashReport(ctx, err, ReportTypeAssertionFailure) + } } diff --git a/pkg/util/must/BUILD.bazel b/pkg/util/must/BUILD.bazel new file mode 100644 index 000000000000..4d98ff02256b --- /dev/null +++ b/pkg/util/must/BUILD.bazel @@ -0,0 +1,36 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "must", + srcs = ["must.go"], + importpath = "github.com/cockroachdb/cockroach/pkg/util/must", + visibility = ["//visibility:public"], + deps = [ + "//pkg/build", + "//pkg/util", + "//pkg/util/envutil", + "//pkg/util/log", + "@com_github_cockroachdb_errors//:errors", + "@org_golang_x_exp//constraints", + ], +) + +go_test( + name = "must_test", + srcs = ["must_test.go"], + args = ["-test.timeout=295s"], + data = glob(["testdata/**"]), + deps = [ + ":must", + "//pkg/build", + "//pkg/keys", + "//pkg/testutils/datapathutils", + "//pkg/testutils/echotest", + "//pkg/util", + "//pkg/util/hlc", + "//pkg/util/leaktest", + "@com_github_cockroachdb_errors//:errors", + "@com_github_cockroachdb_redact//:redact", + "@com_github_stretchr_testify//require", + ], +) diff --git a/pkg/util/must/must.go b/pkg/util/must/must.go new file mode 100644 index 000000000000..85937544f88b --- /dev/null +++ b/pkg/util/must/must.go @@ -0,0 +1,579 @@ +// 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 must provides a convenient and performant API for runtime assertions, +// similar to Testify. It is intended to encourage liberal use of assertions. +// +// Runtime assertions are great: they build confidence in invariants, plug gaps +// in test coverage, and run across all CRDB environments and workloads where +// they can detect bugs even in exceedingly rare corner cases. +// +// Assertions are used to check and enforce invariants, not for general error +// handling. As a rule of thumb, only use an assertion for something that you +// never expect to fail, ever. Some examples: +// +// - Good assertion: a range's start key must be before its end key. This must +// always be true, so this should be an assertion. +// +// - Bad assertion: writing to a file must succeed. We expect this to fail +// sometimes with e.g. faulty disks, so this should be a (possibly fatal) +// error, not an assertion. +// +// - Worse assertion: users must enter a valid SQL query. We expect users to +// get this wrong all the time, so this should return an error. +// +// In non-release builds (including roachprod and roachtest clusters), assertion +// failures kill the process with a stack trace to ensure failures are noticed. +// +// In release builds, assertion failures do not kill the process, since this is +// often excessive -- for example, an assertion failure during RPC request +// processing should fail the RPC request, not kill the process. Instead, an +// assertion error is logged with a stack trace, reported to Sentry (if +// enabled), and returned to the caller. The caller should process the error as +// appropriate, e.g. return it up the stack or ignore it. +// +// In rare cases, it may be appropriate to always fatal or panic, regardless of +// build type, typically when it is unsafe to keep the process running. The +// helpers FatalOn() and PanicOn() can be used for this. +// +// Additionally, fatal assertions can be enabled or disabled via the +// COCKROACH_FATAL_ASSERTIONS environment variable, regardless of build type. +// +// Because assertion failures return an error, they must be explicitly handled, +// or the errcheck linter will complain. At the very least, the error must be +// ignored (if it is safe) with e.g. _ = must.True(ctx, foo, "bar"). This is +// intentional: we don't want to fatal in release builds, so the caller must +// consider how to handle the error there. +// +// This can also lead to non-fatal assertion failures being logged or reported +// in two places: at the assertion failure site, and where the original caller +// fails when it receives the error. This is also intentional, since it provides +// additional details about the caller, who is often an RPC client on a +// different node (thus stack traces can be insufficient by themselves). +// +// Some assertions may be too expensive to always run. For example, we may want +// to recompute a range's MVCC stats after every write command, and assert that +// they equal the incremental stats computed by the command. This can be done +// via must.Expensive(), which only runs the given assertions in special test +// builds (currently race builds), and compiles them out in other builds. +// +// Below are several usage examples. +// +// Example: guarding against long log lines. In release builds, the assertion +// failure can be ignored, since it does not affect the subsequent logic (we can +// still log the line even if it's long). +// +// func Log(ctx context.Context, line string) { +// _ = must.LessOrEqual(ctx, len(line), 1024, "log line too long: %q", line) +// log.Infof(ctx, line) // runs in release builds +// } +// +// Example: double-stopping a component. In release builds, we can simply ignore +// the failure and return early, since it has already been stopped. +// +// func (p *Processor) Stop(ctx context.Context) { +// if err := must.False(ctx, p.stopped, "already stopped"); err != nil { +// _ = err +// return // runs in release builds +// } +// p.stopped = true +// // ... +// } +// +// Example: missing lease during a range split. Return an assertion error to the +// caller which fails the split, since we have an error return path. +// +// func splitTrigger( +// ctx context.Context, rec EvalContext, batch storage.Batch, split *roachpb.SplitTrigger, +// ) (result.Result, error) { +// leftLease, err := MakeStateLoader(rec).LoadLease(ctx, batch) +// if err != nil { +// return result.Result{}, err +// } +// if err := must.NotZero(ctx, leftLease, "LHS of split has no lease"); err != nil { +// return result.Result{}, err // runs in release builds +// } +// // ... +// } +// +// Example: unknown txn status enum value during EndTxn. We call must.Fail +// directly since we've already checked the condition, and return the error to +// the caller in release builds. +// +// func EndTxn( +// ctx context.Context, rw storage.ReadWriter, cArgs CommandArgs, resp kvpb.Response, +// ) (result.Result, error) { +// // ... +// switch reply.Txn.Status { +// case roachpb.COMMITTED: // ... +// case roachpb.ABORTED: // ... +// case roachpb.PENDING: // ... +// case roachpb.STAGING: // ... +// default: +// err := must.Fail(ctx, "invalid txn status %s", reply.Txn.Status) +// return result.Result{}, err // runs in release builds +// } +// // ... +// } +// +// Example: verify range MVCC stats are accurate after every write. This is too +// expensive to do even in non-release builds, but we may want to check this in +// dedicated test builds. +// +// func (b *replicaAppBatch) ApplyToStateMachine(ctx context.Context) error { +// *r.mu.state.Stats = *b.state.Stats +// if err := must.Expensive(func() error { +// computedStats, err := store.ComputeStats(r.store.Engine, desc.StartKey, desc.EndKey, now) +// if err != nil { +// return err +// } +// return must.Equal(ctx, r.mu.state.Stats, computedStats, "MVCC stats diverged") +// }); err != nil { +// return err +// } +// // ... +// } +// +// Example: Pebble returns a corruption error, so we should kill the process. +// This is not an assertion, but rather legitimate error handling, so we fatal +// via log.Fatalf() even in release builds. +// +// if err := iter.Close(); err == pebble.ErrCorruption { +// log.Fatalf(ctx, "%v", err) +// } +package must + +import ( + "context" + "fmt" + + "github.com/cockroachdb/cockroach/pkg/build" + "github.com/cockroachdb/cockroach/pkg/util" + "github.com/cockroachdb/cockroach/pkg/util/envutil" + "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/errors" + "golang.org/x/exp/constraints" +) + +var ( + // FatalAssertions will exit the process with a fatal error on assertion + // failures. It defaults to true in non-release builds. Otherwise, an + // assertion error is logged and returned. + FatalAssertions = envutil.EnvOrDefaultBool("COCKROACH_FATAL_ASSERTIONS", !build.IsRelease()) + + // MaybeSendReport is injected by package logcrash, and reports errors to + // Sentry if enabled. + MaybeSendReport func(ctx context.Context, err error) + + // OnFail can be injected in tests. + OnFail func(ctx context.Context, err error) +) + +// Fail triggers an assertion failure. In non-release builds, it fatals with a +// stack trace. In release builds, if returns an assertion error, logs it with a +// stack trace, and reports it to Sentry (if enabled). +// +// gcassert:inline +func Fail(ctx context.Context, format string, args ...interface{}) error { + return failDepth(ctx, 1, format, args...) +} + +// failDepth is like Fail, but removes the given number of stack frames in the +// stack trace and the source code origin of the log message. +func failDepth(ctx context.Context, depth int, format string, args ...interface{}) error { + depth += 1 + err := errors.AssertionFailedWithDepthf(depth, format, args...) + if OnFail != nil { + OnFail(ctx, err) + } else if FatalAssertions { + log.FatalfDepth(ctx, depth, "%+v", err) + } else { + log.ErrorfDepth(ctx, depth, "%+v", err) + MaybeSendReport(ctx, err) + } + return err +} + +// FatalOn unconditionally promotes an assertion failure to a fatal error, even +// in release builds or when fatal assertions are disabled. This should only be +// used when it is unsafe to keep the process running. +// +// gcassert:inline +func FatalOn(ctx context.Context, err error) { + if err != nil { + log.FatalfDepth(ctx, 1, "%+v", err) + } +} + +// PanicOn promotes a non-fatal assertion failure to a panic (typically in +// release builds). This is mostly for use in SQL APIs where errors are +// propagated as panics. +// +// gcassert:inline +func PanicOn(err error) { + if err != nil { + panic(fmt.Sprintf("%+v", err)) + } +} + +// Expensive is used for assertions that must be compiled out of regular +// release/dev/test builds for performance reasons. They're enabled in race +// builds. The function should use assertions as normal and return failures. +// +// gcassert:inline +func Expensive(f func() error) error { + // TODO(erikgrinaker): consider gating these on a different tag. + if util.RaceEnabled { + return f() + } + return nil +} + +// True requires v to be true. Otherwise, fatals in dev builds or errors +// in release builds (by default). +// +// gcassert:inline +func True(ctx context.Context, v bool, format string, args ...interface{}) error { + if v { + return nil + } + return failDepth(ctx, 1, format, args...) +} + +// False requires v to be false. Otherwise, fatals in dev builds or errors +// in release builds (by default). +// +// gcassert:inline +func False(ctx context.Context, v bool, format string, args ...interface{}) error { + if !v { + return nil + } + return failDepth(ctx, 1, format, args...) +} + +// Equal requires a and b to be equal. Otherwise, fatals in dev builds or errors +// in release builds (by default). +// +// gcassert:inline +func Equal[T comparable](ctx context.Context, a, b T, format string, args ...interface{}) error { + // TODO(erikgrinaker): Consider erroring out on pointers, if possible. It's + // usually not what one wants, and can be a footgun. + if a == b { + return nil + } + // TODO(erikgrinaker): Consider printing a diff, or otherwise improve the + // formatting, for large values like entire structs. + format += ": %#v != %#v" + return failDepth(ctx, 1, format, append(args, a, b)...) +} + +// NotEqual requires a and b not to be equal. Otherwise, fatals in dev builds or +// errors in release builds (by default). +// +// gcassert:inline +func NotEqual[T comparable](ctx context.Context, a, b T, format string, args ...interface{}) error { + if a != b { + return nil + } + format += ": %#v == %#v" + return failDepth(ctx, 1, format, append(args, a, b)...) +} + +// Greater requires a > b. Otherwise, fatals in dev builds or errors in release +// builds (by default). +// +// gcassert:inline +func Greater[T constraints.Ordered]( + ctx context.Context, a, b T, format string, args ...interface{}, +) error { + if a > b { + return nil + } + format += ": %v <= %v" + return failDepth(ctx, 1, format, append(args, a, b)...) +} + +// GreaterOrEqual requires a >= b. Otherwise, fatals in dev builds or errors in +// release builds (by default). +// +// gcassert:inline +func GreaterOrEqual[T constraints.Ordered]( + ctx context.Context, a, b T, format string, args ...interface{}, +) error { + if a >= b { + return nil + } + format += ": %v < %v" + return failDepth(ctx, 1, format, append(args, a, b)...) +} + +// Less requires a < b. Otherwise, fatals in dev builds or errors in release +// builds (by default). +// +// gcassert:inline +func Less[T constraints.Ordered]( + ctx context.Context, a, b T, format string, args ...interface{}, +) error { + if a < b { + return nil + } + format += ": %v >= %v" + return failDepth(ctx, 1, format, append(args, a, b)...) +} + +// LessOrEqual requires a <= b. Otherwise, fatals in dev builds or errors in +// release builds (by default). +// +// gcassert:inline +func LessOrEqual[T constraints.Ordered]( + ctx context.Context, a, b T, format string, args ...interface{}, +) error { + if a <= b { + return nil + } + format += ": %v > %v" + return failDepth(ctx, 1, format, append(args, a, b)...) +} + +// EqualBytes requires two byte slices or strings to be equal. Otherwise, fatals +// in dev builds or errors in release builds (by default). +// +// gcassert:inline +func EqualBytes[T ~[]byte | ~string]( + ctx context.Context, a, b T, format string, args ...interface{}, +) error { + if string(a) == string(b) { + return nil + } + format += ": %q != %q" + return failDepth(ctx, 1, format, append(args, a, b)...) +} + +// NotEqualBytes requires two byte slices or strings not to be equal. Otherwise, +// fatals in dev builds or errors in release builds (by default). +// +// gcassert:inline +func NotEqualBytes[T ~[]byte | ~string]( + ctx context.Context, a, b T, format string, args ...interface{}, +) error { + if string(a) != string(b) { + return nil + } + format += ": %q == %q" + return failDepth(ctx, 1, format, append(args, a, b)...) +} + +// PrefixBytes requires a byte slice or string to have a given prefix. +// Otherwise, fatals in dev builds or errors in release builds (by default). +// +// gcassert:inline +func PrefixBytes[T ~[]byte | ~string]( + ctx context.Context, v, prefix T, format string, args ...interface{}, +) error { + if len(v) >= len(prefix) && string(v[:len(prefix)]) == string(prefix) { + return nil + } + format += ": %q does not have prefix %q" + return failDepth(ctx, 1, format, append(args, v, prefix)...) +} + +// NotPrefixBytes requires a byte slice or string to not have a given prefix. +// Otherwise, fatals in dev builds or errors in release builds (by default). +// +// gcassert:inline +func NotPrefixBytes[T ~[]byte | ~string]( + ctx context.Context, v, prefix T, format string, args ...interface{}, +) error { + if len(v) < len(prefix) || string(v[:len(prefix)]) != string(prefix) { + return nil + } + format += ": %q has prefix %q" + return failDepth(ctx, 1, format, append(args, v, prefix)...) +} + +// Len requires a slice to have the specified length. Otherwise, fatals in dev +// builds or errors in release builds (by default). +// +// TODO(erikgrinaker): This should handle maps and strings too, if possible. +// +// gcassert:inline +func Len[V any, T ~[]V]( + ctx context.Context, c T, length int, format string, args ...interface{}, +) error { + l := len(c) + if l == length { + return nil + } + format += ": length %d != %d" + return failDepth(ctx, 1, format, append(args, l, length)...) +} + +// Contains requires the slice to contain the given element. Otherwise, fatals +// in dev builds or errors in release builds (by default). +// +// gcassert:inline +func Contains[V comparable, T ~[]V]( + ctx context.Context, slice T, value V, format string, args ...interface{}, +) error { + for _, v := range slice { + if v == value { + return nil + } + } + format += ": %v not in %v" + return failDepth(ctx, 1, format, append(args, value, slice)...) +} + +// NotContains requires the slice to not contain the given element. Otherwise, +// fatals in dev builds or errors in release builds (by default). +// +// gcassert:inline +func NotContains[V comparable, T ~[]V]( + ctx context.Context, slice T, value V, format string, args ...interface{}, +) error { + for _, v := range slice { + if v == value { + format += ": %v is in %v" + return failDepth(ctx, 1, format, append(args, value, slice)...) + } + } + return nil +} + +// Empty requires an empty slice. Otherwise, fatals in dev builds or errors in +// release builds (by default). +// +// TODO(erikgrinaker): This should handle maps and strings too, if possible. +// +// gcassert:inline +func Empty[V any, T ~[]V](ctx context.Context, c T, format string, args ...interface{}) error { + l := len(c) + if l == 0 { + return nil + } + format += ": length %d != 0" + return failDepth(ctx, 1, format, append(args, l)...) +} + +// NotEmpty requires a non-empty slice. Otherwise, fatals in dev builds or +// errors in release builds (by default). +// +// TODO(erikgrinaker): This should handle maps and strings too, if possible. +// +// gcassert:inline +func NotEmpty[V any, T ~[]V](ctx context.Context, c T, format string, args ...interface{}) error { + if len(c) > 0 { + return nil + } + format += ": is empty" + return failDepth(ctx, 1, format, args...) +} + +// Nil requires v to be nil. Otherwise, fatals in dev builds or errors in +// release builds (by default). +// +// TODO(erikgrinaker): Should handle interfaces too, if possible with generics, +// but beware typed and untyped nils. +// +// gcassert:inline +func Nil[T any](ctx context.Context, v *T, format string, args ...interface{}) error { + if v == nil { + return nil + } + format += ": expected nil, got %#v" + return failDepth(ctx, 1, format, append(args, v)...) +} + +// NotNil requires v not to be nil. Otherwise, fatals in dev builds or errors in +// release builds (by default). +// +// gcassert:inline +func NotNil[T any](ctx context.Context, v *T, format string, args ...interface{}) error { + if v != nil { + return nil + } + format += ": value is nil" + return failDepth(ctx, 1, format, args...) +} + +// Zero requires v to be zero-valued for its type. Otherwise, fatals in dev +// builds or errors in release builds (by default). +// +// gcassert:inline +func Zero[T comparable](ctx context.Context, v T, format string, args ...interface{}) error { + var zero T + if v == zero { + return nil + } + format += ": expected zero value, got %#v" + return failDepth(ctx, 1, format, append(args, v)...) +} + +// NotZero requires v not to be zero-valued for its type. Otherwise, fatals in +// dev builds or errors in release builds (by default). +// +// gcassert:inline +func NotZero[T comparable](ctx context.Context, v T, format string, args ...interface{}) error { + var zero T + if v != zero { + return nil + } + format += ": %#v is zero-valued" + return failDepth(ctx, 1, format, append(args, v)...) +} + +// Same requires a and b to point to the same object (i.e. memory location). +// Otherwise, fatals in dev builds or errors in release builds (by default). +// +// gcassert:inline +func Same[T any](ctx context.Context, a, b *T, format string, args ...interface{}) error { + if a == b { + return nil + } + format += ": %p != %p\n%p is %#v\n%p is %#v'" + return failDepth(ctx, 1, format, append(args, a, b, a, a, b, b)...) +} + +// NotSame requires a and b to not point to the same object (i.e. memory +// location). Otherwise, fatals in dev builds or errors in release builds (by +// default). +// +// gcassert:inline +func NotSame[T any](ctx context.Context, a, b *T, format string, args ...interface{}) error { + if a != b { + return nil + } + format += ": %p == %p, %#v" + return failDepth(ctx, 1, format, append(args, a, b, a)...) +} + +// Error requires a non-nil error. Otherwise, fatals in dev builds or errors in +// release builds (by default). +// +// gcassert:inline +func Error(ctx context.Context, err error, format string, args ...interface{}) error { + if err != nil { + return nil // nolint:returnerrcheck + } + format += ": expected error, got nil" + return failDepth(ctx, 1, format, args...) +} + +// NoError requires a nil error. Otherwise, fatals in dev builds or errors in +// release builds (by default). +// +// gcassert:inline +func NoError(ctx context.Context, err error, format string, args ...interface{}) error { + if err == nil { + return nil + } + format += ": error %v" + return failDepth(ctx, 1, format, append(args, err)...) +} diff --git a/pkg/util/must/must_test.go b/pkg/util/must/must_test.go new file mode 100644 index 000000000000..094b4ee132a1 --- /dev/null +++ b/pkg/util/must/must_test.go @@ -0,0 +1,426 @@ +// 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 must_test + +import ( + "context" + "fmt" + "regexp" + "testing" + + "github.com/cockroachdb/cockroach/pkg/build" + "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/testutils/datapathutils" + "github.com/cockroachdb/cockroach/pkg/testutils/echotest" + "github.com/cockroachdb/cockroach/pkg/util" + "github.com/cockroachdb/cockroach/pkg/util/hlc" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/cockroach/pkg/util/must" + "github.com/cockroachdb/errors" + "github.com/cockroachdb/redact" + "github.com/stretchr/testify/require" +) + +// noopFail sets must.OnFail to a noop, and returns a cleanup function +// to be called when the test completes. +func noopFail() func() { + must.OnFail = func(context.Context, error) {} + return func() { + must.OnFail = nil + } +} + +// TestFail tests that assertion failures behave correctly. +func TestFail(t *testing.T) { + defer leaktest.AfterTest(t)() + defer noopFail()() + + // Fail returns an assertion error. + ctx := context.Background() + err := must.Fail(ctx, "foo: %s", "bar") + require.Error(t, err) + require.True(t, errors.HasAssertionFailure(err)) + + // Format args are redacted. + require.Equal(t, err.Error(), "foo: bar") + require.EqualValues(t, redact.Sprint(err), "foo: ‹bar›") + + // The error includes a stack trace, but strips the must.Fail frame. + require.Contains(t, fmt.Sprintf("%+v", err), ".TestFail") + require.NotContains(t, fmt.Sprintf("%+v", err), "must.Fail") + + // FatalAssertions is false in release builds, true otherwise. + require.Equal(t, must.FatalAssertions, !build.IsRelease()) + + // We don't test the fatal handling, since it can't easily be tested, and the + // logic is trivial. +} + +// TestExpensive tests that Expensive only runs under race builds. +func TestExpensive(t *testing.T) { + defer leaktest.AfterTest(t)() + defer noopFail()() + + // Expensive should only run under the race tag. When it does run, it returns + // the error from the closure. + errExpensive := errors.New("expensive") + var called bool + err := must.Expensive(func() error { + called = true + return errExpensive + }) + + if util.RaceEnabled { + require.True(t, called) + require.ErrorIs(t, err, errExpensive) + } else { + require.False(t, called) + require.NoError(t, err) + } +} + +// TestAssertions tests that various assertions fire correctly. +func TestAssertions(t *testing.T) { + defer leaktest.AfterTest(t)() + defer noopFail()() + + ctx := context.Background() + const format = "message with %s" + const arg = "argument" + p1, p2 := &hlc.Timestamp{Logical: 1}, &hlc.Timestamp{Logical: 1} + pNil := (*hlc.Timestamp)(nil) + + testcases := map[string][]struct { + err error + expectOK bool + }{ + "True": { + {must.True(ctx, true, format, arg), true}, + {must.True(ctx, false, format, arg), false}, + }, + + "False": { + {must.False(ctx, false, format, arg), true}, + {must.False(ctx, true, format, arg), false}, + }, + + "Equal": { + {must.Equal(ctx, true, true, format, arg), true}, + {must.Equal(ctx, true, false, format, arg), false}, + {must.Equal(ctx, 1, 1, format, arg), true}, + {must.Equal(ctx, 0, 1, format, arg), false}, + {must.Equal(ctx, 3.14, 3.14, format, arg), true}, + {must.Equal(ctx, 3.14, 2.717, format, arg), false}, + {must.Equal(ctx, "a", "a", format, arg), true}, + {must.Equal(ctx, "a", "b", format, arg), false}, + {must.Equal(ctx, hlc.Timestamp{Logical: 1}, hlc.Timestamp{Logical: 1}, format, arg), true}, + {must.Equal(ctx, hlc.Timestamp{Logical: 1}, hlc.Timestamp{Logical: 2}, format, arg), false}, + {must.Equal(ctx, p1, p1, format, arg), true}, + {must.Equal(ctx, p1, p2, format, arg), false}, + {must.Equal(ctx, pNil, pNil, format, arg), true}, + {must.Equal(ctx, pNil, p2, format, arg), false}, + }, + + "NotEqual": { + {must.NotEqual(ctx, true, false, format, arg), true}, + {must.NotEqual(ctx, true, true, format, arg), false}, + {must.NotEqual(ctx, 0, 1, format, arg), true}, + {must.NotEqual(ctx, 1, 1, format, arg), false}, + {must.NotEqual(ctx, 3.14, 2.717, format, arg), true}, + {must.NotEqual(ctx, 3.14, 3.14, format, arg), false}, + {must.NotEqual(ctx, "a", "b", format, arg), true}, + {must.NotEqual(ctx, "a", "a", format, arg), false}, + {must.NotEqual(ctx, hlc.Timestamp{Logical: 1}, hlc.Timestamp{Logical: 2}, format, arg), true}, + {must.NotEqual(ctx, hlc.Timestamp{Logical: 1}, hlc.Timestamp{Logical: 1}, format, arg), false}, + {must.NotEqual(ctx, p1, p2, format, arg), true}, + {must.NotEqual(ctx, p1, p1, format, arg), false}, + {must.NotEqual(ctx, pNil, p2, format, arg), true}, + {must.NotEqual(ctx, pNil, pNil, format, arg), false}, + }, + + "Greater": { + {must.Greater(ctx, 1, 0, format, arg), true}, + {must.Greater(ctx, 1, 1, format, arg), false}, + {must.Greater(ctx, 0, 1, format, arg), false}, + {must.Greater(ctx, 3.14, 2.717, format, arg), true}, + {must.Greater(ctx, 3.14, 3.14, format, arg), false}, + {must.Greater(ctx, 2.717, 3.14, format, arg), false}, + {must.Greater(ctx, "b", "a", format, arg), true}, + {must.Greater(ctx, "aa", "a", format, arg), true}, + {must.Greater(ctx, "b", "b", format, arg), false}, + {must.Greater(ctx, "a", "b", format, arg), false}, + }, + + "GreaterOrEqual": { + {must.GreaterOrEqual(ctx, 1, 0, format, arg), true}, + {must.GreaterOrEqual(ctx, 1, 1, format, arg), true}, + {must.GreaterOrEqual(ctx, 0, 1, format, arg), false}, + {must.GreaterOrEqual(ctx, 3.14, 2.717, format, arg), true}, + {must.GreaterOrEqual(ctx, 3.14, 3.14, format, arg), true}, + {must.GreaterOrEqual(ctx, 2.717, 3.14, format, arg), false}, + {must.GreaterOrEqual(ctx, "b", "a", format, arg), true}, + {must.GreaterOrEqual(ctx, "aa", "a", format, arg), true}, + {must.GreaterOrEqual(ctx, "b", "b", format, arg), true}, + {must.GreaterOrEqual(ctx, "a", "b", format, arg), false}, + }, + + "Less": { + {must.Less(ctx, 0, 1, format, arg), true}, + {must.Less(ctx, 1, 1, format, arg), false}, + {must.Less(ctx, 1, 0, format, arg), false}, + {must.Less(ctx, 2.717, 3.14, format, arg), true}, + {must.Less(ctx, 3.14, 3.14, format, arg), false}, + {must.Less(ctx, 3.14, 2.717, format, arg), false}, + {must.Less(ctx, "a", "b", format, arg), true}, + {must.Less(ctx, "a", "aa", format, arg), true}, + {must.Less(ctx, "b", "b", format, arg), false}, + {must.Less(ctx, "b", "a", format, arg), false}, + }, + + "LessOrEqual": { + {must.LessOrEqual(ctx, 0, 1, format, arg), true}, + {must.LessOrEqual(ctx, 1, 1, format, arg), true}, + {must.LessOrEqual(ctx, 1, 0, format, arg), false}, + {must.LessOrEqual(ctx, 2.717, 3.14, format, arg), true}, + {must.LessOrEqual(ctx, 3.14, 3.14, format, arg), true}, + {must.LessOrEqual(ctx, 3.14, 2.717, format, arg), false}, + {must.LessOrEqual(ctx, "a", "b", format, arg), true}, + {must.LessOrEqual(ctx, "a", "aa", format, arg), true}, + {must.LessOrEqual(ctx, "b", "b", format, arg), true}, + {must.LessOrEqual(ctx, "b", "a", format, arg), false}, + }, + + "EqualBytes": { + {must.EqualBytes(ctx, []byte(nil), nil, format, arg), true}, + {must.EqualBytes(ctx, []byte("foo"), nil, format, arg), false}, + {must.EqualBytes(ctx, []byte("foo"), []byte("foo"), format, arg), true}, + {must.EqualBytes(ctx, []byte("foo"), []byte("bar"), format, arg), false}, + {must.EqualBytes(ctx, []byte{1, 2, 3}, []byte{4, 5, 6}, format, arg), false}, + {must.EqualBytes(ctx, "", "", format, arg), true}, + {must.EqualBytes(ctx, "foo", "", format, arg), false}, + {must.EqualBytes(ctx, "foo", "foo", format, arg), true}, + {must.EqualBytes(ctx, "foo", "bar", format, arg), false}, + {must.EqualBytes(ctx, keys.NodeLivenessKey(1), keys.NodeLivenessKey(1), format, arg), true}, + {must.EqualBytes(ctx, keys.NodeLivenessKey(1), keys.NodeLivenessKey(2), format, arg), false}, + }, + + "NotEqualBytes": { + {must.NotEqualBytes(ctx, []byte(nil), nil, format, arg), false}, + {must.NotEqualBytes(ctx, []byte("foo"), nil, format, arg), true}, + {must.NotEqualBytes(ctx, []byte("foo"), []byte("foo"), format, arg), false}, + {must.NotEqualBytes(ctx, []byte("foo"), []byte("bar"), format, arg), true}, + {must.NotEqualBytes(ctx, []byte{1, 2, 3}, []byte{4, 5, 6}, format, arg), true}, + {must.NotEqualBytes(ctx, "", "", format, arg), false}, + {must.NotEqualBytes(ctx, "foo", "", format, arg), true}, + {must.NotEqualBytes(ctx, "foo", "foo", format, arg), false}, + {must.NotEqualBytes(ctx, "foo", "bar", format, arg), true}, + {must.NotEqualBytes(ctx, keys.NodeLivenessKey(1), keys.NodeLivenessKey(1), format, arg), false}, + {must.NotEqualBytes(ctx, keys.NodeLivenessKey(1), keys.NodeLivenessKey(2), format, arg), true}, + }, + + "PrefixBytes": { + {must.PrefixBytes(ctx, []byte(nil), nil, format, arg), true}, + {must.PrefixBytes(ctx, []byte{}, nil, format, arg), true}, + {must.PrefixBytes(ctx, []byte("foo"), nil, format, arg), true}, + {must.PrefixBytes(ctx, []byte("foo"), []byte{}, format, arg), true}, + {must.PrefixBytes(ctx, []byte("foo"), []byte("f"), format, arg), true}, + {must.PrefixBytes(ctx, []byte("foo"), []byte("foo"), format, arg), true}, + {must.PrefixBytes(ctx, []byte("foo"), []byte("foobar"), format, arg), false}, + {must.PrefixBytes(ctx, []byte("foo"), []byte("bar"), format, arg), false}, + {must.PrefixBytes(ctx, []byte(""), []byte("bar"), format, arg), false}, + {must.PrefixBytes(ctx, []byte{1, 2, 3}, []byte{7, 8, 9}, format, arg), false}, + {must.PrefixBytes(ctx, "foo", "f", format, arg), true}, + {must.PrefixBytes(ctx, "foo", "foo", format, arg), true}, + {must.PrefixBytes(ctx, "foo", "bar", format, arg), false}, + {must.PrefixBytes(ctx, keys.NodeLivenessKey(1), keys.NodeLivenessPrefix, format, arg), true}, + {must.PrefixBytes(ctx, keys.NodeLivenessKey(1), keys.LocalPrefix, format, arg), false}, + }, + + "NotPrefixBytes": { + {must.NotPrefixBytes(ctx, []byte(nil), nil, format, arg), false}, + {must.NotPrefixBytes(ctx, []byte{}, nil, format, arg), false}, + {must.NotPrefixBytes(ctx, []byte("foo"), nil, format, arg), false}, + {must.NotPrefixBytes(ctx, []byte("foo"), []byte{}, format, arg), false}, + {must.NotPrefixBytes(ctx, []byte("foo"), []byte("f"), format, arg), false}, + {must.NotPrefixBytes(ctx, []byte("foo"), []byte("foo"), format, arg), false}, + {must.NotPrefixBytes(ctx, []byte("foo"), []byte("foobar"), format, arg), true}, + {must.NotPrefixBytes(ctx, []byte("foo"), []byte("bar"), format, arg), true}, + {must.NotPrefixBytes(ctx, []byte(""), []byte("bar"), format, arg), true}, + {must.NotPrefixBytes(ctx, []byte{1, 2, 3}, []byte{1, 2, 3}, format, arg), false}, + {must.NotPrefixBytes(ctx, "foo", "f", format, arg), false}, + {must.NotPrefixBytes(ctx, "foo", "foo", format, arg), false}, + {must.NotPrefixBytes(ctx, "foo", "bar", format, arg), true}, + {must.NotPrefixBytes(ctx, keys.NodeLivenessKey(1), keys.NodeLivenessPrefix, format, arg), false}, + {must.NotPrefixBytes(ctx, keys.NodeLivenessKey(1), keys.LocalPrefix, format, arg), true}, + }, + + "Error": { + {must.Error(ctx, errors.New("error"), format, arg), true}, + {must.Error(ctx, nil, format, arg), false}, + }, + + "NoError": { + {must.NoError(ctx, nil, format, arg), true}, + {must.NoError(ctx, errors.New("boom"), format, arg), false}, + }, + + "Len": { + {must.Len(ctx, []int{1, 2, 3}, 3, format, arg), true}, + {must.Len(ctx, []int(nil), 0, format, arg), true}, + {must.Len(ctx, []int{1, 2, 3}, 2, format, arg), false}, + }, + + "Contains": { + {must.Contains(ctx, []int{1, 2, 3}, 1, format, arg), true}, + {must.Contains(ctx, []int{1, 2, 3}, 0, format, arg), false}, + {must.Contains(ctx, []int(nil), 1, format, arg), false}, + {must.Contains(ctx, []int{}, 1, format, arg), false}, + {must.Contains(ctx, []string{"foo", "bar"}, "baz", format, arg), false}, + }, + + "NotContains": { + {must.NotContains(ctx, []int{1, 2, 3}, 1, format, arg), false}, + {must.NotContains(ctx, []int{1, 2, 3}, 0, format, arg), true}, + {must.NotContains(ctx, []int(nil), 1, format, arg), true}, + {must.NotContains(ctx, []int{}, 1, format, arg), true}, + {must.NotContains(ctx, []string{"foo", "bar"}, "bar", format, arg), false}, + }, + + "Empty": { + {must.Empty(ctx, []int{}, format, arg), true}, + {must.Empty(ctx, []int(nil), format, arg), true}, + {must.Empty(ctx, []int{1, 2, 3}, format, arg), false}, + }, + + "NotEmpty": { + {must.NotEmpty(ctx, []int{1, 2, 3}, format, arg), true}, + {must.NotEmpty(ctx, []int{}, format, arg), false}, + {must.NotEmpty(ctx, []int(nil), format, arg), false}, + }, + + "Nil": { + {must.Nil(ctx, (*hlc.Timestamp)(nil), format, arg), true}, + {must.Nil(ctx, &hlc.Timestamp{}, format, arg), false}, + }, + + "NotNil": { + {must.NotNil(ctx, &hlc.Timestamp{}, format, arg), true}, + {must.NotNil(ctx, (*hlc.Timestamp)(nil), format, arg), false}, + }, + + "Zero": { + {must.Zero(ctx, false, format, arg), true}, + {must.Zero(ctx, true, format, arg), false}, + {must.Zero(ctx, 0, format, arg), true}, + {must.Zero(ctx, 1, format, arg), false}, + {must.Zero(ctx, 0.0, format, arg), true}, + {must.Zero(ctx, 0.1, format, arg), false}, + {must.Zero(ctx, "", format, arg), true}, + {must.Zero(ctx, "a", format, arg), false}, + {must.Zero(ctx, hlc.Timestamp{}, format, arg), true}, + {must.Zero(ctx, hlc.Timestamp{Logical: 1}, format, arg), false}, + {must.Zero(ctx, (*hlc.Timestamp)(nil), format, arg), true}, + {must.Zero(ctx, &hlc.Timestamp{}, format, arg), false}, + }, + + "NotZero": { + {must.NotZero(ctx, true, format, arg), true}, + {must.NotZero(ctx, false, format, arg), false}, + {must.NotZero(ctx, 1, format, arg), true}, + {must.NotZero(ctx, 0, format, arg), false}, + {must.NotZero(ctx, 0.1, format, arg), true}, + {must.NotZero(ctx, 0.0, format, arg), false}, + {must.NotZero(ctx, "a", format, arg), true}, + {must.NotZero(ctx, "", format, arg), false}, + {must.NotZero(ctx, hlc.Timestamp{Logical: 1}, format, arg), true}, + {must.NotZero(ctx, hlc.Timestamp{}, format, arg), false}, + {must.NotZero(ctx, &hlc.Timestamp{}, format, arg), true}, + {must.NotZero(ctx, (*hlc.Timestamp)(nil), format, arg), false}, + }, + + "Same": { + {must.Same(ctx, p1, p1, format, arg), true}, + {must.Same(ctx, pNil, pNil, format, arg), true}, + {must.Same(ctx, p1, p2, format, arg), false}, + {must.Same(ctx, p1, pNil, format, arg), false}, + }, + + "NotSame": { + {must.NotSame(ctx, p1, p2, format, arg), true}, + {must.NotSame(ctx, p1, pNil, format, arg), true}, + {must.NotSame(ctx, p1, p1, format, arg), false}, + {must.NotSame(ctx, pNil, pNil, format, arg), false}, + }, + } + + w := echotest.NewWalker(t, datapathutils.TestDataPath(t, t.Name())) + for name, tcs := range testcases { + t.Run(name, w.Run(t, name, func(t *testing.T) string { + var output string + for _, tc := range tcs { + t.Run("", func(t *testing.T) { + if tc.expectOK { + require.NoError(t, tc.err) + return + } + + require.Error(t, tc.err) + require.True(t, errors.HasAssertionFailure(tc.err)) + + // Format args are included but properly redacted. + require.Contains(t, redact.Sprint(tc.err), redact.Sprintf(format, arg)) + + // The error includes a stack trace, but strips any frames from the must package. + require.Contains(t, fmt.Sprintf("%+v", tc.err), ".TestAssertions") + require.NotContains(t, fmt.Sprintf("%+v", tc.err), "must.") + + // Output and check error output. + errString := string(redact.Sprint(tc.err)) + + // Replace any hex memory addresses with 0xf00, since they'll change + // for each run. + reHex := regexp.MustCompile(`0x[0-9a-f]{8,}`) + errString = reHex.ReplaceAllString(errString, `0xf00`) + + output += errString + "\n" + }) + } + return output + })) + } +} + +// TestPanicOn tests that PanicOn panics when expected. +func TestPanicOn(t *testing.T) { + defer leaktest.AfterTest(t)() + defer noopFail()() + + ctx := context.Background() + + must.PanicOn(must.True(ctx, true, "message")) // success is noop + + require.Panics(t, func() { + must.PanicOn(must.True(ctx, false, "message")) + }) +} + +// TestFatalOn tests that FatalOn doesn't fatal on success. We can't easily test +// that it fatals. +func TestFatalOn(t *testing.T) { + defer leaktest.AfterTest(t)() + defer noopFail()() + + ctx := context.Background() + + must.FatalOn(ctx, must.True(ctx, true, "message")) // success is noop + + // We don't test failures, because it fatals. +} diff --git a/pkg/util/must/testdata/TestAssertions/Contains b/pkg/util/must/testdata/TestAssertions/Contains new file mode 100644 index 000000000000..03c71fb21b6c --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/Contains @@ -0,0 +1,6 @@ +echo +---- +message with ‹argument›: 0 not in [1 2 3] +message with ‹argument›: 1 not in [] +message with ‹argument›: 1 not in [] +message with ‹argument›: ‹baz› not in [‹foo› ‹bar›] diff --git a/pkg/util/must/testdata/TestAssertions/Empty b/pkg/util/must/testdata/TestAssertions/Empty new file mode 100644 index 000000000000..f1b6da7927fb --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/Empty @@ -0,0 +1,3 @@ +echo +---- +message with ‹argument›: length 3 != 0 diff --git a/pkg/util/must/testdata/TestAssertions/Equal b/pkg/util/must/testdata/TestAssertions/Equal new file mode 100644 index 000000000000..75fe4c03079d --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/Equal @@ -0,0 +1,9 @@ +echo +---- +message with ‹argument›: true != false +message with ‹argument›: 0 != 1 +message with ‹argument›: 3.14 != 2.717 +message with ‹argument›: ‹"a"› != ‹"b"› +message with ‹argument›: hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false} != hlc.Timestamp{WallTime:0, Logical:2, Synthetic:false} +message with ‹argument›: &hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false} != &hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false} +message with ‹argument›: (*hlc.Timestamp)(nil) != &hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false} diff --git a/pkg/util/must/testdata/TestAssertions/EqualBytes b/pkg/util/must/testdata/TestAssertions/EqualBytes new file mode 100644 index 000000000000..6f59db439ca5 --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/EqualBytes @@ -0,0 +1,8 @@ +echo +---- +message with ‹argument›: ‹"foo"› != ‹""› +message with ‹argument›: ‹"foo"› != ‹"bar"› +message with ‹argument›: ‹"\x01\x02\x03"› != ‹"\x04\x05\x06"› +message with ‹argument›: ‹"foo"› != ‹""› +message with ‹argument›: ‹"foo"› != ‹"bar"› +message with ‹argument›: /System/NodeLiveness/1 != /System/NodeLiveness/2 diff --git a/pkg/util/must/testdata/TestAssertions/Error b/pkg/util/must/testdata/TestAssertions/Error new file mode 100644 index 000000000000..93ff2f45b67c --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/Error @@ -0,0 +1,3 @@ +echo +---- +message with ‹argument›: expected error, got nil diff --git a/pkg/util/must/testdata/TestAssertions/False b/pkg/util/must/testdata/TestAssertions/False new file mode 100644 index 000000000000..67378b5b5aba --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/False @@ -0,0 +1,3 @@ +echo +---- +message with ‹argument› diff --git a/pkg/util/must/testdata/TestAssertions/Greater b/pkg/util/must/testdata/TestAssertions/Greater new file mode 100644 index 000000000000..488a334fedda --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/Greater @@ -0,0 +1,8 @@ +echo +---- +message with ‹argument›: 1 <= 1 +message with ‹argument›: 0 <= 1 +message with ‹argument›: 3.14 <= 3.14 +message with ‹argument›: 2.717 <= 3.14 +message with ‹argument›: ‹b› <= ‹b› +message with ‹argument›: ‹a› <= ‹b› diff --git a/pkg/util/must/testdata/TestAssertions/GreaterOrEqual b/pkg/util/must/testdata/TestAssertions/GreaterOrEqual new file mode 100644 index 000000000000..156a39d3bb8f --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/GreaterOrEqual @@ -0,0 +1,5 @@ +echo +---- +message with ‹argument›: 0 < 1 +message with ‹argument›: 2.717 < 3.14 +message with ‹argument›: ‹a› < ‹b› diff --git a/pkg/util/must/testdata/TestAssertions/Len b/pkg/util/must/testdata/TestAssertions/Len new file mode 100644 index 000000000000..1fc2ee9a96bb --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/Len @@ -0,0 +1,3 @@ +echo +---- +message with ‹argument›: length 3 != 2 diff --git a/pkg/util/must/testdata/TestAssertions/Less b/pkg/util/must/testdata/TestAssertions/Less new file mode 100644 index 000000000000..d9c60f2e0597 --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/Less @@ -0,0 +1,8 @@ +echo +---- +message with ‹argument›: 1 >= 1 +message with ‹argument›: 1 >= 0 +message with ‹argument›: 3.14 >= 3.14 +message with ‹argument›: 3.14 >= 2.717 +message with ‹argument›: ‹b› >= ‹b› +message with ‹argument›: ‹b› >= ‹a› diff --git a/pkg/util/must/testdata/TestAssertions/LessOrEqual b/pkg/util/must/testdata/TestAssertions/LessOrEqual new file mode 100644 index 000000000000..f19f63547bca --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/LessOrEqual @@ -0,0 +1,5 @@ +echo +---- +message with ‹argument›: 1 > 0 +message with ‹argument›: 3.14 > 2.717 +message with ‹argument›: ‹b› > ‹a› diff --git a/pkg/util/must/testdata/TestAssertions/Nil b/pkg/util/must/testdata/TestAssertions/Nil new file mode 100644 index 000000000000..c2ee7fc8709a --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/Nil @@ -0,0 +1,3 @@ +echo +---- +message with ‹argument›: expected nil, got &hlc.Timestamp{WallTime:0, Logical:0, Synthetic:false} diff --git a/pkg/util/must/testdata/TestAssertions/NoError b/pkg/util/must/testdata/TestAssertions/NoError new file mode 100644 index 000000000000..4a3a877994f4 --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/NoError @@ -0,0 +1,3 @@ +echo +---- +message with ‹argument›: error boom diff --git a/pkg/util/must/testdata/TestAssertions/NotContains b/pkg/util/must/testdata/TestAssertions/NotContains new file mode 100644 index 000000000000..7d4ab98a53e5 --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/NotContains @@ -0,0 +1,4 @@ +echo +---- +message with ‹argument›: 1 is in [1 2 3] +message with ‹argument›: ‹bar› is in [‹foo› ‹bar›] diff --git a/pkg/util/must/testdata/TestAssertions/NotEmpty b/pkg/util/must/testdata/TestAssertions/NotEmpty new file mode 100644 index 000000000000..1092d7f95fab --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/NotEmpty @@ -0,0 +1,4 @@ +echo +---- +message with ‹argument›: is empty +message with ‹argument›: is empty diff --git a/pkg/util/must/testdata/TestAssertions/NotEqual b/pkg/util/must/testdata/TestAssertions/NotEqual new file mode 100644 index 000000000000..33570a617c23 --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/NotEqual @@ -0,0 +1,9 @@ +echo +---- +message with ‹argument›: true == true +message with ‹argument›: 1 == 1 +message with ‹argument›: 3.14 == 3.14 +message with ‹argument›: ‹"a"› == ‹"a"› +message with ‹argument›: hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false} == hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false} +message with ‹argument›: &hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false} == &hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false} +message with ‹argument›: (*hlc.Timestamp)(nil) == (*hlc.Timestamp)(nil) diff --git a/pkg/util/must/testdata/TestAssertions/NotEqualBytes b/pkg/util/must/testdata/TestAssertions/NotEqualBytes new file mode 100644 index 000000000000..86506c4eaeb1 --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/NotEqualBytes @@ -0,0 +1,7 @@ +echo +---- +message with ‹argument›: ‹""› == ‹""› +message with ‹argument›: ‹"foo"› == ‹"foo"› +message with ‹argument›: ‹""› == ‹""› +message with ‹argument›: ‹"foo"› == ‹"foo"› +message with ‹argument›: /System/NodeLiveness/1 == /System/NodeLiveness/1 diff --git a/pkg/util/must/testdata/TestAssertions/NotNil b/pkg/util/must/testdata/TestAssertions/NotNil new file mode 100644 index 000000000000..6dc6ea47f1e3 --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/NotNil @@ -0,0 +1,3 @@ +echo +---- +message with ‹argument›: value is nil diff --git a/pkg/util/must/testdata/TestAssertions/NotPrefixBytes b/pkg/util/must/testdata/TestAssertions/NotPrefixBytes new file mode 100644 index 000000000000..7c9377715c47 --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/NotPrefixBytes @@ -0,0 +1,12 @@ +echo +---- +message with ‹argument›: ‹""› has prefix ‹""› +message with ‹argument›: ‹""› has prefix ‹""› +message with ‹argument›: ‹"foo"› has prefix ‹""› +message with ‹argument›: ‹"foo"› has prefix ‹""› +message with ‹argument›: ‹"foo"› has prefix ‹"f"› +message with ‹argument›: ‹"foo"› has prefix ‹"foo"› +message with ‹argument›: ‹"\x01\x02\x03"› has prefix ‹"\x01\x02\x03"› +message with ‹argument›: ‹"foo"› has prefix ‹"f"› +message with ‹argument›: ‹"foo"› has prefix ‹"foo"› +message with ‹argument›: /System/NodeLiveness/1 has prefix /System/NodeLiveness diff --git a/pkg/util/must/testdata/TestAssertions/NotSame b/pkg/util/must/testdata/TestAssertions/NotSame new file mode 100644 index 000000000000..9832a68b466b --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/NotSame @@ -0,0 +1,4 @@ +echo +---- +message with ‹argument›: 0xf00 == 0xf00, &hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false} +message with ‹argument›: 0x0 == 0x0, (*hlc.Timestamp)(nil) diff --git a/pkg/util/must/testdata/TestAssertions/NotZero b/pkg/util/must/testdata/TestAssertions/NotZero new file mode 100644 index 000000000000..1f5756524f77 --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/NotZero @@ -0,0 +1,8 @@ +echo +---- +message with ‹argument›: false is zero-valued +message with ‹argument›: 0 is zero-valued +message with ‹argument›: 0 is zero-valued +message with ‹argument›: ‹""› is zero-valued +message with ‹argument›: hlc.Timestamp{WallTime:0, Logical:0, Synthetic:false} is zero-valued +message with ‹argument›: (*hlc.Timestamp)(nil) is zero-valued diff --git a/pkg/util/must/testdata/TestAssertions/PrefixBytes b/pkg/util/must/testdata/TestAssertions/PrefixBytes new file mode 100644 index 000000000000..9374b559b81a --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/PrefixBytes @@ -0,0 +1,8 @@ +echo +---- +message with ‹argument›: ‹"foo"› does not have prefix ‹"foobar"› +message with ‹argument›: ‹"foo"› does not have prefix ‹"bar"› +message with ‹argument›: ‹""› does not have prefix ‹"bar"› +message with ‹argument›: ‹"\x01\x02\x03"› does not have prefix ‹"\a\b\t"› +message with ‹argument›: ‹"foo"› does not have prefix ‹"bar"› +message with ‹argument›: /System/NodeLiveness/1 does not have prefix /Local‹/›‹"›‹"› diff --git a/pkg/util/must/testdata/TestAssertions/Same b/pkg/util/must/testdata/TestAssertions/Same new file mode 100644 index 000000000000..4a5f61e88fdf --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/Same @@ -0,0 +1,8 @@ +echo +---- +message with ‹argument›: 0xf00 != 0xf00 +0xf00 is &hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false} +0xf00 is &hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false}' +message with ‹argument›: 0xf00 != 0x0 +0xf00 is &hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false} +0x0 is (*hlc.Timestamp)(nil)' diff --git a/pkg/util/must/testdata/TestAssertions/True b/pkg/util/must/testdata/TestAssertions/True new file mode 100644 index 000000000000..67378b5b5aba --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/True @@ -0,0 +1,3 @@ +echo +---- +message with ‹argument› diff --git a/pkg/util/must/testdata/TestAssertions/Zero b/pkg/util/must/testdata/TestAssertions/Zero new file mode 100644 index 000000000000..463af8003b8f --- /dev/null +++ b/pkg/util/must/testdata/TestAssertions/Zero @@ -0,0 +1,8 @@ +echo +---- +message with ‹argument›: expected zero value, got true +message with ‹argument›: expected zero value, got 1 +message with ‹argument›: expected zero value, got 0.1 +message with ‹argument›: expected zero value, got ‹"a"› +message with ‹argument›: expected zero value, got hlc.Timestamp{WallTime:0, Logical:1, Synthetic:false} +message with ‹argument›: expected zero value, got &hlc.Timestamp{WallTime:0, Logical:0, Synthetic:false} From 0a1699db3988998abf27159e887156083588dd5d Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Sat, 22 Jul 2023 20:21:51 +0000 Subject: [PATCH 11/14] *: add a few example usages of `must` Epic: none Release note: None --- pkg/kv/kvserver/BUILD.bazel | 1 + pkg/kv/kvserver/batcheval/BUILD.bazel | 2 +- pkg/kv/kvserver/batcheval/cmd_clear_range.go | 90 ++++++++++---------- pkg/kv/kvserver/batcheval/cmd_export.go | 20 ++--- pkg/kv/kvserver/batcheval/cmd_migrate.go | 6 +- pkg/kv/kvserver/scheduler.go | 10 +-- pkg/roachpb/BUILD.bazel | 1 + pkg/roachpb/data.go | 10 +-- 8 files changed, 63 insertions(+), 77 deletions(-) diff --git a/pkg/kv/kvserver/BUILD.bazel b/pkg/kv/kvserver/BUILD.bazel index ded9ad9e4212..b3dfc8a2ef7e 100644 --- a/pkg/kv/kvserver/BUILD.bazel +++ b/pkg/kv/kvserver/BUILD.bazel @@ -206,6 +206,7 @@ go_library( "//pkg/util/metric", "//pkg/util/metric/aggmetric", "//pkg/util/mon", + "//pkg/util/must", "//pkg/util/pprofutil", "//pkg/util/protoutil", "//pkg/util/quotapool", diff --git a/pkg/kv/kvserver/batcheval/BUILD.bazel b/pkg/kv/kvserver/batcheval/BUILD.bazel index 5a38c4ec1998..54b9bc85b8f9 100644 --- a/pkg/kv/kvserver/batcheval/BUILD.bazel +++ b/pkg/kv/kvserver/batcheval/BUILD.bazel @@ -55,7 +55,6 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval", visibility = ["//visibility:public"], deps = [ - "//pkg/build", "//pkg/clusterversion", "//pkg/keys", "//pkg/kv/kvpb", @@ -85,6 +84,7 @@ go_library( "//pkg/util/limit", "//pkg/util/log", "//pkg/util/mon", + "//pkg/util/must", "//pkg/util/protoutil", "//pkg/util/tracing", "//pkg/util/uuid", diff --git a/pkg/kv/kvserver/batcheval/cmd_clear_range.go b/pkg/kv/kvserver/batcheval/cmd_clear_range.go index dfdbdcb0a76f..3b7aac49b16d 100644 --- a/pkg/kv/kvserver/batcheval/cmd_clear_range.go +++ b/pkg/kv/kvserver/batcheval/cmd_clear_range.go @@ -23,10 +23,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" - "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/must" "github.com/cockroachdb/errors" - "github.com/kr/pretty" ) // ClearRangeBytesThreshold is the threshold over which the ClearRange @@ -174,63 +173,60 @@ func computeStatsDelta( // We can avoid manually computing the stats delta if we're clearing // the entire range. - entireRange := desc.StartKey.Equal(from) && desc.EndKey.Equal(to) - if entireRange { + if desc.StartKey.Equal(from) && desc.EndKey.Equal(to) { // Note this it is safe to use the full range MVCC stats, as - // opposed to the usual method of computing only a localizied + // opposed to the usual method of computing only a localized // stats delta, because a full-range clear prevents any concurrent // access to the stats. Concurrent changes to range-local keys are // explicitly ignored (i.e. SysCount, SysBytes). delta = cArgs.EvalCtx.GetMVCCStats() delta.SysCount, delta.SysBytes, delta.AbortSpanBytes = 0, 0, 0 // no change to system stats - } - // If we can't use the fast stats path, or race test is enabled, compute stats - // across the key span to be cleared. - if !entireRange || util.RaceEnabled { - computed, err := storage.ComputeStats(readWriter, from, to, delta.LastUpdateNanos) + // Assert correct stats. + if err := must.Expensive(func() error { + if delta.ContainsEstimates != 0 { + return nil + } + computed, err := storage.ComputeStats(readWriter, from, to, delta.LastUpdateNanos) + if err != nil { + return err + } + return must.Equal(ctx, delta, computed, "range MVCC stats differ from computed") + }); err != nil { + return enginepb.MVCCStats{}, err + } + + } else { + // If we can't use the fast path, compute stats across the cleared span. + var err error + delta, err = storage.ComputeStats(readWriter, from, to, delta.LastUpdateNanos) if err != nil { return enginepb.MVCCStats{}, err } - // If we took the fast path but race is enabled, assert stats were correctly - // computed. - if entireRange { - // Retain the value of ContainsEstimates for tests under race. - computed.ContainsEstimates = delta.ContainsEstimates - // We only want to assert the correctness of stats that do not contain - // estimates. - if delta.ContainsEstimates == 0 && !delta.Equal(computed) { - log.Fatalf(ctx, "fast-path MVCCStats computation gave wrong result: diff(fast, computed) = %s", - pretty.Diff(delta, computed)) - } + + // We need to adjust for the fragmentation of any MVCC range tombstones that + // straddle the span bounds. The clearing of the inner fragments has already + // been accounted for above. We take care not to peek outside the Raft range + // bounds. + leftPeekBound, rightPeekBound := rangeTombstonePeekBounds( + from, to, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey()) + rkIter := readWriter.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + KeyTypes: storage.IterKeyTypeRangesOnly, + LowerBound: leftPeekBound, + UpperBound: rightPeekBound, + }) + defer rkIter.Close() + + if cmp, lhs, err := storage.PeekRangeKeysLeft(rkIter, from); err != nil { + return enginepb.MVCCStats{}, err + } else if cmp > 0 { + delta.Subtract(storage.UpdateStatsOnRangeKeySplit(from, lhs.Versions)) } - delta = computed - - // If we're not clearing the entire range, we need to adjust for the - // fragmentation of any MVCC range tombstones that straddle the span bounds. - // The clearing of the inner fragments has already been accounted for above. - // We take care not to peek outside the Raft range bounds. - if !entireRange { - leftPeekBound, rightPeekBound := rangeTombstonePeekBounds( - from, to, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey()) - rkIter := readWriter.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ - KeyTypes: storage.IterKeyTypeRangesOnly, - LowerBound: leftPeekBound, - UpperBound: rightPeekBound, - }) - defer rkIter.Close() - - if cmp, lhs, err := storage.PeekRangeKeysLeft(rkIter, from); err != nil { - return enginepb.MVCCStats{}, err - } else if cmp > 0 { - delta.Subtract(storage.UpdateStatsOnRangeKeySplit(from, lhs.Versions)) - } - if cmp, rhs, err := storage.PeekRangeKeysRight(rkIter, to); err != nil { - return enginepb.MVCCStats{}, err - } else if cmp < 0 { - delta.Subtract(storage.UpdateStatsOnRangeKeySplit(to, rhs.Versions)) - } + if cmp, rhs, err := storage.PeekRangeKeysRight(rkIter, to); err != nil { + return enginepb.MVCCStats{}, err + } else if cmp < 0 { + delta.Subtract(storage.UpdateStatsOnRangeKeySplit(to, rhs.Versions)) } } diff --git a/pkg/kv/kvserver/batcheval/cmd_export.go b/pkg/kv/kvserver/batcheval/cmd_export.go index fd4e245a92f9..a2c7be6603bb 100644 --- a/pkg/kv/kvserver/batcheval/cmd_export.go +++ b/pkg/kv/kvserver/batcheval/cmd_export.go @@ -16,7 +16,6 @@ import ( "fmt" "time" - "github.com/cockroachdb/cockroach/pkg/build" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval/result" @@ -27,6 +26,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/must" "github.com/cockroachdb/cockroach/pkg/util/tracing" "github.com/cockroachdb/errors" ) @@ -274,19 +274,11 @@ func evalExport( reply.ResumeReason = kvpb.RESUME_ELASTIC_CPU_LIMIT break } else { - if !resumeInfo.CPUOverlimit { - // We should never come here. There should be no condition aside from - // resource constraints that results in an early exit without - // exporting any data. Regardless, if we have a resumeKey we - // immediately retry the ExportRequest from that key and timestamp - // onwards. - if !build.IsRelease() { - return result.Result{}, errors.AssertionFailedf("ExportRequest exited without " + - "exporting any data for an unknown reason; programming error") - } else { - log.Warningf(ctx, "unexpected resume span from ExportRequest without exporting any data for an unknown reason: %v", resumeInfo) - } - } + // There should be no condition aside from resource constraints that + // results in an early exit without exporting any data. Regardless, if + // we have a resumeKey we immediately retry the ExportRequest from + // that key and timestamp onwards. + _ = must.True(ctx, resumeInfo.CPUOverlimit, "Export returned no data: %+v", resumeInfo) start = resumeInfo.ResumeKey.Key resumeKeyTS = resumeInfo.ResumeKey.Timestamp continue diff --git a/pkg/kv/kvserver/batcheval/cmd_migrate.go b/pkg/kv/kvserver/batcheval/cmd_migrate.go index a0fc7d03ab59..c2f5f89a8abd 100644 --- a/pkg/kv/kvserver/batcheval/cmd_migrate.go +++ b/pkg/kv/kvserver/batcheval/cmd_migrate.go @@ -22,7 +22,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage" - "github.com/cockroachdb/errors" + "github.com/cockroachdb/cockroach/pkg/util/must" ) func init() { @@ -63,8 +63,8 @@ func Migrate( migrationVersion := args.Version fn, ok := migrationRegistry[migrationVersion] - if !ok { - return result.Result{}, errors.AssertionFailedf("migration for %s not found", migrationVersion) + if err := must.True(ctx, ok, "migration for %s not found", migrationVersion); err != nil { + return result.Result{}, err } pd, err := fn(ctx, readWriter, cArgs) if err != nil { diff --git a/pkg/kv/kvserver/scheduler.go b/pkg/kv/kvserver/scheduler.go index 06bc40b91d2d..ab60f8e980ab 100644 --- a/pkg/kv/kvserver/scheduler.go +++ b/pkg/kv/kvserver/scheduler.go @@ -18,8 +18,8 @@ import ( "unsafe" "github.com/cockroachdb/cockroach/pkg/roachpb" - "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/must" "github.com/cockroachdb/cockroach/pkg/util/stop" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/timeutil" @@ -400,11 +400,9 @@ func (ss *raftSchedulerShard) worker( state.flags |= stateRaftReady } } - if util.RaceEnabled { // assert the ticks invariant - if tick := state.flags&stateRaftTick != 0; tick != (state.ticks != 0) { - log.Fatalf(ctx, "stateRaftTick is %v with ticks %v", tick, state.ticks) - } - } + + _ = must.Equal(ctx, state.flags&stateRaftTick != 0, state.ticks != 0, + "flags %d with %d ticks", state.flags, state.ticks) // safe to continue if state.flags&stateRaftTick != 0 { for t := state.ticks; t > 0; t-- { // processRaftTick returns true if the range should perform ready diff --git a/pkg/roachpb/BUILD.bazel b/pkg/roachpb/BUILD.bazel index d8240ed0d63f..7a07e2462e02 100644 --- a/pkg/roachpb/BUILD.bazel +++ b/pkg/roachpb/BUILD.bazel @@ -38,6 +38,7 @@ go_library( "//pkg/util/humanizeutil", "//pkg/util/interval", "//pkg/util/log", + "//pkg/util/must", "//pkg/util/protoutil", "//pkg/util/syncutil", "//pkg/util/timetz", diff --git a/pkg/roachpb/data.go b/pkg/roachpb/data.go index a264fa07f7cd..b9bbce342039 100644 --- a/pkg/roachpb/data.go +++ b/pkg/roachpb/data.go @@ -32,13 +32,13 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" - "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/bitarray" "github.com/cockroachdb/cockroach/pkg/util/duration" "github.com/cockroachdb/cockroach/pkg/util/encoding" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/interval" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/must" "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/cockroach/pkg/util/timetz" "github.com/cockroachdb/cockroach/pkg/util/uuid" @@ -2464,11 +2464,9 @@ var _ sort.Interface = SequencedWriteBySeq{} // Find searches for the index of the SequencedWrite with the provided // sequence number. Returns -1 if no corresponding write is found. func (s SequencedWriteBySeq) Find(seq enginepb.TxnSeq) int { - if util.RaceEnabled { - if !sort.IsSorted(s) { - panic("SequencedWriteBySeq must be sorted") - } - } + _ = must.Expensive(func() error { + return must.True(context.TODO(), sort.IsSorted(s), "SequencedWriteBySeq not sorted") + }) if i := sort.Search(len(s), func(i int) bool { return s[i].Sequence >= seq }); i < len(s) && s[i].Sequence == seq { From 7792db18cab520e2e57323a8e5b0fc3b4b03027f Mon Sep 17 00:00:00 2001 From: Ricky Stewart Date: Fri, 28 Jul 2023 10:48:59 -0500 Subject: [PATCH 12/14] teamcity-trigger: don't start a job for an empty target This makes no sense, so skip these cases. Closes: #107779 Closes: #107781 Epic: none Release note: None --- pkg/cmd/teamcity-trigger/main.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/cmd/teamcity-trigger/main.go b/pkg/cmd/teamcity-trigger/main.go index 9be9a5473050..4944020694a8 100644 --- a/pkg/cmd/teamcity-trigger/main.go +++ b/pkg/cmd/teamcity-trigger/main.go @@ -126,6 +126,9 @@ func runTC(queueBuild func(string, map[string]string)) { // Queue stress builds. One per configuration per test target. for _, testTarget := range strings.Split(string(targets), "\n") { testTarget = strings.TrimSpace(testTarget) + if testTarget == "" { + continue + } // By default, run each package for up to 100 iterations. maxRuns := 100 maxTime := getMaxTime(testTarget) From 207991c3d175bd2b38f0eda431e08131d5f3a92e Mon Sep 17 00:00:00 2001 From: Ricky Stewart Date: Fri, 28 Jul 2023 10:58:34 -0500 Subject: [PATCH 13/14] githubpost: set `map` field if `null` Go is a really good language. Informs: #107779 Epic: none Release note: None --- pkg/cmd/bazci/githubpost/githubpost.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/cmd/bazci/githubpost/githubpost.go b/pkg/cmd/bazci/githubpost/githubpost.go index 2903a7da0230..f431ff2e93c4 100644 --- a/pkg/cmd/bazci/githubpost/githubpost.go +++ b/pkg/cmd/bazci/githubpost/githubpost.go @@ -101,6 +101,9 @@ func getIssueFilerForFormatter(formatterName string) func(ctx context.Context, f return func(ctx context.Context, f failure) error { fmter, req := reqFromFailure(ctx, f) if stress := os.Getenv("COCKROACH_NIGHTLY_STRESS"); stress != "" { + if req.ExtraParams == nil { + req.ExtraParams = make(map[string]string) + } req.ExtraParams["stress"] = "true" } return issues.Post(ctx, log.Default(), fmter, req) From ce27952208fe7edaf95d209b30643b72b68f37bb Mon Sep 17 00:00:00 2001 From: Lidor Carmel Date: Tue, 18 Jul 2023 13:10:11 -0500 Subject: [PATCH 14/14] streamingest: unskip TestTenantStreamingUnavailableStreamAddress Changing a few things to get this test to pass under stress: - use 50 ranges instead of 10, because there are already 50-ish system ranges, so if we write only 10 more ranges those might not get distributed on all servers. - avoid reading from the source cluster after stopping a node, it's flaky, see #107499 for more info. Epic: none Fixes: #107023 Fixes: #106865 Release note: None --- .../replicationtestutils/testutils.go | 7 ++- .../replication_stream_e2e_test.go | 48 ++++++------------- 2 files changed, 20 insertions(+), 35 deletions(-) diff --git a/pkg/ccl/streamingccl/replicationtestutils/testutils.go b/pkg/ccl/streamingccl/replicationtestutils/testutils.go index 5a46080b9a45..df1e4355dddb 100644 --- a/pkg/ccl/streamingccl/replicationtestutils/testutils.go +++ b/pkg/ccl/streamingccl/replicationtestutils/testutils.go @@ -420,8 +420,11 @@ func requireReplicatedTime(targetTime hlc.Timestamp, progress *jobspb.Progress) } func CreateScatteredTable(t *testing.T, c *TenantStreamingClusters, numNodes int) { - // Create a source table with multiple ranges spread across multiple nodes - numRanges := 10 + // Create a source table with multiple ranges spread across multiple nodes. We + // need around 50 or more ranges because there are already over 50 system + // ranges, so if we write just a few ranges those might all be on a single + // server, which will cause the test to flake. + numRanges := 50 rowsPerRange := 20 c.SrcTenantSQL.Exec(t, "CREATE TABLE d.scattered (key INT PRIMARY KEY)") c.SrcTenantSQL.Exec(t, "INSERT INTO d.scattered (key) SELECT * FROM generate_series(1, $1)", diff --git a/pkg/ccl/streamingccl/streamingest/replication_stream_e2e_test.go b/pkg/ccl/streamingccl/streamingest/replication_stream_e2e_test.go index b9b55fa11fc0..b92047e86513 100644 --- a/pkg/ccl/streamingccl/streamingest/replication_stream_e2e_test.go +++ b/pkg/ccl/streamingccl/streamingest/replication_stream_e2e_test.go @@ -15,7 +15,6 @@ import ( "testing" "time" - "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/ccl/streamingccl/replicationtestutils" "github.com/cockroachdb/cockroach/pkg/ccl/streamingccl/replicationutils" _ "github.com/cockroachdb/cockroach/pkg/cloud/impl" @@ -460,11 +459,12 @@ func TestTenantStreamingDropTenantCancelsStream(t *testing.T) { }) } +// TestTenantStreamingUnavailableStreamAddress verifies that after a +// pause/resume (replan) we will not use a dead server as a source. func TestTenantStreamingUnavailableStreamAddress(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - skip.WithIssue(t, 106865) skip.UnderDeadlock(t, "multi-node may time out under deadlock") skip.UnderRace(t, "takes too long with multiple nodes") @@ -495,38 +495,23 @@ func TestTenantStreamingUnavailableStreamAddress(t *testing.T) { streamAddresses := progress.GetStreamIngest().StreamAddresses require.Greater(t, len(streamAddresses), 1) - destroyedAddress := c.SrcURL.String() + // Write something to the source cluster, note that the job is paused - and + // therefore not replicated for now. + c.SrcTenantSQL.Exec(t, "CREATE TABLE d.x (id INT PRIMARY KEY, n INT)") + c.SrcTenantSQL.Exec(t, `INSERT INTO d.x VALUES (3);`) + // Stop a server on the source cluster. Note that in this test we are trying + // to avoid using the source cluster after this point because if we do the + // test flakes, see #107499 for more info. + destroyedAddress := c.SrcURL.String() require.NoError(t, c.SrcTenantConn.Close()) c.SrcTenantServer.Stopper().Stop(ctx) c.SrcCluster.StopServer(0) - // Once SrcCluster.Server(0) is shut down queries must be ran against a different server - alternateSrcSysSQL := sqlutils.MakeSQLRunner(c.SrcCluster.ServerConn(1)) - _, alternateSrcTenantConn := serverutils.StartTenant(t, c.SrcCluster.Server(1), - base.TestTenantArgs{ - TenantID: c.Args.SrcTenantID, - TenantName: c.Args.SrcTenantName, - DisableCreateTenant: true, - }) - defer alternateSrcTenantConn.Close() - alternateSrcTenantSQL := sqlutils.MakeSQLRunner(alternateSrcTenantConn) - - alternateCompareResult := func(query string) { - sourceData := alternateSrcTenantSQL.QueryStr(c.T, query) - destData := c.DestTenantSQL.QueryStr(c.T, query) - require.Equal(c.T, sourceData, destData) - } - c.DestSysSQL.Exec(t, `RESUME JOB $1`, ingestionJobID) jobutils.WaitForJobToRun(t, c.DestSysSQL, jobspb.JobID(ingestionJobID)) - alternateSrcTenantSQL.Exec(t, "CREATE TABLE d.x (id INT PRIMARY KEY, n INT)") - alternateSrcTenantSQL.Exec(t, `INSERT INTO d.x VALUES (3);`) - - var cutoverTime time.Time - alternateSrcSysSQL.QueryRow(t, "SELECT clock_timestamp()").Scan(&cutoverTime) - + cutoverTime := c.SrcCluster.Server(1).Clock().Now().GoTime() var cutoverStr string c.DestSysSQL.QueryRow(c.T, `ALTER TENANT $1 COMPLETE REPLICATION TO SYSTEM TIME $2::string`, c.Args.DestTenantName, cutoverTime).Scan(&cutoverStr) @@ -539,18 +524,15 @@ func TestTenantStreamingUnavailableStreamAddress(t *testing.T) { require.NoError(t, cleanUpTenant()) }() - // The destroyed address should have been removed from the topology + // The destroyed address should have been removed from the topology. progress = jobutils.GetJobProgress(c.T, c.DestSysSQL, jobspb.JobID(ingestionJobID)) newStreamAddresses := progress.GetStreamIngest().StreamAddresses require.Contains(t, streamAddresses, destroyedAddress) require.NotContains(t, newStreamAddresses, destroyedAddress) - alternateCompareResult("SELECT * FROM d.t1") - alternateCompareResult("SELECT * FROM d.t2") - alternateCompareResult("SELECT * FROM d.x") - - // We can't use alternateCompareResult because it'll try to contact the deceased - // n1 even if the lease holders for d.scattered have all moved to other nodes + // Verify the destination tenant is fully replicated. + destData := c.DestTenantSQL.QueryStr(c.T, "SELECT * FROM d.x") + require.Equal(c.T, [][]string{{"3", "NULL"}}, destData) dstScatteredData := c.DestTenantSQL.QueryStr(c.T, "SELECT * FROM d.scattered ORDER BY key") require.Equal(t, srcScatteredData, dstScatteredData) }