-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql: fix "no user specified" errors in statement bundles #80405
Conversation
pkg/sql/explain_bundle.go
Outdated
// explainBundleUserSessionsDataOverride is an InternalExecutorOverride which | ||
// overrides the users to the RootUser. | ||
var explainBundleUserSessionDataOverride = sessiondata.InternalExecutorOverride{ | ||
User: security.MakeSQLUsernameFromPreNormalizedString(security.RootUser)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to use a root user override here? If not, what is a better alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @rafiss, and @RichardJCai)
pkg/sql/explain_bundle.go, line 438 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Is it safe to use a root user override here? If not, what is a better alternative?
This looks suspicious. Why can't this use the identity of the user requesting the bundle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @rafiss, and @RichardJCai)
pkg/sql/explain_bundle.go, line 438 at r1 (raw file):
Previously, knz (kena) wrote…
This looks suspicious. Why can't this use the identity of the user requesting the bundle?
As in, make stmtEnvCollector
take the requester's username as argument or something like that.
was this fixed by #79098 ? |
A regression was introduced in cockroachdb#71246 that caused errors when running internal queries to populate files in statement bundles. As a result, critical information was missing from the `env.sql`, `schema.sql`, and `stats*.sql` files. This commit fixes the issue by using the internal executor factory to create an internal executor with the current session's session data. Fixes cockroachdb#80396 Release note: None
f0413eb
to
aec2c44
Compare
How about this second attempt that doesn't use an override at all? |
is it already common practice to instantiate InternalExecutors on-the-fly? Is this an expensive operation? (Asking for a friend - if we don't need to keep a predefined instance around, I would cut down on the server code) |
I copied this pattern from: cockroach/pkg/sql/opt_exec_factory.go Lines 1160 to 1163 in 2115a44
It seems fairly prolific given that planner.QueryRowEx , which is used in numerous places, creates a new internal executor: Line 879 in d4daa99
|
Amazing. TIL. Thanks for explaining. |
TFTR! bors r+ |
Build succeeded: |
A regression was introduced in #71246 that caused errors when running
internal queries to populate files in statement bundles. As a result,
critical information was missing from the
env.sql
,schema.sql
, andstats*.sql
files. This commit fixes the issue by using the internalexecutor factory to create an internal executor with the current
session's session data.
Fixes #80396
Release note: None