gregfelice commented on issue #2494: URL: https://github.com/apache/age/issues/2494#issuecomment-5171538278
Correction to my previous comment, and a scope finding. I said there that the same hazard applies to `SET`, `DELETE` and `MERGE` and that "the fix is the same one-liner in each `begin_*` function". **That is wrong.** I have now tried it, and registering the other three the same way takes the regression suite from 42/42 to 14 failures, in two distinct modes. ## Results | Clause | Registered in `es_auxmodifytables` | Outcome | |---|---|---| | `CREATE` | safe | 42/42 | | `MERGE` | **duplicates data** | `cypher_merge`, `cypher_vle` fail — entities created twice (2 rows where 1 expected, 6 where 3) | | `SET` + `DELETE` | **segfault** | backend killed by signal 11, core dumped; 12 further tests fall over behind it | The crash reproduces on a statement carrying both a DELETE and a SET: ```cypher MATCH (n)-[e]->(m) DETACH DELETE n SET e.i = 1 RETURN e ``` ``` LOG: client backend (PID 3650008) was terminated by signal 11: Segmentation fault ``` That query is already in `regress/sql/cypher_delete.sql`, so the suite catches it immediately. ## Why My earlier claim that re-running an exhausted node is a no-op holds for `CREATE` only — I had verified it there and generalised it without checking, which was the error. `ExecPostprocessPlan()` re-runs everything on the list unconditionally, so registration is safe only for a node that is genuinely idempotent on re-entry. `exec_cypher_merge` re-processes and creates duplicates; the `SET`/`DELETE` paths dereference state that is no longer valid by then. ## What a real extension needs A per-node "finished" flag: set on every path where exec returns `NULL`, checked on entry so the drain is a true no-op for an already-exhausted node. That means a struct field plus guards on every exit path — five in `exec_cypher_merge` alone — across four executors, each needing separate verification. It also opens a path that has never run before: a `SET` or `DELETE` node that was skipped entirely and is then drained at `ExecutorFinish`. That is a much bigger change than the registration itself, and I would not want it rushed alongside a release. ## Revised proposal Ship `CREATE` only, as a bounded fix for the reported case, and treat the other three as a separate piece of work behind the flag. `CREATE` is where the reported symptom occurs (this issue and #2491), and it is the clause most exposed in practice, because it is the one that populates a previously empty label table — which is the condition that lets the executor skip the write in the first place. For whoever picks up the follow-up, the `LIMIT`-over-write numbers I measured for each clause, 5 matched rows and `LIMIT 1`: | Clause | master today | with registration | |---|---|---| | `CREATE` | 1 created | 5 created | | `SET` | 1 updated | 5 updated | | `DELETE` | 1 deleted | **all 5 deleted** | | `MERGE` | 5 | 5 (already eager) | `DELETE` going from "deletes one" to "deletes all matched" deserves an explicit decision of its own, separate from the `CREATE` question I asked above. The `CREATE`-only branch is ready to push whenever there is a direction on that question. -- 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]
