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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
