Skip to content

Fix prepared-plan and cursor lifetimes - #35

Open
sfc-gh-okalaci wants to merge 6 commits into
up/03-crashesfrom
up/04-lifetimes
Open

Fix prepared-plan and cursor lifetimes#35
sfc-gh-okalaci wants to merge 6 commits into
up/03-crashesfrom
up/04-lifetimes

Conversation

@sfc-gh-okalaci

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

Copy link
Copy Markdown
Collaborator

Six fixes to how long a prepared plan and its portal live, and where their memory
comes from.

Plan data was allocated in the caller's short-lived context while the plan itself was
saved, so the plan outlived the state it pointed at. It now comes from
CacheMemoryContext. Note that the plan's parserSetupArg still points at a
stack-local pljs_param_state, which is why freeing the plan before that frame exits
is load-bearing rather than an optimisation; there is a comment saying so.

Unreachable plan handles are reclaimed by a GC finalizer rather than leaking until the
session ends, and an explicit plan.free() clears the handle's opaque pointer so the
finalizer cannot double-free afterwards.

A cursor now keeps its plan alive. pljs.prepare(...).cursor(...) leaves the plan
object unreachable the moment .cursor() returns, so the finalizer could
SPI_freeplan() a plan whose portal was still open and about to be re-entered by
fetch. SPI_freeplan's contract says a plan in use must not be freed.

Being straight about the evidence for that last one: it has no reproduction. Under GC
pressure, with CLOBBER_FREED_MEMORY, and under AddressSanitizer with the fix
reversed, nothing reports. The portal holds a refcount on the CachedPlan rather than
the CachedPlanSource and does not appear to dereference the plansource during a
fetch. The fix rests on the documented contract, not on a crash, and the test says so
rather than implying otherwise.

pljs_plan_free() is reachable from JavaScript and SPI_freeplan() raises on an
invalid plan pointer, so the release path is guarded too: the opaque pointer is cleared
before the free and the free runs under PG_TRY. Letting that error out would
siglongjmp past QuickJS's live frames, and clearing first means a failed free cannot
leave the handle pointing at a plan the finalizer would try again.

Commits

  • Allocate prepared plan data in CacheMemoryContext
  • Free parstate on the pljs.prepare error path
  • Reclaim prepared-statement plans with a GC finalizer
  • Free the cursor's parameter arrays and mark the portal volatile
  • Keep the prepared plan alive for the lifetime of its cursor
  • Guard the plan-release path against an error escaping into QuickJS

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
Allocate prepared plan data in CacheMemoryContext
99fb189 
pljs_prepare() saved the plan with SPI_keepplan() but allocated the surrounding
pljs_plan struct and its parameter-type array in the caller's short-lived context.  The
plan therefore outlived the state describing it, and the next use read whatever had
taken that memory.

Both now come from CacheMemoryContext, which is where a saved plan's lifetime already
points.

Note the plan's parserSetupArg still points at a stack-local pljs_param_state in the
execute path, so freeing that plan before its frame exits is load-bearing rather than an
optimisation; there is a comment at that site saying so.
@sfc-gh-okalaci
Free parstate on the pljs.prepare error path
6490774 
pljs_prepare() allocated its pljs_param_state before entering PG_TRY and freed it only
on success.  An error while preparing -- a syntax error in the query, or an unknown
type name in the argument list -- leaked it, and since the allocation had just been
moved to CacheMemoryContext it leaked for the life of the backend rather than the
transaction.

The PG_CATCH now frees it before re-throwing.

No regression test accompanies this. The leak is a few hundred bytes in a context that the surrounding transaction frees, so no observable at the SQL level distinguishes the two versions.
@sfc-gh-okalaci
Reclaim prepared-statement plans with a GC finalizer
f4bb467 
A plan prepared and never explicitly freed stayed alive until the session ended:
nothing connected the JavaScript handle becoming unreachable to SPI_freeplan().  A
function that prepares in a loop, which is a natural way to write it, accumulated saved
plans in CacheMemoryContext until the backend was restarted.

The plan handle now has a QuickJS class with a finalizer, so an unreachable handle
releases its plan when the collector runs.

pljs_plan_free() clears the handle's opaque pointer before freeing, which is what stops
the finalizer freeing the same plan a second time afterwards -- the part of this design
that is easy to get wrong.

Adds sql/pg_prepared_plan_gc.sql, which prepares many plans without freeing them,
forces a collection, and asserts memory does not grow without bound; it also frees a
plan explicitly and then forces a collection, to prove the double free cannot happen.
@sfc-gh-okalaci
Free the cursor's parameter arrays and mark the portal volatile
fd0a876 
pljs_plan_cursor() allocated the Datum and null-flag arrays for its parameters and never
freed them, so every cursor opened from a parameterised plan leaked both for the
duration of the call.

The Portal is also assigned inside PG_TRY and read after PG_END_TRY without being
volatile.  That is safe as written, because the read only happens on the path where no
longjmp occurred, but it is exactly the pattern PostgreSQL's coding conventions call
out: the compiler is entitled to keep the value in a register that siglongjmp does not
restore, so an unrelated change or a different optimisation level can make it wrong.

No regression test accompanies this. Both parts are of the same kind: the arrays are small and their context is freed with the transaction, and the volatile qualifier only changes what the compiler may keep in a register across a longjmp.
@sfc-gh-okalaci
Keep the prepared plan alive for the lifetime of its cursor
6648fdf 
pljs_plan_cursor() returns a cursor object carrying only the portal name, so the plan
handle it came from can become unreachable immediately:

    var c = pljs.prepare('select * from t where id = $1', ['int']).cursor([1]);
    while (c.fetch()) { ... }

QuickJS is refcount-primary, so the temporary plan object's count drops to zero the
moment .cursor() returns, the finalizer runs, and SPI_freeplan() frees a plan whose
portal is still open and about to be re-entered by fetch.  SPI_freeplan's own contract
says a plan in use must not be freed.

The cursor now holds a duplicated reference to the plan handle, released in close() and
in the cursor's own finalizer, so the plan cannot outlive its portal.

On the evidence: this has no reproduction.  Under GC pressure, with CLOBBER_FREED_MEMORY
via --enable-cassert, and under AddressSanitizer with the fix reversed, nothing reports.
The portal holds a refcount on the CachedPlan rather than the CachedPlanSource and does
not appear to dereference the plansource during a fetch, so the contract violation does
not surface as a crash here.  The fix rests on the documented contract; the test pins
the observable behaviour -- correct row counts across a collection and across an
explicit free -- and says plainly that it does not discriminate.
@sfc-gh-okalaci
Guard the plan-release path against an error escaping into QuickJS
2340189 
pljs_plan_free() is called from JavaScript, and SPI_freeplan() raises on an invalid plan
pointer.  Letting that error out siglongjmps past QuickJS's live stack frames, leaving
its frame list describing frames that no longer exist; the next JavaScript operation
then walks freed memory.

The opaque pointer is cleared before the free and the free runs under PG_TRY, so a
failure becomes a JavaScript exception instead of a longjmp -- and clearing first means
a failed free cannot leave the handle pointing at a plan the finalizer would try to free
again.

No regression test accompanies this. The guard covers an error raised while releasing a plan, which needs SPI_freeplan() itself to fail; there is no way to provoke that from SQL.
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