Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql/parser: memory leak of parsed SQL comments #127710

Closed
mgartner opened this issue Jul 25, 2024 · 3 comments · Fixed by #127711
Closed

sql/parser: memory leak of parsed SQL comments #127710

mgartner opened this issue Jul 25, 2024 · 3 comments · Fixed by #127711
Assignees
Labels
branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@mgartner
Copy link
Collaborator

mgartner commented Jul 25, 2024

We've discovered a memory leak in the parser that can retain memory for parsed SQL comments for as long as a session remains open.

Here's a reproduction that continually executes SQL statements that contain comments:

package main

import (
	"context"
	"fmt"
	"github.com/jackc/pgx/v4"
	rand2 "math/rand/v2"
	"os"
	"os/signal"
	"syscall"
)

const url = "postgresql://[email protected]:26257/defaultdb?sslmode=disable"

func main() {
	ctx := context.Background()

	ch := make(chan os.Signal, 1)
	signal.Notify(ch, syscall.SIGINT)

	conn, err := pgx.Connect(context.Background(), url)
	if err != nil {
		fmt.Fprintf(os.Stderr, "Unable to connect to database: %v\n", err)
		os.Exit(1)
	}
	defer conn.Close(ctx)

LOOP:
	for i := 0; true; i++ {
		d := rand2.Int()
		q := fmt.Sprintf("SELECT /* This is a query for the integer: %d. */ %d", d, d)
		if i%1000 == 0 {
			select {
			case <-ch:
				fmt.Println("Stopping queries...")
				break LOOP
			default:
				fmt.Println(q)
			}
		}
		if _, err := conn.Exec(ctx, q); err != nil {
			fmt.Fprintf(os.Stderr, "error running query: %v\n", err)
			os.Exit(1)
		}
	}

	fmt.Println("Shutting down...")
}

Running this against v23.2.7 we can observe that memory continues to climb past 1GB after a couple of minutes:

Screenshot 2024-07-25 at 16 31 28

After applying a simply patch to remove the leak, memory plateaus just above 600MB:

Screenshot 2024-07-25 at 16 35 21

Jira issue: CRDB-40562

@mgartner mgartner added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jul 25, 2024
Copy link

blathers-crl bot commented Jul 25, 2024

Hi @mgartner, please add branch-* labels to identify which branch(es) this C-bug affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@github-project-automation github-project-automation bot moved this to Triage in SQL Queries Jul 25, 2024
@mgartner mgartner self-assigned this Jul 25, 2024
@mgartner mgartner added branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 labels Jul 25, 2024
@mgartner mgartner moved this from Triage to Active in SQL Queries Jul 25, 2024
@mgartner
Copy link
Collaborator Author

The best workaround is to regularly close and re-open connections, e.g., with a "max age" connection pool setting in the client.

mgartner added a commit to mgartner/cockroach that referenced this issue Jul 25, 2024
This commit fixes a memory leak that cause a session's
`scanner.Scanner`, which is embedded in a `pgwire.conn` via a
`parser.Parser`, to retain previously scanned SQL comments in all parsed
SQL statements. The scanner's slice of comments would continue to grow
as it parsed new SQL strings with comments, as long as the session
remained open. Also, the slice of comments holds references to the bytes
in `pgwire` read buffers, preventing those buffers from being GC'd.

Fixes cockroachdb#127710

Release note: None
@mgartner
Copy link
Collaborator Author

Some follow-up work: #127713

@mgartner mgartner added the branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 label Jul 26, 2024
craig bot pushed a commit that referenced this issue Jul 26, 2024
127711: sql/parser: fix memory leak of scanned comments r=mgartner a=mgartner

This commit fixes a memory leak that cause a session's
`scanner.Scanner`, which is embedded in a `pgwire.conn` via a
`parser.Parser`, to retain previously scanned SQL comments in all parsed
SQL statements. The scanner's slice of comments would continue to grow
as it parsed new SQL strings with comments, as long as the session
remained open. Also, the slice of comments holds references to the bytes
in `pgwire` read buffers, preventing those buffers from being GC'd.

