Skip to content

Fix five allocation and reference leaks, and make the cap that measures them work - #36

Open
sfc-gh-okalaci wants to merge 6 commits into
up/04-lifetimesfrom
up/05-leaks
Open

Fix five allocation and reference leaks, and make the cap that measures them work#36
sfc-gh-okalaci wants to merge 6 commits into
up/04-lifetimesfrom
up/05-leaks

Conversation

@sfc-gh-okalaci

@sfc-gh-okalaci sfc-gh-okalaci commented Aug 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

None of these is visible in a single call; all of them accumulate in a long-lived
backend.

The first commit is what makes the rest testable. pljs.memory_limit was read once, at
library load, so SET pljs.memory_limit = 64 in a session changed the GUC without ever
reaching the interpreter — the runtime kept the 512MB default. A leak test that sets a
low cap and expects out of memory therefore passed whether or not the leak was fixed.
Applying the value to the live runtime on SET is a fix in its own right, and it is
placed first here because the two heap-leak tests below assert against it.

The one that matters most is a leaked QuickJS reference per column, per row, in the
composite conversion loops: JS_GetPropertyStr()'s result was never released on either
the null path or the conversion path. That is the hottest path in the extension for a
RETURNS TABLE function — its test fails with out of memory without the fix, under
the 64MB cap the first commit makes effective. It asserts against pljs.memory_limit
rather than pg_backend_memory_contexts deliberately: QuickJS allocates on the libc
heap, so that view cannot see a leaked JavaScript reference at all, and a test built on
it would pass either way.

The rest: the SPI plan and parameter list allocated per iteration of
pljs.execute()/plan.execute() were freed only on success, so a raising query leaked
both — a short-lived child context deleted unconditionally replaces the per-path frees;
the property-name table from JS_GetOwnPropertyNames() and its atoms were dropped on
the floor during object key enumeration; two JavaScript references were taken and never
released in the storage helpers.

Commits

  • Re-apply pljs.memory_limit to the live runtime on SET
  • Drop leaked JavaScript references in the storage helpers
  • Free the per-iteration SPI plan and parameter list
  • Free the property-name table from JS_GetOwnPropertyNames
  • Release the per-column JSValue in the composite conversion loops
  • Free the SPI plan and parameters when a parameterised execute raises

Every commit in this series builds from clean and passes the full ordered suite on its
own, verified per commit on PostgreSQL 17. The tip is additionally green on PostgreSQL
16, 17, 18 and 19beta3 — the versions this repository's CI matrix builds — with
pljs.memory_limit=64, and under AddressSanitizer with no reports.

@sfc-gh-okalaci sfc-gh-okalaci changed the title Fix five allocation and reference leaks Fix five allocation and reference leaks, and make the cap that measures them work Aug 18, 2026
@sfc-gh-okalaci
Re-apply pljs.memory_limit to the live runtime on SET
5473d49 
pljs.memory_limit was read when the runtime was created and never again, so SET
pljs.memory_limit changed the GUC without reaching the interpreter.  Raising it did not
allow more allocation and lowering it did not constrain any.

An assign hook now applies the value to the live runtime.  Lowering it below current
usage does not reclaim anything -- the next allocation fails with a JavaScript "out of
memory" instead, which is QuickJS's behaviour and worth knowing rather than hiding.

Adds sql/pg_memory_limit_set.sql, which lowers the limit mid-session and asserts the new
value takes effect, including that a rolled-back SET leaves the previous limit in force.
@sfc-gh-okalaci
Drop leaked JavaScript references in the storage helpers
9ac4cba 
pljs_storage_for_context() and store_storage_in_context() each took references to the
pljs namespace object and the global object and never released them.  Both run on every
context setup, so each one leaked two QuickJS references for the life of the context --
which in turn is what stops JS_FreeContext() from ever freeing that context.

The global object keeps the storage alive across the release, so reading the storage
afterwards is still safe; the comment at the site says so, because it is the obvious
worry when reading the change.

No regression test accompanies this. Each leak is a single reference per call on paths that a regression test would have to run tens of thousands of times to make visible against pljs.memory_limit, which is what the soak measurement is for rather than the suite.
@sfc-gh-okalaci
Free the per-iteration SPI plan and parameter list
b903040 
pljs_execute() and plan.execute() build a one-shot SPI plan and a ParamListInfo per
call and freed both only after a successful execute.  Anything that raised in between --
a constraint violation, a type mismatch, a cancelled query -- leaked both, for every
failing call, without bound.

Both are now allocated in a short-lived child context that is deleted unconditionally,
with SPI_freeplan() in a PG_CATCH.  That is less code than freeing on each path and it
cannot be got wrong later by adding a new early return.

Note the plan's parserSetupArg points at a stack-local pljs_param_state, so the plan has
to be freed before this frame exits; a comment at the site records that, because it looks
like an ordering that could safely be relaxed.
@sfc-gh-okalaci
Free the property-name table from JS_GetOwnPropertyNames
2ced27a 
JS_GetOwnPropertyNames() returns a table the caller owns, and each entry holds an atom
reference that must be released as well.  Object key enumeration dropped both on the
floor, so every conversion of a JavaScript object to a record leaked the table and one
atom per key.

The leak is on the QuickJS heap, which pg_backend_memory_contexts cannot see at all, so
it does not show up in the usual place one would look.

Adds sql/pg_object_keys_leak.sql.  It measures against a low pljs.memory_limit rather
than PostgreSQL's memory accounting, because that is the only SQL-visible instrument
that counts QuickJS allocations; with the release sites removed the file fails, so it
discriminates.
@sfc-gh-okalaci
Release the per-column JSValue in the composite conversion loops
e31dc61 
pljs_jsvalue_to_datums() and the column loop in pljs_jsvalue_to_record() fetch each
column's value with JS_GetPropertyStr() and never release it -- on the null path or the
conversion path.  That is one leaked QuickJS reference per column, per row.

For a RETURNS TABLE function this is the hottest path in the extension, so it is also
the leak most likely to matter: a query returning ten columns over a hundred thousand
rows leaks a million references.

Adds sql/pg_record_column_leak.sql, which returns enough rows under a low
pljs.memory_limit that the unfixed code fails with "out of memory" rather than merely
growing.
@sfc-gh-okalaci
Free the SPI plan and parameters when a parameterised execute raises
8dcf76a 
pljs_execute_params() freed its plan and parameter arrays after the execute returned, so
a raising query leaked them.  The arrays are palloc'd and the plan is a saved SPI plan,
so the leak was per failing call and unbounded in a long-lived backend.

The allocation now lives in a child context deleted on both paths, with SPI_freeplan()
in a PG_CATCH.

Adds sql/pg_param_plan_error_leak.sql, which runs a parameterised execute that raises
many times and asserts memory does not trend upwards.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@sfc-gh-okalaci