gregfelice commented on PR #2347:
URL: https://github.com/apache/age/pull/2347#issuecomment-4289997250
@MuhammadTahaNaveed — both crashes fixed in `f0ed4c95`. Also addressed the
`-Wmaybe-uninitialized` warning on `new_property_value`.
**Root cause (parser, not executor).** When MERGE has a previous clause,
`transform_cypher_merge` takes the lateral-left-join path via
`transform_merge_make_lateral_join`. That helper called
`addRangeTableEntryForJoin` with `nscolumns=NULL`, leaving the join
ParseNamespaceItem's `p_nscolumns` unset. The existing code paths never tripped
on it because `RETURN` (and other clauses after MERGE) get processed in a fresh
namespace built by `handle_prev_clause`. ON CREATE / ON MATCH SET, by contrast,
is transformed in-line while the lateral-join's half-initialised nsitem is
still the top of `pstate->p_namespace`, so `transform_cypher_set_item_list` →
`colNameToVar` → `scanNSItemForColumn` dereferenced `p_nscolumns[attnum-1]` and
the backend segfaulted on any ON SET whose RHS referenced a bound variable.
(Parse-time crash — cassert PG shows a plain SIGSEGV, no assertion. GDB bt
confirms: `scanNSItemForColumn` at `parse_relation.c:746`, down through
`colNameToVar` → `transform_column_ref_for_indirection` →
`transform_cypher_set_item_list` → `transform_cypher_merge`.)
**Fix.** Populate the join's `p_nscolumns` from `res_colvars` in
`transform_merge_make_lateral_join`. One subtlety caught during testing: the
`Var` we end up producing for a bound entity lives inside `prop_expr`, which is
opaque to the planner, so it is **not** rewritten to match the plan's output
slots. At `ExecEvalScalarVar` time only `varattno` is consulted, and the
scantuple layout mirrors the join's `eref->colnames` (via
`make_target_list_from_join`). So the nscolumns must use the join rtindex and
the 1-based position in `eref->colnames`, not the underlying RTE's
varno/varattno — otherwise `Var(l_rte, 1)` and `Var(r_rte, 1)` both index
`scantuple[0]` and `b.id` silently evaluates to `a.id`. That variant wasn't in
your report but would have masqueraded as a correctness bug; caught it with
`MATCH (a) MERGE (b {id: N}) ON CREATE SET b.x = b.id` once the crash was gone.
**Regression coverage added to `cypher_merge`:**
- Outer-ref RHS (`b.source_name = a.name`)
- Self-ref RHS with a previous clause (the silent-wrong-value variant)
- UNWIND-driven MERGE with self-ref (your second reproducer)
- Multi-item ON CREATE SET mixing outer-ref / self-ref / literal
- ON MATCH SET with a variable RHS on the match-branch second run
Cassert installcheck green — 33/33 AGE tests pass.
Both of your reproducers verbatim now return the expected values. Ready for
another look.
--
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]
