sql: do not print stack trace when logging if txn is not open#91160
sql: do not print stack trace when logging if txn is not open#91160craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
Here is an example I saw in one of the tickets (somewhat redacted): |
knz
left a comment
There was a problem hiding this comment.
nice find 💯
this will benefit from a backport
andreimatei
left a comment
There was a problem hiding this comment.
Any chance I can snipe you into figuring out a better story for the relationship between a txn and resolving names? I no longer know what I'm talking about, but it has always bothered me that resolving names is tied to the kv txn needing the resolving. Resolving names should be independent of any kv txn; I think it should only need a timestamp and a descriptor collection (or whatever has to do with the state of dirty descriptors modified in the current SQL txn). I feel like this has come up many times and we keep patching over it, and if I understand correctly, this here is one more bandaid.
cc @ajwerner
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/exec_log.go line 337 at r1 (raw file):
// extract a name to include in the logging events. if p.txn != nil && p.txn.IsAborted() { // Do not attempt to resolve the table name if the txn is
nit: cut the embellishment from this comment; say that you can't use an aborted txn
ajwerner
left a comment
There was a problem hiding this comment.
Well, in this case, the collection doesn't have a modified copy of this table and doesn't have a lease/couldn't get one. The descs.Collection is tied implicitly to a transaction. I think if you want to resolve a name outside the context of a transaction, you should have to create a new transaction and a new collection. There are tools to do that.
An alternative is to fail if we fail to find the table in the lease manager if the transaction we have a handle to is not open. Maybe that'd be better. My guess is that the table in question was dropped.
Reviewable status:
ajwerner
left a comment
There was a problem hiding this comment.
I'll revise the above assessment to say that the database was dropped.
Reviewable status:
e793fa6 to
ba16ab8
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
I initially misread the stack trace - note REASON_TXN_COMMITTED, so we can only use the txn for the resolution if it is still in an open state. PTAL.
Reviewable status:
ba16ab8 to
2242e68
Compare
After executing each statement, that statement might be logged. If there were any audit events, then we attempt to resolve the table names for which the audit events have occurred. To do the resolution we're using the current txn. Previously, if that txn has been aborted or committed, it would result in a scary-looking stack trace added to the log, and this commit fixes it. Release note: None
2242e68 to
d9726d2
Compare
|
@knz PTAL |
|
TFTR! 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 d9726d2 to blathers/backport-release-22.1-91160: 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 22.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
After executing each statement, that statement might be logged. If there
were any audit events, then we attempt to resolve the table names for
which the audit events have occurred. To do the resolution we're using
the current txn. Previously, if that txn has been aborted or committed,
it would result in a scary-looking stack trace added to the log, and
this commit fixes it.
Epic: None
Release note: None