Skip to content

Do not read the catalog in is_age_extension_exists() outside a live transaction - #2581

Merged
MuhammadTahaNaveed merged 1 commit into
apache:masterfrom
SpongeRobert:fix-extension-check-outside-transaction
Oct 11, 2026
Merged

MuhammadTahaNaveed merged 1 commit into
apache:masterfrom
SpongeRobert:fix-extension-check-outside-transaction

Conversation

@SpongeRobert

Copy link
Copy Markdown
Contributor

Fixes #2580.

With age in shared_preload_libraries, the post_parse_analyze hook runs for the ROLLBACK of a failed transaction too. If the caches of that backend were reset in the meantime, is_age_extension_exists() reads pg_extension (and pg_class) in TBLOCK_ABORT, after the resource owner was released: ResourceOwnerEnlarge called after release started. The locks leak into the PGPROC, and every later backend on it fails at connect with failed to re-find shared lock object until the server restarts. The mechanism, a gdb stack of the failing path and a standalone reproduction are in #2580.

The fix returns false outside a live transaction and caches nothing, so the next live transaction asks the catalog again. In an aborted transaction only the statements that leave it reach the parser, and AGE has nothing to analyse in them.

Verified on PostgreSQL 18.6, master at fa109ef1dd:

No regression test is added: the failure needs a backend held stopped while another session overflows the shared invalidation queue, and then every free PGPROC taken, which pg_regress cannot express. The same change applies cleanly to PG18 (e43dc1a), where it was verified the same way.

🤖 Generated with Claude Code

…ransaction

With age in shared_preload_libraries, post_parse_analyze runs for every
statement, the ROLLBACK of a failed transaction included. When the caches of
that backend were reset (the shared invalidation queue overflowed while it
was not reading), the cached answer of is_age_extension_exists() is gone and
get_extension_oid() misses the catcache: pg_extension and pg_class are opened
in TBLOCK_ABORT, after the resource owner of the transaction was released.
The locks are granted on the fast path and ResourceOwnerEnlarge() then fails
("called after release started"). Nothing releases those locks any more; the
backend exits with them in its PGPROC, and every later backend on that PGPROC
fails at connect with "failed to re-find shared lock object" until the
server restarts. With --enable-cassert the same path trips
Assert(IsTransactionState()) in AssertCouldGetRelation().

Return false outside a live transaction and cache nothing, so the next live
transaction asks the catalog again. Only the statements that leave a failed
transaction reach the parser there, and AGE has nothing to analyse in them.

Fixes apache#2580.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@jok-carbonarc

Copy link
Copy Markdown

Thanks for the fix. Could #2581 go to the PG18 branch in the next batch from master, with a 1.8.1 tag? @jrgemignani

PG18 is still at e43dc1a (1.8.0), so PG18 users have no release with it. The commit cherry-picks cleanly onto e43dc1a.

We run 1.8.0 on PostgreSQL 18.6 with age in shared_preload_libraries and reproduced #2580 with repro.sh from the issue, ten fresh servers per row:

build rollback quit (control) rollback, failure = SET LOCAL statement_timeout expiring inside BEGIN, CREATE EXTENSION age done
1.8.0 10/10 reproduced 0/10 10/10 reproduced
1.8.0 + #2581 0/10 0/10 0/10

The last column is a common client pattern: a per-transaction statement timeout fires and the driver sends ROLLBACK. So any PG18 application with timeouts inside explicit transactions can poison PGPROCs, given enough invalidation traffic on the server.

#2577 (B-tree strategy numbers) is another master-only fix PG18 users would want in the same batch.

@jok-carbonarc

Copy link
Copy Markdown

A follow-up for the same PG18 batch: please also consider the detached graph-context build from #2561 (3eeb20bd). On PG18 it fixes a silent wrong-result bug.

