jok-carbonarc commented on PR #2581:
URL: https://github.com/apache/age/pull/2581#issuecomment-6450541137
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`, #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):
```sql
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):
- applied on `PG18` (with #2581), it gives 800000 at every timeout (20 of
20);
- our build with it passes `make installcheck`, 44 of 44;
- after 10 canceled loads on one backend, `pg_backend_memory_contexts` shows
no AGE edge table left, and the backend's memory stays flat.
```diff
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
