Skip to content

release-22.1: sql: improve stack trace for get-user-timeout timeouts#95888

Merged
ecwall merged 1 commit intorelease-22.1from
blathers/backport-release-22.1-95797
Jan 26, 2023
Merged

release-22.1: sql: improve stack trace for get-user-timeout timeouts#95888
ecwall merged 1 commit intorelease-22.1from
blathers/backport-release-22.1-95797

Conversation

@blathers-crl
Copy link
Copy Markdown

@blathers-crl blathers-crl bot commented Jan 26, 2023 •
edited by ecwall
Loading

Backport 1/1 commits from #95797 on behalf of @ecwall.

/cc @cockroachdb/release

Fixes #95794

The cause of the get-user-timeout errors is unknown. Part of the problem is that the stack trace gets cut off at

  |   | github.com/cockroachdb/cockroach/pkg/sql.retrieveSessionInitInfoWithCache
  |   | 	github.com/cockroachdb/cockroach/pkg/sql/user.go:238

which does not explain what is actually being blocked.

The reason that the stack trace is cut off is that the timeout is initiated by contextutil.RunWithTimeout which results in a "simple" (no stack trace) context.DeadlineExceeded error.

retrieveSessionInitInfoWithCache is the first line in the stack trace because it calls errors.Wrap on context.DeadlineExceeded.

To get a fuller stack trace, context.DeadlineExceeded must be wrapped immediately (errors.Wrap or errors.WithStack) before it bubbles up.

Release note: None

Release justification: Stack trace enhancement.

@ecwall
sql: improve stack trace for get-user-timeout timeouts
60e3926 
Fixes #95794

The cause of the `get-user-timeout errors` is unknown.
Part of the problem is that the stack trace gets cut off at
```
  |   | github.com/cockroachdb/cockroach/pkg/sql.retrieveSessionInitInfoWithCache
  |   | 	github.com/cockroachdb/cockroach/pkg/sql/user.go:238
```
which does not explain what is actually being blocked.

The reason that the stack trace is cut off is that the timeout is initiated by
`contextutil.RunWithTimeout` which results in a "simple" (no stack trace)
`context.DeadlineExceeded` error.

`retrieveSessionInitInfoWithCache` is the first line in the stack trace because
it calls `errors.Wrap` on `context.DeadlineExceeded`.

To get a fuller stack trace, `context.DeadlineExceeded` must be wrapped
immediately (`errors.Wrap` or `errors.WithStack`) before it bubbles up.

Release note: None
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-22.1-95797 branch from ff7de3f to 60e3926 Compare January 26, 2023 00:01
@blathers-crl
Copy link
Copy Markdown
Author

blathers-crl bot commented Jan 26, 2023 •
edited by knz
Loading

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Jan 26, 2023
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Jan 26, 2023

This change is Reviewable

@ecwall ecwall merged commit fddcbff into release-22.1 Jan 26, 2023
@ecwall ecwall deleted the blathers/backport-release-22.1-95797 branch January 26, 2023 15:30
@otan otan mentioned this pull request Jan 30, 2023
Closed
@otan
Copy link
Copy Markdown
Contributor

otan commented Jan 30, 2023

I believe this broke a test: https://cockroachlabs.slack.com/archives/C0168LW5THS/p1675069929758059

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ecwall ecwall Awaiting requested review from ecwall

@rafiss rafiss Awaiting requested review from rafiss

@yuzefovich yuzefovich Awaiting requested review from yuzefovich

1 more reviewer

@knz knz knz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ecwall ecwall

Labels

blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cockroach-teamcity @otan @knz @ecwall