gregfelice commented on issue #2494:
URL: https://github.com/apache/age/issues/2494#issuecomment-5171231812
I have a working fix for this, but it carries a semantics change that I
would rather have agreement on before opening a PR — particularly with 1.8.0
mid-release.
**Environment:** `master` @ 80141740, PostgreSQL 18.4, built from source.
## Proposed fix
PostgreSQL already solves exactly this problem for data-modifying CTEs.
`ExecPostprocessPlan()` runs everything on `EState.es_auxmodifytables` to
completion once the main plan is done, specifically so that writes do not
depend on the main query pulling from them. It iterates that list as plain
`PlanState *` and calls `ExecProcNode`, so AGE's DML CustomScan can join it
directly:
```c
/* in begin_cypher_create(), after Increment_Estate_CommandId() */
if (!(eflags & EXEC_FLAG_EXPLAIN_ONLY))
{
estate->es_auxmodifytables = lappend(estate->es_auxmodifytables, node);
}
```
Re-running an already-exhausted node is a no-op — its subtree returns NULL
immediately. `EXPLAIN` without `ANALYZE` never reaches `ExecutorFinish`, hence
the guard.
Result:
| Query | Writes before | Writes after |
|---|---|---|
| Minimal repro in this issue | 0 nodes, 0 edges | **4 nodes, 2 edges** |
| #2491 as filed | 0 nodes, 0 edges | **8 nodes, 56 edges** |
Full regression suite: **42/42**.
## What it does not fix
The writes now happen, but they happen at `ExecutorFinish`, after the
reading clause has already run. So the *query results* are unchanged: this
issue's repro still returns `0`, and #2491 still returns `"FAIL"`. Only the
silent data loss is fixed.
Making the results correct means getting the planner to order the write
before the read — the DML CustomScan would have to be somewhere the executor
cannot skip *and* cannot schedule after the reading scan. That is a much larger
change and I have not attempted it.
So this is a fix for the title of the issue, not for the full openCypher
semantics behind it.
## The semantics change to agree on
Draining to completion also changes what `LIMIT` does over a write clause.
Against 5 `:src` vertices:
```cypher
MATCH (n:src) CREATE (:made {from: n.id}) RETURN n LIMIT 1
```
| | `:made` vertices created |
|---|---|
| master today | 1 |
| with this change | **5** |
Measured both ways on the same build, not inferred.
I think 5 is the correct answer — it is what Neo4j does (the write clause
processes every incoming row; `LIMIT` bounds the returned rows, not the
writes), and it matches PostgreSQL's rule that data-modifying CTEs always
execute to completion regardless of what the primary query reads. By that
reading, today's behaviour is the same defect as this issue wearing a different
hat: a write silently skipped because the executor stopped pulling.
But it is a user-visible change, no test in the suite asserts either
behaviour, and it would land during a release. Hence asking rather than filing.
## Scope
The same hazard applies to `SET`, `DELETE`, and `MERGE` — each is a
CustomScan the planner can place on a skippable side of a join. The fix is the
same one-liner in each `begin_*` function. I held off pending direction on the
above.
@jrgemignani @MuhammadTahaNaveed — three questions:
1. Is the `LIMIT`-over-write change acceptable, or should writes stay
bounded by what the plan pulls?
2. Land this for 1.8.0, or hold for after the release?
3. All four DML clauses in one PR, or `CREATE` first?
Happy to open the PR immediately once there is a direction.
--
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]