Fixes #127710

Release note (bug fix): A bug has been fixed that caused a memory leak
when executing SQL statements with comments, for example,
`SELECT /* comment */ 1;`. Memory owned by a SQL session would continue
to grow as these types of statements were executed. The memory would
only be released when closing the SQL session. This bug has been present
since version 23.1.


Co-authored-by: Marcus Gartner <[email protected]>
@craig craig bot closed this as completed in 6ea14db Jul 26, 2024
@github-project-automation github-project-automation bot moved this from Active to Done in SQL Queries Jul 26, 2024
blathers-crl bot pushed a commit that referenced this issue Jul 26, 2024
This commit fixes a memory leak that cause a session's
`scanner.Scanner`, which is embedded in a `pgwire.conn` via a
`parser.Parser`, to retain previously scanned SQL comments in all parsed
SQL statements. The scanner's slice of comments would continue to grow
as it parsed new SQL strings with comments, as long as the session
remained open. Also, the slice of comments holds references to the bytes
in `pgwire` read buffers, preventing those buffers from being GC'd.

Fixes #127710

Release note (bug fix): A bug has been fixed that caused a memory leak
when executing SQL statements with comments, for example,
`SELECT /* comment */ 1;`. Memory owned by a SQL session would continue
to grow as these types of statements were executed. The memory would
only be released when closing the SQL session. This bug has been present
since version 23.1.
blathers-crl bot pushed a commit that referenced this issue Jul 26, 2024
This commit fixes a memory leak that cause a session's
`scanner.Scanner`, which is embedded in a `pgwire.conn` via a
`parser.Parser`, to retain previously scanned SQL comments in all parsed
SQL statements. The scanner's slice of comments would continue to grow
as it parsed new SQL strings with comments, as long as the session
remained open. Also, the slice of comments holds references to the bytes
in `pgwire` read buffers, preventing those buffers from being GC'd.

Fixes #127710

Release note (bug fix): A bug has been fixed that caused a memory leak
when executing SQL statements with comments, for example,
`SELECT /* comment */ 1;`. Memory owned by a SQL session would continue
to grow as these types of statements were executed. The memory would
only be released when closing the SQL session. This bug has been present
since version 23.1.
blathers-crl bot pushed a commit that referenced this issue Jul 26, 2024
This commit fixes a memory leak that cause a session's
`scanner.Scanner`, which is embedded in a `pgwire.conn` via a
`parser.Parser`, to retain previously scanned SQL comments in all parsed
SQL statements. The scanner's slice of comments would continue to grow
as it parsed new SQL strings with comments, as long as the session
remained open. Also, the slice of comments holds references to the bytes
in `pgwire` read buffers, preventing those buffers from being GC'd.

Fixes #127710

Release note (bug fix): A bug has been fixed that caused a memory leak
when executing SQL statements with comments, for example,
`SELECT /* comment */ 1;`. Memory owned by a SQL session would continue
to grow as these types of statements were executed. The memory would
only be released when closing the SQL session. This bug has been present
since version 23.1.
blathers-crl bot pushed a commit that referenced this issue Jul 26, 2024
This commit fixes a memory leak that cause a session's
`scanner.Scanner`, which is embedded in a `pgwire.conn` via a
`parser.Parser`, to retain previously scanned SQL comments in all parsed
SQL statements. The scanner's slice of comments would continue to grow
as it parsed new SQL strings with comments, as long as the session
remained open. Also, the slice of comments holds references to the bytes
in `pgwire` read buffers, preventing those buffers from being GC'd.

Fixes #127710

Release note (bug fix): A bug has been fixed that caused a memory leak
when executing SQL statements with comments, for example,
`SELECT /* comment */ 1;`. Memory owned by a SQL session would continue
to grow as these types of statements were executed. The memory would
only be released when closing the SQL session. This bug has been present
since version 23.1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant