Skip to content

Commit

Permalink
Merge #64894
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
craig[bot] and knz committed May 21, 2021
2 parents 0553b25 + 5d96260 commit 303f766
Show file tree
Hide file tree
Showing 6 changed files with 419 additions and 307 deletions.
7 changes: 3 additions & 4 deletions pkg/server/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -823,11 +823,10 @@ func (n *Node) recordJoinEvent(ctx context.Context) {
if err := n.storeCfg.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
return sql.InsertEventRecord(ctx, n.sqlExec,
txn,
int32(n.Descriptor.NodeID),
int32(n.Descriptor.NodeID),
true, /* skipExternalLog - we already call log.StructuredEvent above */
int32(n.Descriptor.NodeID), /* reporting ID: the node where the event is logged */
sql.LogToSystemTable|sql.LogToDevChannelIfVerbose, /* LogEventDestination: we already call log.StructuredEvent above */
int32(n.Descriptor.NodeID), /* target ID: the node that is joining (ourselves) */
event,
false, /* onlyLog */
)
}); err != nil {
log.Warningf(ctx, "%s: unable to log event %v: %v", n, event, err)
Expand Down
6 changes: 3 additions & 3 deletions pkg/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -2346,10 +2346,10 @@ func (s *Server) Decommission(
ctx,
s.sqlServer.execCfg.InternalExecutor,
txn,
int32(nodeID), int32(s.NodeID()),
true, /* skipExternalLog - we already call log.StructuredEvent above */
int32(s.NodeID()), /* reporting ID: the node where the event is logged */
sql.LogToSystemTable|sql.LogToDevChannelIfVerbose, /* we already call log.StructuredEvent above */
int32(nodeID), /* target ID: the node that we wee a membership change for */
event,
false, /* onlyLog */
)
}); err != nil {
log.Ops.Errorf(ctx, "unable to record event: %+v: %+v", event, err)
Expand Down
26 changes: 16 additions & 10 deletions pkg/sql/create_stats.go
Original file line number Diff line number Diff line change
Expand Up @@ -575,16 +575,22 @@ func (r *createStatsResumer) Resume(ctx context.Context, execCtx interface{}) er
// CREATE STATISTICS statement.
// See: https://github.com/cockroachdb/cockroach/issues/57739
return evalCtx.ExecCfg.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
return logEventInternalForSQLStatements(ctx, evalCtx.ExecCfg, txn,
descpb.IDs{details.Table.ID},
evalCtx.SessionData.User(),
evalCtx.SessionData.ApplicationName,
details.Statement,
"CREATE STATISTICS",
nil, /* no placeholders known at this point */
true, /* writeToEventLog */
&eventpb.CreateStatistics{
TableName: details.FQTableName,
return logEventInternalForSQLStatements(ctx,
evalCtx.ExecCfg, txn,
0, /* depth: use event_log=2 for vmodule filtering */
eventLogOptions{dst: LogEverywhere},
sqlEventCommonExecPayload{
user: evalCtx.SessionData.User(),
appName: evalCtx.SessionData.ApplicationName,
stmt: details.Statement,
stmtTag: "CREATE STATISTICS",
placeholders: nil, /* no placeholders known at this point */
},
eventLogEntry{
targetID: int32(details.Table.ID),
event: &eventpb.CreateStatistics{
TableName: details.FQTableName,
},
},
)
})
Expand Down
Loading

0 comments on commit 303f766

Please sign in to comment.