You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@age.apache.org by "gregfelice (via GitHub)" <gi...@apache.org> on 2026/04/22 00:16:19 UTC
[I] Latent NULL p_nscolumns in OPTIONAL MATCH lateral-join ParseNamespaceItem [age]
gregfelice opened a new issue, #2404:
URL: https://github.com/apache/age/issues/2404
Follow-up from #2347. While tracing the MERGE ON CREATE/MATCH SET crash, I confirmed that `transform_cypher_optional_match_clause` at `src/backend/parser/cypher_clause.c:3238` carries the same defect that caused #2347:
```c
jnsitem = addRangeTableEntryForJoin(pstate,
res_colnames,
NULL, /* <-- nscolumns */
j->jointype,
...);
```
Passing NULL for nscolumns leaves the join ParseNamespaceItem's `p_nscolumns` unset. In #2347 this segfaulted whenever `colNameToVar` → `scanNSItemForColumn` tried to resolve a column reference against the nsitem (PG's `scanNSItemForColumn` indexes `nsitem->p_nscolumns[attnum-1]` without null-checking).
I audited the other two `addRangeTableEntryForJoin` call sites in this file (lines 717 and 7564); both pass a real nscolumns array. Line 3238 is the only remaining NULL.
### Why it isn't triggered today
Every current path that would resolve an expression against an OPTIONAL MATCH's output first runs through `handle_prev_clause`, which wraps the prior as a new `RTE_SUBQUERY` with properly-built `p_nscolumns`. The incompletely-populated join nsitem is only in `pstate->p_namespace` during `transform_cypher_optional_match_clause` itself — and no in-line expression resolution happens there today. The jnsitem is also added with `addToRelNameSpace=false` (line 3262), so it's column-visible but not relation-visible, keeping it out of `refnameNamespaceItem` lookups.
### Why it's worth fixing
Any future feature that resolves an in-line expression against the OPTIONAL MATCH's output (e.g. OPTIONAL MATCH analogues of ON SET, or a WHERE processed before the helper returns) will hit the same segfault. The fix is the same three-line change made in #2347's follow-up commit (`f0ed4c95`): populate `p_nscolumns` from `res_colvars`, using the join rtindex and 1-based eref position (Vars inside opaque metadata are not planner-rewritten; see the commit message for context).
### Reproducer
None that triggers the crash from SQL on current master — this is a code-level observation, not a user-visible bug. Recording it so it isn't rediscovered the hard way.
### Proposed fix
Mirror the fix pattern from #2347's `transform_merge_make_lateral_join` change to `transform_cypher_optional_match_clause`. No regression test is feasible until a feature exercises it, but the change is local and the crash mode is well understood.
Happy to submit a PR if maintainers want the defensive fix landed now; otherwise leaving this open so the next person in the neighborhood sees it.
--
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: dev-unsubscribe@age.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Latent NULL p_nscolumns in OPTIONAL MATCH lateral-join ParseNamespaceItem [age]
Posted by "gregfelice (via GitHub)" <gi...@apache.org>.
gregfelice commented on issue #2404:
URL: https://github.com/apache/age/issues/2404#issuecomment-4683696672
@jrgemignani — agreed. Rather than open a standalone PR for an unreachable defect, here's the defensive patch parked on the issue so it's captured. Your call whether to pull it in now or wait until a feature actually exercises the path.
The fix mirrors the #2347 remediation in `transform_merge_make_lateral_join` (`f0ed4c95`) — build a `ParseNamespaceColumn` array from `res_colvars` instead of passing `NULL`:
```diff
@@ transform_cypher_optional_match_clause @@
- jnsitem = addRangeTableEntryForJoin(pstate, res_colnames, NULL,
- j->jointype, 0, res_colvars,
- NIL, NIL, j->alias, NULL, false);
+ {
+ int colcount = list_length(res_colvars);
+ int join_rtindex = list_length(pstate->p_rtable) + 1;
+ ParseNamespaceColumn *nscolumns = palloc0(colcount * sizeof(ParseNamespaceColumn));
+ ListCell *lvar; int col_idx = 0;
+ foreach (lvar, res_colvars) {
+ Var *v = (Var *) lfirst(lvar);
+ nscolumns[col_idx].p_varno = join_rtindex;
+ nscolumns[col_idx].p_varattno = col_idx + 1;
+ nscolumns[col_idx].p_vartype = v->vartype;
+ nscolumns[col_idx].p_vartypmod = v->vartypmod;
+ nscolumns[col_idx].p_varcollid = v->varcollid;
+ nscolumns[col_idx].p_varnosyn = join_rtindex;
+ nscolumns[col_idx].p_varattnosyn = col_idx + 1;
+ col_idx++;
+ }
+ jnsitem = addRangeTableEntryForJoin(pstate, res_colnames, nscolumns,
+ j->jointype, 0, res_colvars,
+ NIL, NIL, j->alias, NULL, false);
+ }
```
*(full version has the block comment cross-referencing `f0ed4c95`.)*
**Why it's safe now / why there's no test** — the defect is unreachable on current master, so the change is behavior-neutral and untestable by construction:
- `transform_cypher_optional_match_clause` resets `p_namespace` to `NIL`, then adds the jnsitem **column-namespace-only** (`addToRelNameSpace=false`); nothing resolves a column between adding it and returning.
- The sole caller consumes the result via `make_target_list_from_join`, which reads `joinaliasvars`/`eref->colnames` directly — never `p_nscolumns`.
- Every in-line expression is resolved only after the query is wrapped as an `RTE_SUBQUERY` by `handle_prev_clause`, which builds its own `p_nscolumns`.
So `p_nscolumns` on this nsitem is never read today — which is why no SQL reproducer exists and no regression test is feasible. Full cassert `installcheck` stays green (35/35, CI-exact extras).
**One caveat:** the `join-rtindex/position` layout is what #2347 needed because its Vars lived inside opaque `prop_expr`. Whether that exact mapping is right for a *future* OPTIONAL MATCH consumer can't be verified until one exists — and if it's wrong, the failure mode would be a silent wrong column rather than a crash. The NULL-deref hazard is removed regardless; layout should be validated against real behavior when a consumer lands.
Happy to turn this into a PR whenever you want it in.
--
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: dev-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Latent NULL p_nscolumns in OPTIONAL MATCH lateral-join ParseNamespaceItem [age]
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on issue #2404:
URL: https://github.com/apache/age/issues/2404#issuecomment-4652128202
@gregfelice
> Happy to submit a PR if maintainers want the defensive fix landed now; otherwise leaving this open so the next person in the neighborhood sees it.
Better to clean up issues before they become issues
--
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: dev-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Latent NULL p_nscolumns in OPTIONAL MATCH lateral-join ParseNamespaceItem [age]
Posted by "gregfelice (via GitHub)" <gi...@apache.org>.
gregfelice commented on issue #2404:
URL: https://github.com/apache/age/issues/2404#issuecomment-4684337410
Minor correction: my validation numbers above were against a stale local fork base. Re-checked on current apache `master` (`12e2a31c`) — the patch still applies cleanly, the inertness argument holds unchanged (the join nsitem's `p_nscolumns` is still never read on any current path), and full cassert `installcheck` is green at **39/39** (CI-exact extras). No change to the analysis or the patch.
--
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: dev-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org