sql: fix exec+audit logs for BEGIN, COMMIT, SET stmts#107912
sql: fix exec+audit logs for BEGIN, COMMIT, SET stmts#107912craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
j82w
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
-- commits line 4 at r1:
Does the audit log capture queries that fail from syntax issues?
pkg/sql/conn_executor_exec.go line 755 at r1 (raw file):
// If adminAuditLogging is enabled, we want to check for HasAdminRole // before maybeLogStatement. // We must check prior to execution in the case the txn is aborted due to
Is there any way to keep all this logic in the same place or have a flag on the request?
pkg/sql/conn_executor_exec.go line 757 at r1 (raw file):
// We must check prior to execution in the case the txn is aborted due to // an error. HasAdminRole can only be checked in a valid txn. if adminAuditLog := adminAuditLogEnabled.Get(
Should the ex.extraTxnState.hasAdminRoleCache.IsSet be checked and skip this logic if it's already set?
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, j82w (Jake) wrote…
Does the audit log capture queries that fail from syntax issues?
i don't believe it does, but if we want it to, i think that should be handled as a separate issue, since it sounds more like a feature request
pkg/sql/conn_executor_exec.go line 755 at r1 (raw file):
Previously, j82w (Jake) wrote…
Is there any way to keep all this logic in the same place or have a flag on the request?
just to clarify, which logic do you refer to? and same place as what other logic?
pkg/sql/conn_executor_exec.go line 757 at r1 (raw file):
Previously, j82w (Jake) wrote…
Should the
ex.extraTxnState.hasAdminRoleCache.IsSetbe checked and skip this logic if it's already set?
the code is already doing that (3 lines below this one)
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, rafiss (Rafi Shamim) wrote…
i don't believe it does, but if we want it to, i think that should be handled as a separate issue, since it sounds more like a feature request
we do log all syntax errors in the exec log though:
cockroach/pkg/sql/pgwire/conn.go
Line 349 in dc28e19
j82w
left a comment
There was a problem hiding this comment.
Please add a test. Other than that it LGTM.
Reviewable status:
de1dad2 to
ae2a391
Compare
Release note (bug fix): Fixed a bug where BEGIN, COMMIT, SET, ROLLBACK, and SAVEPOINT statements would not be written to the execution or audit logs.
There was no need for this to live in the sessioninit cache. It just caused an extra dependency. Release note: None
We don't need this field, since it's only used in one conditional, and that check can be replaced by a direct check of the txn associated with the planner. Release note: None
|
tftr! bors r=j82w |
|
Timed out. |
|
bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 4a3e787 to blathers/backport-release-23.1-107912: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Epic: None
Release note (bug fix): Fixed a bug where BEGIN, COMMIT, SET, ROLLBACK, and SAVEPOINT statements would not be written to the execution or audit logs.