sql: fix memory accounting of prepared statements and portals in error cases

[yuzefovich]

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

[cockroach-teamcity]

This change is 

[DrewKimball]

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]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @DrewKimball, and @mgartner)

---

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 prepare for 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]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach, @mgartner, and @yuzefovich)

---

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

I see, makes sense.

[yuzefovich]

TFTR!

bors r+

[craig]

Build succeeded:

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