sql: prep migration for sampled_query and sampled_transaction events #151949
+166
−136
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kyle-a-wong
requested review from
alyshanjahani-crl,
arjunmahishi,
aa-joshi and
Abhinav1299
and removed request for
a team
|
This change is |
kyle-a-wong
force-pushed
the
migrate_sampledQuery_log
branch
from
a80a35f to
437b341
Compare
kyle-a-wong
force-pushed
the
migrate_sampledQuery_log
branch
from
437b341 to
3f769d7
Compare
| @@ -436,7 +437,11 @@ func (p *planner) maybeLogStatementInternal( | |||
| SchemaChangerMode: p.curPlan.instrumentation.schemaChangerMode.String(), | |||
| } | |||
|
|
|||
| p.logEventsOnlyExternally(ctx, sampledQuery) | |||
| migrator := log.NewStructuredEventMigrator(func() bool { | |||
There was a problem hiding this comment.
it's not obvious to me that logEventsOnlyExternally is equivalent to migrator.StructuredEvent
Is there some test coverage for this?
There was a problem hiding this comment.
@alyshanjahani-crl There are a decent amount of tests that cover logging SampledQuery events. The main functionality that logEventsOnlyExternally provides over using log.StructuredEvent are:
- It supports a variable amount of events as the last parameter, which isn't used here
- It adds sql event details via
p.getCommonSQLEventDetails, which i added above
IMO this function should just be removed as its not a very beneficial wrapper, but I'll leave that for another PR
alyshanjahani-crl
approved these changes
Aug 18, 2025
kyle-a-wong
force-pushed
the
migrate_sampledQuery_log
branch
from
3f769d7 to
89ed383
Compare
In v26.1, sampled_query and sampled_transaction events will be moved from the TELEMETRY logging channel to the SQL_EXEC logging channel. This commit gates this migration under the cluster setting: `log.channel_compatibility_mode.enabled` and will log these events to the SQL_EXEC channel if this setting is set to false. Users can set this setting to false in their clusters to validate, test, and identify potential downstream impacts to their logging setups and pipelines. Epic: CRDB-53410 Part of: CRDB-53412 Release note (ops change): sampled_query and sampled_transaction events will be moved to the SQL_EXEC channel in 26.1. In order to test the impact of these changes, users can set the setting: `log.channel_compatibility_mode.enabled` to false. Note that this will cause these logs to start logging in the SQL_EXEC channel so this shouldn't be tested in a production environment.
kyle-a-wong
force-pushed
the
migrate_sampledQuery_log
branch
from
89ed383 to
6e52e0c
Compare
|
tftr bors r+ |
|
bors retry |
craig bot
pushed a commit
that referenced
this pull request
Aug 19, 2025
151362: sql: gate system database behind session variable r=angles-n-daemons a=angles-n-daemons It today isn't quite clear which internal objects are safe for external usage. For example, selecting from crdb_internal.zones is considered safe, while deleting from system.jobs is not. To address this, we're adding a gate protecting some of these internals, as well as auditing around their usage so that we can better see how the internal state of the database may have been modified. The start of this effort is this PR, which adds an gate on the privilege check for system descriptors. This gate is simple now, it checks whether the caller is internal or if the override session variable is set to access "unsafe internals". It should be noted that allowance is the default to start, so that extensive testing can be done on this feature before its rolled out more widely. Fixes: #149594 Fixes: #151333 Epic: CRDB-24527 Release note (sql change): Adds a new session variable `allow_unsafe_internals` and corresponding cluster setting (`sql.defaults.allow_unsafe_internals`) which controls access to the `system` database. 151774: kv,sql: explicitly set IsReverse on the Header r=yuzefovich a=yuzefovich When processing batches that touch multiple ranges, the DistSender needs to know whether to iterate across those ranges in the forward or reverse manner (i.e. ASC or DESC for range keys). Currently, it only uses the reverse direction if it finds at least one ReverseScan request in the batch, and it uses the forward direction otherwise. This goes against the needs of SQL which might issue point Gets when performing a SQL revscan operation, and currently in such a scenario we would hit an error (instead of returning the results in incorrect order). This commit fixes the issue by introducing `IsReverse` boolean on the batch header to explicitly indicate the direction for range iteration. It seems reasonable that the caller should be explicit about this, and we also add a validation that the boolean is set correctly (meaning that it should be `false` when only forward range requests are present and `true` when only reverse range requests are present). In order to simplify the story a bit, the DistSender will no longer allow batches that have both forward and reverse range requests. Previously, this limitation only applied to the batches with limits, but now it's extended to be unconditional. SQL never issues such batches, so the limitation seems acceptable. This limitation required some updates to the existing tests, including KVNemesis to not generate batches that are now disallowed. See also 619f395 for some related context. Given that the new header field is only examined on the KV client, the change can be backported with no mixed-version concerns. Fixes: #146637. Release note (bug fix): Previously, CockroachDB could hit an error `ERROR: span with results after resume span...` when evaluating some queries with ORDER BY ... DESC in an edge case. The bug has been present since about v22.1 and is now fixed. 151949: sql: prep migration for sampled_query and sampled_transaction events r=kyle-a-wong a=kyle-a-wong In v26.1, sampled_query and sampled_transaction events will be moved from the TELEMETRY logging channel to the SQL_EXEC logging channel. This commit gates this migration under the cluster setting: `log.channel_compatibility_mode.enabled` and will log these events to the SQL_EXEC channel if this setting is set to false. Users can set this setting to false in their clusters to validate, test, and identify potential downstream impacts to their logging setups and pipelines. Epic: [CRDB-53410](https://cockroachlabs.atlassian.net/browse/CRDB-53410) Part of: [CRDB-53412](https://cockroachlabs.atlassian.net/browse/CRDB-53412) Release note (ops change): sampled_query and sampled_transaction events will be moved to the SQL_EXEC channel in 26.1. In order to test the impact of these changes, users can set the setting: `log.channel_compatibility_mode.enabled` to false. Note that this will cause these logs to start logging in the SQL_EXEC channel so this shouldn't be tested in a production environment. ---- This is a stacked PR, only the last commit in this PR should be reviewed Co-authored-by: Brian Dillmann <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Kyle Wong <[email protected]>
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
bors retry |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In v26.1, sampled_query and sampled_transaction events will be moved
from the TELEMETRY logging channel to the SQL_EXEC logging channel.
This commit gates this migration under the cluster setting:
log.channel_compatibility_mode.enabledand will log these eventsto the SQL_EXEC channel if this setting is set to false. Users can
set this setting to false in their clusters to validate, test, and
identify potential downstream impacts to their logging setups and
pipelines.
Epic: CRDB-53410
Part of: CRDB-53412
Release note (ops change): sampled_query and sampled_transaction
events will be moved to the SQL_EXEC channel in 26.1. In order to
test the impact of these changes, users can set the setting:
log.channel_compatibility_mode.enabledto false. Note that thiswill cause these logs to start logging in the SQL_EXEC channel so
this shouldn't be tested in a production environment.
This is a stacked PR, only the last commit in this PR should be reviewed