cockroachdb / cockroachdb/cockroach
sql: byte monitor panic when a hard(ungraceful) shutdown catches up with a graceful drain
- 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
Assessment
This issue has not been assessed yet.