Skip to content

Make the validator validate, and keep the function cache coherent - #38

Open
sfc-gh-okalaci wants to merge 3 commits into
up/06-operabilityfrom
up/07-validator-and-cache
Open

Make the validator validate, and keep the function cache coherent#38
sfc-gh-okalaci wants to merge 3 commits into
up/06-operabilityfrom
up/07-validator-and-cache

Conversation

@sfc-gh-okalaci

@sfc-gh-okalaci sfc-gh-okalaci commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Three fixes to CREATE FUNCTION and the per-session compiled-function cache. These do
change when an error surfaces, which is why they are separate from the crash fixes.

The validator never validated anything

pljs_call_validator() read fcinfo->flinfo->fn_oid — its own OID, not the OID of the
function being created, which arrives as PG_GETARG_OID(0). So it fetched its own
pg_proc row and compiled that row's prosrc: the C symbol name
pljs_call_validator, which parses as a bare JavaScript identifier. The compile always
succeeded, every invalid body was accepted at CREATE time, and the syntax error
appeared only on the first call.

Correcting the OID alone is not enough. A pljs body is a function body, not a
program, so return 42; is a syntax error at top level — validating the raw prosrc
rejects almost every valid function. The wrapper construction is therefore extracted
from pljs_compile_function() and shared, so the validator and the compiler agree by
construction rather than by two copies staying in sync. check_function_bodies = off
still skips validation, so restoring a dump whose functions reference not-yet-created
objects keeps working.

Repeated DDL grew the backend without bound

Creating or replacing a function called pljs_cache_reset(), which destroys every
per-user JSContext. QuickJS will not free a context that still has live references
into it, so the old one was not necessarily reclaimed and the next call built a fresh
one. A tight CREATE OR REPLACE FUNCTION loop reached roughly 536MB and then crashed
the backend within seconds. Dropping only the affected function's entry keeps the
context — which is per-user state, not per-function state. Measured on the same loop:
~20MB peak, no crash, and throughput rises because the common case no longer rebuilds
an interpreter per statement.

Whether the context survived is observable from JavaScript, which is what
sql/pg_targeted_invalidation.sql checks: anything held on globalThis lives in that
context, so it is still there after unrelated DDL exactly when the context was not
destroyed. Without the fix it reads (gone).

A replaced function kept running the old body elsewhere

The cache is per session and there is no syscache invalidation callback, so
CREATE OR REPLACE FUNCTION was invisible to every other backend: a session that had
already called the function ran the body it first compiled for the rest of its life.
Deploying a new definition required recycling every existing connection, and nothing
said so.

pljs_func already declared fn_xmin and fn_tid for this; they were never written
or read. They now record which pg_proc tuple an entry was compiled from and are
checked on every cache hit, which is what plpgsql does. This also settles DROP FUNCTION followed by OID reuse, where the old body could otherwise run under the new
function's name.

Commits

  • Validate the function being created, not the validator itself
  • Invalidate only the replaced function, not the whole context cache
  • Detect a stale cached function instead of running the old body

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.

pljs_call_validator() read fcinfo->flinfo->fn_oid -- its own OID.  The OID of the
function being created arrives as PG_GETARG_OID(0).  So it fetched its own pg_proc row
and compiled that row's prosrc: the C symbol name "pljs_call_validator", which parses as
a bare JavaScript identifier.  The compile always succeeded, every invalid body was
accepted at CREATE FUNCTION time, and the syntax error appeared only on the first call.

Correcting the OID alone is not enough, and this is the part worth explaining.  A pljs
body is a function *body*, not a program: `return 42;` is a syntax error at top level, so
validating the raw prosrc rejects almost every valid function.  The body has to be
wrapped exactly the way compilation wraps it, including the argument names and a
trigger's implicit variables.

The wrapper construction is therefore extracted from pljs_compile_function() and shared,
so the validator and the compiler agree by construction rather than by two copies
staying in step.  The validator compiles with JS_EVAL_FLAG_COMPILE_ONLY -- it must not
run the body -- and releases the value and the context on both paths.

check_function_bodies = off still skips validation, so restoring a dump whose functions
reference not-yet-created objects keeps working.

Adds sql/pg_validator.sql: an invalid body rejected at CREATE time, the valid shapes a
raw-prosrc check would wrongly reject (a bare return, named arguments, a trigger), and
deferral under check_function_bodies = off with the error surfacing on first call.
Creating or replacing a pljs function called pljs_cache_reset(), which walks every
per-user cache entry and destroys the per-user JSContext with it.  JS_FreeContext() does
not free a context that still has live references into it, so the old context was not
necessarily reclaimed -- and the next call built a fresh one.  A backend doing repeated
DDL therefore accumulated contexts on the QuickJS heap, where PostgreSQL's memory
accounting cannot see them.

Measured on a tight CREATE OR REPLACE FUNCTION loop: around 536MB of resident memory and
then a SIGSEGV, within seconds.  Afterwards, roughly 20MB and no crash, with throughput
up because the common case no longer rebuilds an interpreter per statement.

pljs_cache_function_remove() drops just the entry for the OID being replaced, from every
user's cache, releasing the compiled function and the cached source.  The context
survives, which is correct: it is per-user state, not per-function state.
The compiled-function cache is per session and pljs registers no syscache invalidation
callback, so the only invalidation was the one the validator performs in the backend
issuing the DDL.  CREATE OR REPLACE FUNCTION was therefore invisible to every other
session: one that had already called the function kept running the body it first
compiled, for the rest of its life, however many times the function was replaced.

Deploying a new definition meant recycling every existing connection, and nothing said
so -- which makes it look like a failed deploy rather than a caching rule.

pljs_func already declared fn_xmin and fn_tid for this purpose; neither was ever written
or read.  They now record which pg_proc tuple an entry was compiled from, and every
cache hit compares them against the tuple the caller already holds.  CREATE OR REPLACE
writes a new tuple version while keeping the OID, so a mismatch is exactly the signal
that the compiled copy is stale.  This is what plpgsql does; see plpgsql_compile().

The check lives inside pljs_cache_function_find() rather than at its call sites, so
neither lookup can be changed later without it.

It also settles DROP FUNCTION followed by OID reuse, where a new function inherits a
dropped one's OID: the tuple differs, so the mismatch is caught rather than the old body
running under the new name.

Adds sql/pg_cross_backend_invalidation.sql, which replaces a function from a second
backend and asserts this one sees the new body, for both the direct call path and
pljs.find_function().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant