sql: free scanner and parser allocations after large statement

[michae2]

After a large statement is scanned and parsed, we hang onto the scanner and parser allocations in

cockroach/pkg/sql/parser/parse.go

Lines 111 to 119 in 3d054f3

	// Parser wraps a scanner, parser and other utilities present in the parser
	// package.
	type Parser struct {
	scanner scanner.Scanner
	lexer lexer
	parserImpl sqlParserImpl
	tokBuf [8]sqlSymType
	stmtBuf [1]Statement
	}

until the session is closed. This seems like a temporary memory leak, but with connection pooling we might not actually close the session and free this memory for a long time. We should reset the parser after large statements.

Here's a repro, using python to create a large statement:

Python 3.10.10 (main, Feb  8 2023, 05:40:53) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import psycopg
>>> url = "..."
>>> conn = psycopg.connect(url)
>>> c = conn.cursor()
>>> c.execute("CREATE TABLE IF NOT EXISTS t AS SELECT 0 AS a")
>>> sql = "SELECT count(*) FROM t WHERE a IN (" + ",".join(("0" for _ in range(8000000))) + ")"
>>> len(sql) / 1024 / 1024
15.258822441101074
>>> c.execute(sql)

Even though this statement is less than 16 MiB, it causes us to hold onto over a GiB of memory in the scanner and parser, and this is not released until the connection closes:

This issue is somewhat similar to #47969 and #72581 and #80497

Jira issue: CRDB-24639

[rytaft]

Seems like this may not be a large change, so let's try to get it done this release if possible and backport.

[lyang24]

two questions out of curiosity:
how do we define large statement?

• option 1. we state a const threshold in code.
• option 2. we let customer defined with a session variable.

whats the down side of resetting parser/scanner every loop?

[jordanlewis]

Awesome find!

Did we confirm that this solves the problem? I think in some cases the prepared statement cache may hang onto some of the same memory. It's worth looking into this - depending on how the allocations are performed (are they slices of nodes?) the prepared statement cache may prevent this memory from being collected if there are pointers into bigger backing slices.

We saw problems like this before when cached things pointed directly to the backing byte buffer of e.g. a protobuf, but the same principle could apply to the bytes that come in from a connection and so on.

[michae2]

Did we confirm that this solves the problem? I think in some cases the prepared statement cache may hang onto some of the same memory. It's worth looking into this - depending on how the allocations are performed (are they slices of nodes?) the prepared statement cache may prevent this memory from being collected if there are pointers into bigger backing slices.

Great point. We're actually looking into the prepared statement cache now.

[michae2]

It looks like these two issues with prepared statements were actually responsible for the memory usage we saw:

• sql: de-duplicate prepared statement cache entries #98074
• sql: prepared statements over-account for memory on query cache hits #98071

[mgartner]

@michae2 so is this issue still valid, or should we close it?

[michae2]

@michae2 so is this issue still valid, or should we close it?

This issue is valid (the repro still causes it) but is not as urgent as I thought.

[rafiss]

This is related to this recent issue with how the scanner handles SQL comments: #127710. (Different cause, but same memory leak symptoms.)