The bug (PG18 at e43dc1a, 1.8.0; measured on our build of it, which adds local parser patches, with and without #2581. manage_GRAPH_global_contexts() is unchanged by them):

  • manage_GRAPH_global_contexts() links a new GRAPH_global_context into global_graph_contexts before load_GRAPH_global_hashtables() runs.
  • If the load is interrupted (statement_timeout, a cancel), the half-loaded context stays on the list.
  • Once the graph's version counter is > 0, is_ggctx_invalid() keeps that context valid.
  • So every later VLE in that backend reads a partial graph and returns wrong results with no error, until the graph is written or the connection closes.
  • On master, Add RLS and SELECT privilege enforcement to global graph #2561 builds the context detached and frees it on failure, which fixes this as a side effect of the RLS work.

Reproduction (PG 18.6, shared_preload_libraries = 'age', fresh server):

CREATE EXTENSION age; SET search_path = ag_catalog, "$user", public;
SELECT create_graph('t2f');
SELECT * FROM cypher('t2f', $$ UNWIND range(0, 199999) AS i CREATE (:N {id: i}) $$) AS (a agtype);
SELECT * FROM cypher('t2f', $$ CREATE (:L0)-[:L]->(:L0) $$) AS (a agtype);          -- creates label L
SELECT * FROM cypher('t2f', $$ MATCH (x:L0) DETACH DELETE x $$) AS (a agtype);
CREATE TEMP TABLE v AS SELECT id, (row_number() OVER (ORDER BY id) - 1)::int AS k FROM t2f."N";
INSERT INTO t2f."L" (start_id, end_id, properties)
  SELECT a.id, b.id, '{}'::agtype FROM generate_series(0::bigint, 799999) j
  JOIN v a ON a.k = (j * 7919) % 200000 JOIN v b ON b.k = (j * 104729 + 13) % 200000;

-- then, on a NEW connection:
SET search_path = ag_catalog, "$user", public;
SET statement_timeout = '150ms';   -- must land inside the first VLE's graph load (about 300 ms here)
SELECT count(*) FROM cypher('t2f', $$ MATCH (a:N)-[:L*1..1]->(b:N) RETURN b $$) AS (b agtype);   -- canceled
SET statement_timeout = 0;
SELECT count(*) FROM cypher('t2f', $$ MATCH (a:N)-[:L*1..1]->(b:N) RETURN b $$) AS (b agtype);   -- should be 800000

The second count is wrong on that connection, and right on a fresh one. Four runs, each a fresh server, on our builds with and without #2581:

timeout (ms) none 20 50 100 150 200 250 300 600 1000
without the hunk 800000 800000 0 42022–126206 89896–222904 280976–374952 408728–445720 445672–528224 (3 of 4) 800000 800000
with the hunk below 800000 at every timeout, 2 runs

The hunk (only #2561's detached build and free-on-failure, without the RLS changes):

diff --git a/src/backend/utils/adt/age_global_graph.c b/src/backend/utils/adt/age_global_graph.c
--- a/src/backend/utils/adt/age_global_graph.c
+++ b/src/backend/utils/adt/age_global_graph.c
@@ -1105,20 +1105,16 @@ GRAPH_global_context *manage_GRAPH_global_contexts(char *graph_name,
         curr_ggctx = curr_ggctx->next;
     }
 
-    /* otherwise, we need to create one and possibly attach it */
+    /*
+     * Otherwise, we need to build one. Build it DETACHED from the list and
+     * attach it only after a successful load (upstream #2561): a load stopped
+     * by an error - statement_timeout, a cancel - would otherwise leave a
+     * half-loaded context on the list, and is_ggctx_invalid() keeps judging
+     * it valid while the graph's version counter is unchanged, so every later
+     * VLE on this backend would read a partial graph without an error.
+     */
     new_ggctx = palloc0(sizeof(GRAPH_global_context));
-
-    if (global_graph_contexts != NULL)
-    {
-        new_ggctx->next = global_graph_contexts;
-    }
-    else
-    {
-        new_ggctx->next = NULL;
-    }
-
-    /* set the global context variable */
-    global_graph_contexts = new_ggctx;
+    new_ggctx->next = NULL;
 
     /* set the graph name and oid */
     new_ggctx->graph_name = pstrdup(graph_name);
@@ -1135,10 +1131,31 @@ GRAPH_global_context *manage_GRAPH_global_contexts(char *graph_name,
     /* initialize our vertices list */
     new_ggctx->vertices = NULL;
 
-    /* build the hashtables for this graph */
-    create_GRAPH_global_hashtables(new_ggctx);
-    load_GRAPH_global_hashtables(new_ggctx);
-    freeze_GRAPH_global_hashtables(new_ggctx);
+    /* build the hashtables for this graph (detached from the list) */
+    PG_TRY();
+    {
+        create_GRAPH_global_hashtables(new_ggctx);
+        load_GRAPH_global_hashtables(new_ggctx);
+        freeze_GRAPH_global_hashtables(new_ggctx);
+    }
+    PG_CATCH();
+    {
+        /*
+         * Free the partially-built context and re-throw. This is safe on a
+         * partial build: a vertex id is added to the vertices list only after
+         * its vertex_hashtable entry exists, the edge arrays are freed only
+         * when allocated, and the edge_table lives in its own memory context;
+         * nothing used here re-throws.
+         */
+        (void) free_specific_GRAPH_global_context(new_ggctx);
+        MemoryContextSwitchTo(oldctx);
+        PG_RE_THROW();
+    }
+    PG_END_TRY();
+
+    /* attach the fully-built context to the list */
+    new_ggctx->next = global_graph_contexts;
+    global_graph_contexts = new_ggctx;
 
 
     /* switch back to the previous memory context */

free_specific_GRAPH_global_context() is safe on a partially built context on PG18 too:

  • a vertex id joins the vertices list only after its hash entry exists;
  • vea_free() checks for NULL;
  • the edge table lives in its own memory context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants