Skip to content

Commit

Permalink
sql: make sure to close the reserved account for each session
Browse files Browse the repository at this point in the history
This commit makes it more clear that the "reserved" memory account
created for each connection is closed when that connection is closed.
Previously, the account was already cleared (when "session root" monitor
is stopped which happens when the connExecutor stops), so there was no
leak in the accounting system because of it, yet it wasn't obvious - this
commit makes it more obvious.

Release note: None
  • Loading branch information
yuzefovich committed Jun 30, 2022
1 parent 09e4f6b commit 581c07d
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 4 deletions.
8 changes: 7 additions & 1 deletion pkg/sql/conn_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -746,12 +746,18 @@ func (h ConnectionHandler) GetQueryCancelKey() pgwirecancel.BackendKeyData {
// embedded in the ConnHandler.
//
// If not nil, reserved represents memory reserved for the connection. The
// connExecutor takes ownership of this memory.
// connExecutor takes ownership of this memory and will close the account before
// exiting.
func (s *Server) ServeConn(
ctx context.Context, h ConnectionHandler, reserved *mon.BoundAccount, cancel context.CancelFunc,
) error {
defer func() {
r := recover()
// Make sure to close the reserved account even if closeWrapper below
// panics - so we do it in a defer that is guaranteed to execute. We
// also cannot close it before closeWrapper since we need to close the
// internal monitors of the connExecutor first.
defer reserved.Close(ctx)
h.ex.closeWrapper(ctx, r)
}()
return h.ex.run(ctx, s.pool, reserved, cancel)
Expand Down
6 changes: 3 additions & 3 deletions pkg/sql/pgwire/conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,7 @@ func (c *conn) serveImpl(
} else {
// sqlServer == nil means we are in a local test. In this case
// we only need the minimum to make pgx happy.
defer reserved.Close(ctx)
var err error
for param, value := range testingStatusReportParams {
err = c.sendParamStatus(param, value)
Expand All @@ -376,7 +377,6 @@ func (c *conn) serveImpl(
}
}
if err != nil {
reserved.Close(ctx)
return
}
var ac AuthConn = authPipe
Expand All @@ -387,7 +387,6 @@ func (c *conn) serveImpl(
procCh = dummyCh

if err := c.sendReadyForQuery(0 /* queryCancelKey */); err != nil {
reserved.Close(ctx)
return
}
}
Expand Down Expand Up @@ -621,7 +620,8 @@ func (c *conn) serveImpl(
// Args:
// ac: An interface used by the authentication process to receive password data
// and to ultimately declare the authentication successful.
// reserved: Reserved memory. This method takes ownership.
// reserved: Reserved memory. This method takes ownership and guarantees that it
// will be closed when this function returns.
// cancelConn: A function to be called when this goroutine exits. Its goal is to
// cancel the connection's context, thus stopping the connection's goroutine.
// The returned channel is also closed before this goroutine dies, but the
Expand Down

0 comments on commit 581c07d

Please sign in to comment.