sql: fix "no user specified" errors in statement bundles#80405
sql: fix "no user specified" errors in statement bundles#80405craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
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.
Is it safe to use a root user override here? If not, what is a better alternative?
knz
left a comment
There was a problem hiding this comment.
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?
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
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*.sqlfiles. 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