Skip to content

fix(sql): potential memory leak in plain INSERTs#5706

Merged
bluestreak01 merged 7 commits intomasterfrom
puzpuzpuz_insert_mem_leak
May 30, 2025
Merged

fix(sql): potential memory leak in plain INSERTs#5706
bluestreak01 merged 7 commits intomasterfrom
puzpuzpuz_insert_mem_leak

Conversation

@puzpuzpuz
Copy link
Copy Markdown
Contributor

@puzpuzpuz puzpuzpuz commented May 29, 2025

Fixes potential memory leak in plain INSERTs: timestamp and record SQL functions were never closed

@puzpuzpuz puzpuzpuz self-assigned this May 29, 2025
@puzpuzpuz puzpuzpuz added Bug Incorrect or unexpected behavior SQL Issues or changes relating to SQL execution labels May 29, 2025
@puzpuzpuz puzpuzpuz marked this pull request as ready for review May 30, 2025 06:36
@jerrinot
Copy link
Copy Markdown
Contributor

How come this was not caught by existing tests?

@puzpuzpuz
Copy link
Copy Markdown
Contributor Author

How come this was not caught by existing tests?

That's simple: we had no tests with off-heap SQL functions in INSERT.

@jerrinot
Copy link
Copy Markdown
Contributor

jerrinot commented May 30, 2025

@mtopolnik this is like a deje-vu:

That's simple: we had no tests with off-heap SQL functions in INSERT.

I swear we dealt with this before.

@puzpuzpuz
Copy link
Copy Markdown
Contributor Author

puzpuzpuz commented May 30, 2025

¯\_(ツ)_/¯

@puzpuzpuz
Copy link
Copy Markdown
Contributor Author

@jerrinot fancy doing a code review?

@jerrinot
Copy link
Copy Markdown
Contributor

@puzpuzpuz sure

@glasstiger
Copy link
Copy Markdown
Contributor

[PR Coverage check]

😍 pass : 48 / 51 (94.12%)

file detail

path covered line new line coverage
🔵 io/questdb/cairo/CairoEngine.java 12 15 80.00%
🔵 io/questdb/cutlass/pgwire/modern/PGPipelineEntry.java 19 19 100.00%
🔵 io/questdb/griffin/engine/ops/InsertAsSelectOperationImpl.java 1 1 100.00%
🔵 io/questdb/cutlass/pgwire/modern/PGConnectionContextModern.java 2 2 100.00%
🔵 io/questdb/griffin/SqlCompilerImpl.java 8 8 100.00%
🔵 io/questdb/griffin/engine/ops/InsertOperationImpl.java 2 2 100.00%
🔵 io/questdb/griffin/InsertRowImpl.java 3 3 100.00%
🔵 io/questdb/cutlass/pgwire/modern/TypesAndInsertModern.java 1 1 100.00%

@puzpuzpuz
Copy link
Copy Markdown
Contributor Author

@jerrinot BTW many thanks for adding pgwire driver tests - they're of a great help when it comes to tricky scenarios. Without them this patch would certainly break some clients.

@puzpuzpuz
Copy link
Copy Markdown
Contributor Author

@jerrinot @mtopolnik thanks for the review!

@bluestreak01 bluestreak01 merged commit 5727d25 into master May 30, 2025
37 checks passed
@bluestreak01 bluestreak01 deleted the puzpuzpuz_insert_mem_leak branch May 30, 2025 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Incorrect or unexpected behavior SQL Issues or changes relating to SQL execution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants