Skip to content

Fix earlier clauses' writes being invisible to later clauses (#2493) - #2495

Open
gregfelice wants to merge 1 commit into
apache:masterfrom
gregfelice:fix/2493-clause-write-visibility
Open

Fix earlier clauses' writes being invisible to later clauses (#2493)#2495
gregfelice wants to merge 1 commit into
apache:masterfrom
gregfelice:fix/2493-clause-write-visibility

Conversation

@gregfelice

Copy link
Copy Markdown
Contributor

Fixes #2493. Also fixes the visibility half of #2491, and the underlying defect behind #2490.

Problem

In a multi-part query, a clause that reads sees only the rows written by the first input row of a preceding CREATE or SET. Everything written by the remaining input rows is invisible for the rest of the statement.

SELECT * FROM cypher('g', $cypher$
  UNWIND [1, 2, 3] AS i
  CREATE (:v {id: i})
  WITH count(*) AS ignored
  MATCH (n:v)
  RETURN count(n)
$cypher$) AS (visible agtype);

Returns 1. All three vertices are persisted.

The visible unit is the command id, which covers one input row — so the symptom depends on how the write is driven, not how much it writes:

Setup Written Visible to the later MATCH Persisted
UNWIND range(1,2) AS i CREATE (:N {id:i}) 2 1 2
UNWIND range(1,8) AS i CREATE (:N {id:i}) 8 1 8
5 pre-existing, then create 3 in-statement 3 6 (5 + 1) 8
CREATE (:N),(:N),(:N) — one input row 3 3 ✅ 3

SET behaves the same way: MATCH (n:x) SET n.marked = true WITH count(*) AS ig MATCH (m:x) WHERE m.marked = true RETURN count(m) sees 1 of 3, though all 3 updates persist.

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 es_snapshot points at, and Increment_Estate_CommandId bumps curcid only once, when the clause begins. So curcid sits one step past the command id used by the first input row, and only that row's tuples satisfy cmin < curcid.

This is already documented in-tree at src/backend/executor/cypher_utils.c:248-260, where entity_exists() works around it locally with Max(saved_curcid, GetCurrentCommandId(false)). 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:

estate->es_snapshot->curcid = Max(estate->es_snapshot->curcid,
                                  GetCurrentCommandId(false));

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() 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 it shares. DELETE already synchronizes curcid explicitly (cypher_delete.c:356, :482) and was unaffected; MERGE behaved correctly in the same probes.

Testing

Full suite green — 42/42, PostgreSQL 18.4.

Behaviour changes, all verified:

Case Before After
#2493 as filed rows=1, bound=1 3, 3
#2490 driven by two input rows 2 4
Create k vertices, count visible (k=1,2,3,8) 1, 1, 1, 1 1, 2, 3, 8
5 pre-existing + 3 new 6 8
#2491 middle stage (8 vertices → edges) 0 edges 56 edges
SET 3 vertices, later MATCH on the new property 1 3
REMOVE from 3, later MATCH counts still-marked 2 0
MATCH (n:N) CREATE (:N) — must not self-feed 6 6 (unchanged)

New regression coverage in 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. Each new assertion was confirmed to fail without this change — visible reports 1 instead of 3, edges 0 instead of 6, still_marked 2 instead of 0.

I also ran the suite on unmodified master and with this change through an identical harness: the outputs are byte-identical apart from the new assertions, so nothing else moved.

Out of scope

#2491 additionally hits a separate defect, filed as #2494: the planner can place the DML CustomScan on a side of a join the executor never pulls, so the writes are skipped entirely and nothing persists. That is untouched here, and #2491 will still fail until it is fixed.

…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 apache#2491, which reports the same visibility problem, additionally hits
a separate defect tracked in apache#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.
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.

Nodes created earlier in a multi-part query are not bound by a later OPTIONAL MATCH

1 participant