From 182ac72c6b3d9e068f138f6e8df3899665f13b15 Mon Sep 17 00:00:00 2001 From: Greg Felice Date: Mon, 3 Aug 2026 15:55:53 -0400 Subject: [PATCH] Fix earlier clauses' writes being invisible to later clauses (#2493) In a multi-part query, a clause that reads saw only the rows written by the first input row of a preceding CREATE or SET. Everything written by the remaining input rows stayed invisible for the rest of the statement, so UNWIND [1, 2, 3] AS i CREATE (:v {id: i}) WITH count(*) AS ignored MATCH (n:v) RETURN count(n) returned 1 instead of 3, while all three vertices were persisted. Root cause: entities are written with the global command id (insert_entity_tuple for CREATE, the cid in update_entity_tuple for SET), and CommandCounterIncrement() advances that id once per input row. The executor's snapshot does not follow it -- CommandCounterIncrement() updates the current and secondary snapshots, not the pushed one that es_snapshot points at, and Increment_Estate_CommandId bumps curcid only once, when the clause begins. So curcid stayed one step past the command id used by the first input row, and only that row's tuples satisfied cmin < curcid. The visible unit is the command id, which covers one input row: three vertices created from a single input row were all visible, while three created from three input rows yielded one. entity_exists() already worked around this locally; nothing applied the same correction to ordinary MATCH scans. Fix: when a CREATE or SET clause reaches the end of its input, raise es_snapshot->curcid to the global command id, making everything the clause wrote visible to the clauses that read after it. Doing this at end of input rather than after each row is what preserves the existing protection against a clause seeing its own writes: by that point the subtree is exhausted, so raising curcid cannot feed a written row back into the pattern that wrote it. Max() is used because Increment_Estate_CommandId can push curcid above the global command id, and lowering it would hide tuples that are already visible. REMOVE is covered by the SET path, which it shares. DELETE already synchronizes curcid explicitly and was unaffected. Adds regression coverage to cypher_create and cypher_set for visibility of every written row, for writes from an earlier clause driving a later one, and for a write clause still not seeing its own writes. All are verified to fail without this change. Note that #2491, which reports the same visibility problem, additionally hits a separate defect tracked in #2494, where the planner can place the DML CustomScan on a side of a join the executor never pulls, so the writes are skipped entirely. That is not addressed here. --- regress/expected/cypher_create.out | 109 +++++++++++++++++++++++++++ regress/expected/cypher_set.out | 90 ++++++++++++++++++++++ regress/sql/cypher_create.sql | 61 +++++++++++++++ regress/sql/cypher_set.sql | 49 ++++++++++++ src/backend/executor/cypher_create.c | 23 ++++++ src/backend/executor/cypher_set.c | 23 ++++++ 6 files changed, 355 insertions(+) diff --git a/regress/expected/cypher_create.out b/regress/expected/cypher_create.out index 2388af8a0..efb77ed59 100644 --- a/regress/expected/cypher_create.out +++ b/regress/expected/cypher_create.out @@ -906,6 +906,115 @@ $$) as (m agtype); {"id": 281474976710689, "label": "", "properties": {}}::vertex (1 row) +-- +-- Issue 2493: a clause that reads must see everything a preceding clause +-- wrote, not only the rows written by that clause's first input row. +-- +SELECT create_graph('issue_2493'); +NOTICE: graph "issue_2493" has been created + create_graph +-------------- + +(1 row) + +-- all three created vertices must be visible to the later MATCH +SELECT * FROM cypher('issue_2493', $$ + UNWIND [1, 2, 3] AS i + CREATE (:vis {id: i}) + WITH count(*) AS ignored + MATCH (n:vis) + RETURN count(n) +$$) as (visible agtype); + visible +--------- + 3 +(1 row) + +-- pre-existing and newly created vertices are both visible +SELECT * FROM cypher('issue_2493', $$ CREATE (:pre {id: 0}) $$) as (a agtype); + a +--- +(0 rows) + +SELECT * FROM cypher('issue_2493', $$ + UNWIND [1, 2] AS i + CREATE (:pre {id: i}) + WITH count(*) AS ignored + MATCH (n:pre) + RETURN count(n) +$$) as (visible agtype); + visible +--------- + 3 +(1 row) + +-- as originally reported: OPTIONAL MATCH binds every created vertex +SELECT * FROM cypher('issue_2493', $$ + UNWIND [1, 2, 3] AS i + CREATE (:opt {id: i}) + WITH count(*) AS ignored + OPTIONAL MATCH (a:opt) + RETURN count(*) AS rows, count(a) AS bound +$$) as (rows agtype, bound agtype); + rows | bound +------+------- + 3 | 3 +(1 row) + +-- vertices written earlier drive a later CREATE (6 ordered pairs of 3) +SELECT * FROM cypher('issue_2493', $$ + UNWIND [1, 2, 3] AS i + CREATE (:src {id: i}) + WITH count(*) AS ignored + MATCH (a:src), (b:src) WHERE a.id <> b.id + CREATE (a)-[:rel]->(b) +$$) as (a agtype); + a +--- +(0 rows) + +SELECT * FROM cypher('issue_2493', $$ + MATCH ()-[r:rel]->() RETURN count(r) +$$) as (edges agtype); + edges +------- + 6 +(1 row) + +-- a CREATE must still not see its own writes: this creates 3 vertices, not +-- an unbounded number +SELECT * FROM cypher('issue_2493', $$ + MATCH (n:src) + CREATE (:copy {from: n.id}) +$$) as (a agtype); + a +--- +(0 rows) + +SELECT * FROM cypher('issue_2493', $$ + MATCH (n:copy) RETURN count(n) +$$) as (copies agtype); + copies +-------- + 3 +(1 row) + +SELECT drop_graph('issue_2493', true); +NOTICE: drop cascades to 8 other objects +DETAIL: drop cascades to table issue_2493._ag_label_vertex +drop cascades to table issue_2493._ag_label_edge +drop cascades to table issue_2493.vis +drop cascades to table issue_2493.pre +drop cascades to table issue_2493.opt +drop cascades to table issue_2493.src +drop cascades to table issue_2493.rel +drop cascades to table issue_2493.copy +NOTICE: graph "issue_2493" has been dropped + drop_graph +------------ + +(1 row) + -- -- Clean up -- diff --git a/regress/expected/cypher_set.out b/regress/expected/cypher_set.out index 239234ed6..b37d3a45f 100644 --- a/regress/expected/cypher_set.out +++ b/regress/expected/cypher_set.out @@ -1227,6 +1227,96 @@ $$) AS (a agtype); {"id": 5066549580791809, "label": "TestE2", "properties": {"pathRels": [{"id": 5348024557502465, "label": "E2REL", "end_id": 5066549580791810, "start_id": 5066549580791809, "properties": {}}::edge], "pathNodes": [{"id": 5066549580791809, "label": "TestE2", "properties": {}}::vertex, {"id": 5066549580791810, "label": "TestE2", "properties": {}}::vertex]}}::vertex (1 row) +-- +-- Issue 2493: a clause that reads must see every update a preceding SET +-- made, not only the updates from that clause's first input row. +-- +SELECT create_graph('issue_2493_set'); +NOTICE: graph "issue_2493_set" has been created + create_graph +-------------- + +(1 row) + +SELECT * FROM cypher('issue_2493_set', $$ + UNWIND [1, 2, 3] AS i CREATE (:x {id: i}) +$$) as (a agtype); + a +--- +(0 rows) + +-- all three updates must be visible to the later MATCH +SELECT * FROM cypher('issue_2493_set', $$ + MATCH (n:x) SET n.marked = true + WITH count(*) AS ignored + MATCH (m:x) WHERE m.marked = true + RETURN count(m) +$$) as (visible agtype); + visible +--------- + 3 +(1 row) + +-- the same for REMOVE, which shares the SET executor +SELECT * FROM cypher('issue_2493_set', $$ + MATCH (n:x) REMOVE n.marked + WITH count(*) AS ignored + MATCH (m:x) WHERE m.marked = true + RETURN count(m) +$$) as (still_marked agtype); + still_marked +-------------- + 0 +(1 row) + +-- updates written earlier drive a later CREATE +SELECT * FROM cypher('issue_2493_set', $$ + MATCH (n:x) SET n.ready = true + WITH count(*) AS ignored + MATCH (m:x) WHERE m.ready = true + CREATE (:derived {from: m.id}) +$$) as (a agtype); + a +--- +(0 rows) + +SELECT * FROM cypher('issue_2493_set', $$ + MATCH (d:derived) RETURN count(d) +$$) as (derived agtype); + derived +--------- + 3 +(1 row) + +-- a SET must still not see its own writes: this updates each vertex once +SELECT * FROM cypher('issue_2493_set', $$ + MATCH (n:x) SET n.pass = 1 RETURN count(*) +$$) as (updated agtype); + updated +--------- + 3 +(1 row) + +SELECT * FROM cypher('issue_2493_set', $$ + MATCH (m:x) WHERE m.pass = 1 RETURN count(m) +$$) as (persisted agtype); + persisted +----------- + 3 +(1 row) + +SELECT drop_graph('issue_2493_set', true); +NOTICE: drop cascades to 4 other objects +DETAIL: drop cascades to table issue_2493_set._ag_label_vertex +drop cascades to table issue_2493_set._ag_label_edge +drop cascades to table issue_2493_set.x +drop cascades to table issue_2493_set.derived +NOTICE: graph "issue_2493_set" has been dropped + drop_graph +------------ + +(1 row) + -- -- Clean up -- diff --git a/regress/sql/cypher_create.sql b/regress/sql/cypher_create.sql index 0093dc449..38dbb083f 100644 --- a/regress/sql/cypher_create.sql +++ b/regress/sql/cypher_create.sql @@ -475,6 +475,67 @@ SELECT * FROM cypher('cypher_create', $$ CREATE (n), (m) WITH n AS r CREATE (m) RETURN m $$) as (m agtype); +-- +-- Issue 2493: a clause that reads must see everything a preceding clause +-- wrote, not only the rows written by that clause's first input row. +-- +SELECT create_graph('issue_2493'); + +-- all three created vertices must be visible to the later MATCH +SELECT * FROM cypher('issue_2493', $$ + UNWIND [1, 2, 3] AS i + CREATE (:vis {id: i}) + WITH count(*) AS ignored + MATCH (n:vis) + RETURN count(n) +$$) as (visible agtype); + +-- pre-existing and newly created vertices are both visible +SELECT * FROM cypher('issue_2493', $$ CREATE (:pre {id: 0}) $$) as (a agtype); + +SELECT * FROM cypher('issue_2493', $$ + UNWIND [1, 2] AS i + CREATE (:pre {id: i}) + WITH count(*) AS ignored + MATCH (n:pre) + RETURN count(n) +$$) as (visible agtype); + +-- as originally reported: OPTIONAL MATCH binds every created vertex +SELECT * FROM cypher('issue_2493', $$ + UNWIND [1, 2, 3] AS i + CREATE (:opt {id: i}) + WITH count(*) AS ignored + OPTIONAL MATCH (a:opt) + RETURN count(*) AS rows, count(a) AS bound +$$) as (rows agtype, bound agtype); + +-- vertices written earlier drive a later CREATE (6 ordered pairs of 3) +SELECT * FROM cypher('issue_2493', $$ + UNWIND [1, 2, 3] AS i + CREATE (:src {id: i}) + WITH count(*) AS ignored + MATCH (a:src), (b:src) WHERE a.id <> b.id + CREATE (a)-[:rel]->(b) +$$) as (a agtype); + +SELECT * FROM cypher('issue_2493', $$ + MATCH ()-[r:rel]->() RETURN count(r) +$$) as (edges agtype); + +-- a CREATE must still not see its own writes: this creates 3 vertices, not +-- an unbounded number +SELECT * FROM cypher('issue_2493', $$ + MATCH (n:src) + CREATE (:copy {from: n.id}) +$$) as (a agtype); + +SELECT * FROM cypher('issue_2493', $$ + MATCH (n:copy) RETURN count(n) +$$) as (copies agtype); + +SELECT drop_graph('issue_2493', true); + -- -- Clean up -- diff --git a/regress/sql/cypher_set.sql b/regress/sql/cypher_set.sql index e745d5d6e..4a715a51d 100644 --- a/regress/sql/cypher_set.sql +++ b/regress/sql/cypher_set.sql @@ -542,6 +542,55 @@ SELECT * FROM cypher('issue_1884', $$ RETURN a $$) AS (a agtype); +-- +-- Issue 2493: a clause that reads must see every update a preceding SET +-- made, not only the updates from that clause's first input row. +-- +SELECT create_graph('issue_2493_set'); + +SELECT * FROM cypher('issue_2493_set', $$ + UNWIND [1, 2, 3] AS i CREATE (:x {id: i}) +$$) as (a agtype); + +-- all three updates must be visible to the later MATCH +SELECT * FROM cypher('issue_2493_set', $$ + MATCH (n:x) SET n.marked = true + WITH count(*) AS ignored + MATCH (m:x) WHERE m.marked = true + RETURN count(m) +$$) as (visible agtype); + +-- the same for REMOVE, which shares the SET executor +SELECT * FROM cypher('issue_2493_set', $$ + MATCH (n:x) REMOVE n.marked + WITH count(*) AS ignored + MATCH (m:x) WHERE m.marked = true + RETURN count(m) +$$) as (still_marked agtype); + +-- updates written earlier drive a later CREATE +SELECT * FROM cypher('issue_2493_set', $$ + MATCH (n:x) SET n.ready = true + WITH count(*) AS ignored + MATCH (m:x) WHERE m.ready = true + CREATE (:derived {from: m.id}) +$$) as (a agtype); + +SELECT * FROM cypher('issue_2493_set', $$ + MATCH (d:derived) RETURN count(d) +$$) as (derived agtype); + +-- a SET must still not see its own writes: this updates each vertex once +SELECT * FROM cypher('issue_2493_set', $$ + MATCH (n:x) SET n.pass = 1 RETURN count(*) +$$) as (updated agtype); + +SELECT * FROM cypher('issue_2493_set', $$ + MATCH (m:x) WHERE m.pass = 1 RETURN count(m) +$$) as (persisted agtype); + +SELECT drop_graph('issue_2493_set', true); + -- -- Clean up -- diff --git a/src/backend/executor/cypher_create.c b/src/backend/executor/cypher_create.c index 36ef61b32..64a6edb98 100644 --- a/src/backend/executor/cypher_create.c +++ b/src/backend/executor/cypher_create.c @@ -244,6 +244,29 @@ static TupleTableSlot *exec_cypher_create(CustomScanState *node) */ if (!used) { + /* + * This clause is finished. Make everything it wrote visible to the + * clauses that read after it. + * + * Entities are inserted with the global command id (see + * insert_entity_tuple), and CommandCounterIncrement() above advances + * that id once per input row. es_snapshot->curcid does not follow it, + * so without this it stays one step past the command id used by the + * first input row, and everything written by the remaining rows is + * invisible for the rest of the statement (issue #2493). + * + * Syncing here, rather than after each row, is what keeps the clause + * from seeing its own writes: by this point the subtree is exhausted, + * so raising curcid cannot feed a created row back into the pattern + * that created it. + * + * Max() because Increment_Estate_CommandId can push curcid above the + * global command id, and lowering it would hide tuples that are + * already visible. + */ + estate->es_snapshot->curcid = Max(estate->es_snapshot->curcid, + GetCurrentCommandId(false)); + return NULL; } diff --git a/src/backend/executor/cypher_set.c b/src/backend/executor/cypher_set.c index 7a0d48f0c..9b6bf51c5 100644 --- a/src/backend/executor/cypher_set.c +++ b/src/backend/executor/cypher_set.c @@ -883,6 +883,29 @@ static TupleTableSlot *exec_cypher_set(CustomScanState *node) if (TupIsNull(slot)) { + /* + * This clause is finished. Make everything it wrote visible to the + * clauses that read after it. + * + * Updated tuples are written with the global command id (see the cid + * in update_entity_tuple), and CommandCounterIncrement() advances that + * id once per input row. es_snapshot->curcid does not follow it, so + * without this it stays one step past the command id used by the first + * input row, and every later row's update is invisible for the rest of + * the statement. Same defect as the CREATE path (issue #2493). + * + * Syncing here, rather than after each row, is what keeps the clause + * from seeing its own writes: by this point the subtree is exhausted, + * so raising curcid cannot feed an updated row back into the pattern + * that updated it. + * + * Max() because Increment_Estate_CommandId can push curcid above the + * global command id, and lowering it would hide tuples that are + * already visible. + */ + estate->es_snapshot->curcid = Max(estate->es_snapshot->curcid, + GetCurrentCommandId(false)); + return NULL; }