sql: fix memory accounting of prepared statements and portals in error cases#84049
Conversation
|
This change is |
Previously, it was possible that we would not close the memory account created for a prepared statement when an error is encountered. This was the case because we would not include the prepared stmt into the prep stmts namespace, so it would just get lost. However, up until recently this was not an issue since we mistakenly cleared that memory account when creating the prepared statement. Release note: None
4a785d4 to
e37ea6b
Compare
Previously, it was possible to "leak" a reference to a prepared statement if we made a portal from it (i.e. took a reference to the prepared statement) and the memory reservation was denied. This could lead to "leftover bytes" errors when stopping the "session" monitors. However, the impact is minor because on release builds we'd still return those "leftover bytes" and would just file a sentry issue. This is now fixed. Release note: None
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @mgartner, and @yuzefovich)
pkg/sql/conn_executor_prepare.go line 103 at r2 (raw file):
// Prepare the query. This completes the typing of placeholders. prepared, err := ex.prepare(ctx, stmt, placeholderHints, rawTypeHints, origin) if err != nil {
[nit] Are there cases where error is non-nil but we don't want to close the account? If not, we might be nice to close the account here instead of in prepare for symmetry with the other two cases. Up to you though, it doesn't really matter.
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor_prepare.go line 103 at r2 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
[nit] Are there cases where error is non-nil but we don't want to close the account? If not, we might be nice to close the account here instead of in
preparefor symmetry with the other two cases. Up to you though, it doesn't really matter.
I think we want to close the account in all cases when the reference to the prepared statement is lost (i.e. not assigned into ex.extraTxnState.prepStmtsNamespace.prepStmts map). If an error is returned by ex.prepare, then prepared seems to be nil in all cases, so I think it's better to keep closing the account inside of the function there.
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor_prepare.go line 103 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
I think we want to close the account in all cases when the reference to the prepared statement is lost (i.e. not assigned into
ex.extraTxnState.prepStmtsNamespace.prepStmtsmap). If an error is returned byex.prepare, thenpreparedseems to benilin all cases, so I think it's better to keep closing the account inside of the function there.
I see, makes sense.
|
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 e37ea6b to blathers/backport-release-22.1-84049: 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. |
sql: make sure to close mem acc of prepared stmt in case of an error
Previously, it was possible that we would not close the memory account
created for a prepared statement when an error is encountered. This was
the case because we would not include the prepared stmt into the prep
stmts namespace, so it would just get lost. However, up until recently
this was not an issue since we mistakenly cleared that memory account
when creating the prepared statement.
Release note: None
sql: only increment ref count of prep stmt in non-error case of portals
Previously, it was possible to "leak" a reference to a prepared
statement if we made a portal from it (i.e. took a reference to the
prepared statement) and the memory reservation was denied. This could
lead to "leftover bytes" errors when stopping the "session" monitors.
However, the impact is minor because on release builds we'd still return
those "leftover bytes" and would just file a sentry issue. This is now
fixed.
Fixes: #83935
Release note: None