Skip to content

Commit

Permalink
logcrash: remove debug.panic_on_failed_assertions
Browse files Browse the repository at this point in the history
This patch removes the cluster setting
`debug.panic_on_failed_assertions` in favor of the environment variable
`COCKROACH_FATAL_ASSERTIONS`. Environment variables do not depend on a
functional KV subsystem for propagation and are thus more reliable.
They also avoid having to thread cluster settings through the code.
They are less convenient, but we rarely if ever expect users to enable
fatal assertions, as they are more for use in tests and development.

Epic: none
Release note: None
  • Loading branch information
erikgrinaker committed Jul 31, 2023
1 parent f5acea9 commit d5f275c
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 29 deletions.
1 change: 0 additions & 1 deletion pkg/cli/testdata/explain-bundle/bundle/env.sql
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
-- cluster.organization = (organization name)
-- cluster.preserve_downgrade_option = (disable (automatic or manual) cluster version upgrade from the specified version until reset)
-- cluster.secret = ba65c0ef-be37-4798-ae73-e6a98b9087c4 (cluster specific secret)
-- debug.panic_on_failed_assertions = false (panic when an assertion fails rather than reporting)
-- diagnostics.forced_sql_stat_reset.interval = 2h0m0s (interval after which SQL statement statistics are refreshed even if not collected (should be more than diagnostics.sql_stat_reset.interval). It has a max value of 24H.)
-- diagnostics.reporting.enabled = true (enable reporting diagnostic metrics to cockroach labs)
-- diagnostics.reporting.interval = 1h0m0s (interval at which diagnostics data should be reported)
Expand Down
1 change: 1 addition & 0 deletions pkg/settings/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ var retiredSettings = map[string]struct{}{
"jobs.trace.force_dump_mode": {},
"timeseries.storage.30m_resolution_ttl": {},
"server.cpu_profile.enabled": {},
"debug.panic_on_failed_assertions": {},
}

// sqlDefaultSettings is the list of "grandfathered" existing sql.defaults
Expand Down
36 changes: 18 additions & 18 deletions pkg/sql/logictest/testdata/logic_test/statement_statistics
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,16 @@ SELECT x FROM test WHERE y = 1/z
# Set a cluster setting to make it show up below. Which one is set
# does not matter.
statement ok
SET CLUSTER SETTING debug.panic_on_failed_assertions = true;
SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false;

statement ok
RESET CLUSTER SETTING debug.panic_on_failed_assertions
RESET CLUSTER SETTING sql.stats.automatic_collection.enabled

statement ok
SHOW application_name

statement ok
SHOW CLUSTER SETTING debug.panic_on_failed_assertions
SHOW CLUSTER SETTING sql.stats.automatic_collection.enabled

statement ok
SET application_name = '';
Expand All @@ -156,23 +156,23 @@ SELECT key,flags
FROM test.crdb_internal.node_statement_statistics
WHERE application_name = 'valuetest' ORDER BY key, flags
----
key flags
key flags
INSERT INTO test VALUES (_, _, __more1_10__), (__more1_10__) ·
SELECT (_, _, __more1_10__) FROM test WHERE _ ·
SELECT key FROM test.crdb_internal.node_statement_statistics ·
SELECT sin(_) ·
SELECT sqrt(_) !
SELECT (_, _, __more1_10__) FROM test WHERE _ ·
SELECT key FROM test.crdb_internal.node_statement_statistics ·
SELECT sin(_) ·
SELECT sqrt(_) !
SELECT x FROM (VALUES (_, _, __more1_10__), (__more1_10__)) AS t (x) ·
SELECT x FROM test WHERE y = (_ / z) !+
SELECT x FROM test WHERE y IN (_, _, _ + x, _, _) ·
SELECT x FROM test WHERE y IN (_, _, __more1_10__) +
SELECT x FROM test WHERE y NOT IN (_, _, __more1_10__) ·
SET CLUSTER SETTING "debug.panic_on_failed_assertions" = DEFAULT ·
SET CLUSTER SETTING "debug.panic_on_failed_assertions" = _ ·
SET application_name = '_' ·
SET distsql = "on" ·
SHOW CLUSTER SETTING "debug.panic_on_failed_assertions" ·
SHOW application_name ·
SELECT x FROM test WHERE y = (_ / z) !+
SELECT x FROM test WHERE y IN (_, _, _ + x, _, _) ·
SELECT x FROM test WHERE y IN (_, _, __more1_10__) +
SELECT x FROM test WHERE y NOT IN (_, _, __more1_10__) ·
SET CLUSTER SETTING "sql.stats.automatic_collection.enabled" = DEFAULT ·
SET CLUSTER SETTING "sql.stats.automatic_collection.enabled" = _ ·
SET application_name = '_' ·
SET distsql = "on" ·
SHOW CLUSTER SETTING "sql.stats.automatic_collection.enabled" ·
SHOW application_name ·

# Check that the latency measurements looks reasonable, protecting
# against failure to measure (#22877).
Expand Down
1 change: 0 additions & 1 deletion pkg/sql/show_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1272,7 +1272,6 @@ func TestLintClusterSettingNames(t *testing.T) {
"server.failed_reservation_timeout": `server.failed_reservation_timeout: use ".timeout" instead of "_timeout"`,
"server.web_session_timeout": `server.web_session_timeout: use ".timeout" instead of "_timeout"`,
"sql.distsql.flow_stream_timeout": `sql.distsql.flow_stream_timeout: use ".timeout" instead of "_timeout"`,
"debug.panic_on_failed_assertions": `debug.panic_on_failed_assertions: use .enabled for booleans`,
"diagnostics.reporting.send_crash_reports": `diagnostics.reporting.send_crash_reports: use .enabled for booleans`,
"kv.closed_timestamp.follower_reads_enabled": `kv.closed_timestamp.follower_reads_enabled: use ".enabled" instead of "_enabled"`,
"kv.raft_log.disable_synchronization_unsafe": `kv.raft_log.disable_synchronization_unsafe: use .enabled for booleans`,
Expand Down
10 changes: 1 addition & 9 deletions pkg/util/log/logcrash/crash_reporting.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,6 @@ var (
true,
)

// PanicOnAssertions wraps "debug.panic_on_failed_assertions"
PanicOnAssertions = settings.RegisterBoolSetting(
settings.TenantWritable,
"debug.panic_on_failed_assertions",
"panic when an assertion fails rather than reporting",
false,
)

// startTime records when the process started so that crash reports can
// include the server's uptime as an extra tag.
startTime = timeutil.Now()
Expand Down Expand Up @@ -146,7 +138,7 @@ func RecoverAndReportPanic(ctx context.Context, sv *settings.Values) {
func RecoverAndReportNonfatalPanic(ctx context.Context, sv *settings.Values) {
if r := recover(); r != nil {
ReportPanic(ctx, sv, r, depthForRecoverAndReportPanic)
if !build.IsRelease() || PanicOnAssertions.Get(sv) {
if !build.IsRelease() {
panic(r)
}
}
Expand Down

0 comments on commit d5f275c

Please sign in to comment.