cockroachdb / cockroachdb/cockroach

sql: byte monitor panic when a hard(ungraceful) shutdown catches up with a graceful drain

Open
#101,297 2 comments 0 reactions 0 assignees View on GitHub
A-sql-memmon C-bug T-sql-queries
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Describe the problem**

While investigating #100989 I created the following scenario:

1. SQL graceful shutdown starts. This causes in the background the SQL statement stats provider to start persisting txn/stmt stats.
2. while the above is ongoing, the test triggers a hard stop, by quiescing the stopper + canceling the graceful drain context. We can confirm in logs that it causes the _ongoing_ stmt/txn persistence queries to be canceled.
3. when all stopper tasks have quiesced, this finalizer in stats provider is called by stopper (from ` sql > sqlstats > persistedsqlstats > provider.go > - (*PersistedSQLStats).Start`):
```go
stopper.AddCloser(stop.CloserFn(func() {
s.cfg.InternalExecutorMonitor.Stop(ctx)
}))
```

And this panics:
```
*
* ERROR: a panic has occurred!
* internal SQL executor: unexpected 122880 leftover bytes
* (1) attached stack trace
* -- stack trace:
* | runtime.gopanic
* | GOROOT/src/runtime/panic.go:884
* | [...repeated from below...]
* Wraps: (2) attached stack trace
* -- stack trace:
* | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.ReportOrPanic
* | github.com/cockroachdb/cockroach/pkg/util/log/logcrash/crash_reporting.go:390
* | github.com/cockroachdb/cockroach/pkg/util/mon.(*BytesMonitor).doStop
* | github.com/cockroachdb/cockroach/pkg/util/mon/bytes_usage.go:592
* | github.com/cockroachdb/cockroach/pkg/util/mon.(*BytesMonitor).Stop
* | github.com/cockroachdb/cockroach/pkg/util/mon/bytes_usage.go:565
* | github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats.(*PersistedSQLStats).Start.func1
* | github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats/pkg/sql/sqlstats/persistedsqlstats/provider.go:112
* | github.com/cockroachdb/cockroach/pkg/util/stop.CloserFn.Close
* | github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:107
* | github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).Stop
* | github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:532
```

This bug is relatively hard to trigger because in most cases, a server is shut down either only using a graceful drain, or only using a hard stop, but not both.

`TestServerStartStop` happens to do this and this is where this problem was caught.

I am to work around this by changing `mon.Stop` to `mon.EmergencyStop` in that specific case. This papers over the issue and prevents the panic from occurring. However, this is not a good long term fix because it disables the leak detection code, which we still would like to use.

**To Reproduce**

Take the code from #101296. Remove the last commit which replaces `mon.Stop` by `mon.EmergencyStop` in ` sql > sqlstats > persistedsqlstats > provider.go > - (*PersistedSQLStats).Start`. Then stress test `TestServerStartStop`. Recommended flags:

```
dev test //pkg/ccl/serverccl -f TestServerStartStop --verbose --stress --test-args='-vmodule=bytes_usage=2'
```

**Expected behavior**

The abrupt termination of ongoing SQL stats queries during shutdown should still result in an orderly cleanup of the memory monitors.

Jira issue: CRDB-26896

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.