sql: fix exec+audit logs for BEGIN, COMMIT, SET stmts

[rafiss]

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.

[blathers-crl]

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.}

[cockroach-teamcity]

This change is 

[j82w]

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]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w)

---

-- commits line 4 at r1:

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.IsSet be checked and skip this logic if it's already set?

the code is already doing that (3 lines below this one)

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w)

---

-- commits line 4 at r1:

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

log.SqlExec.Infof(ctx, "could not parse simple query: %s", query)

https://github.com/cockroachdb/cockroach/blob/dc28e1932b9b01907203374005d450cfda2a562a/pkg/sql/pgwire/conn.go#L485C6-L485C6

[j82w]

Please add a test. Other than that it LGTM.

Reviewable status: complete! 0 of 0 LGTMs obtained

[rafiss]

tftr!

bors r=j82w

[craig]

Timed out.

[rafiss]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

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.}