Skip to content
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,log: also include the stmt tag in query logs #64871

Merged
merged 1 commit into from
May 10, 2021

Conversation

knz
Copy link
Contributor

@knz knz commented May 7, 2021

This was requested from a customer through a case handled by @pxlogan

Release note (sql change): The statement type ("tag") is now also
included alongside the full text of the SQL query in the various
structured log entries produced when query execution is being logged.

@knz knz requested a review from rafiss May 7, 2021 12:19
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Release note (sql change): The statement type ("tag") is now also
included alongside the full text of the SQL query in the various
structured log entries produced when query execution is being logged.
@knz knz force-pushed the 20210507-stmt-tag branch from 8dade0b to f6693a5 Compare May 7, 2021 20:47
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@knz
Copy link
Contributor Author

knz commented May 10, 2021

TFYR!

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented May 10, 2021

Build succeeded:

@craig craig bot merged commit 051bcf7 into cockroachdb:master May 10, 2021
@knz knz deleted the 20210507-stmt-tag branch May 11, 2021 10:44
craig bot pushed a commit that referenced this pull request May 21, 2021
64894: sql/event_log: reduce tech debt, simplify and fix bugs r=rafiss a=knz

Fixes #64685.
First commit from #64871. 

Prior to this patch, there was a little mess in `sql/event_log.go`
that had been introduced when "optimizing" GRANT/REVOKE to only
use one write batch for all the events logged:

- a lot of code had been duplicated;
- the control flow had been rendered more complex;
- the API interface for the functions in event_log.go
  were complex to use, requiring callers to provide descriptor
  IDs and structured events as separate slices;
- the optimizing logic was not properly applied to the
  other case where multiple events are emitted:
  SQL audit logging in `exec_log.go`.

This patch streamlines this by reducing `event_log.go` back to its
simpler form: an overall event refinement pipeline with a
straightforward control flow.

To guide future maintainers, the patch also adds an explanatory
comment at the top of the file that sketches the overall structure of
the pipeline.

Finally, this patch fixes a bug introduced when query logging
started using structured events: the ability to automatically
copy all the execution events to the DEV channel when
setting the `vmodule` setting to `exec_log=2` (or above).

In addition to fixing that bug, the following new vmodule-based
abilities are added:

- events for DDL statements and others that call `logEvent()` can now
  be collected in the DEV channel by using the name of the source file
  where they were generated as filter (e.g. `vmodule=create_table=2` for
  the CREATE TABLE events.
- events of other kinds can be collected in the DEV channel
  by setting `vmodule=event_log=2`.

(Note a subtle difference between `vmodule=create_table=2` and
`vmodule=exec_log=2`: the former emits the event to the DEV channel
while the stmt is executed; the latter emits the event after the stmt
completes. If both are enabled, TWO events are sent to the DEV channel.)

Since all the vmodule filtering options are subject to change without
notice between versions, we do not wish to document these nuances.
For this reason, the release note below is left blank.

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants