jrgemignani commented on PR #2303: URL: https://github.com/apache/age/pull/2303#issuecomment-4594391831
## Review: Logic soundness + regression-test validation **Verdict: logic is sound, and all regression-test changes are legitimate (no masked correctness regressions).** This PR replaces the `_agtype_build_vertex/edge(...)` construction path with native composite types (`vertex`, `edge`) plus implicit casts to agtype/json/jsonb, and adds a parser-level accessor optimization that lowers `id()/properties()/label()/type()/start_id()/end_id()/startNode()/endNode()` to direct `FieldSelect` nodes instead of building and re-parsing agtype. ### Logic soundness The one structurally risky element — the `edge` composite field order `(id, label, end_id, start_id, properties)`, with **`end_id` before `start_id`** — is handled correctly and consistently across all four sites that must agree: | Site | Mapping | |---|---| | SQL composite definition | `end_id` = attr 3, `start_id` = attr 4 | | `edge_to_agtype` / `edge_to_json_string` (agtype.c) | reads `values[2]`→end_id, `values[3]`→start_id, calls `make_edge(id, start_id, end_id, …)` ✓ | | `get_record_field_info` (cypher_clause.c) | `endnode`/`end_id`→field 3, `startnode`/`start_id`→field 4 ✓ | | `make_edge_expr` RowExpr | `list_make5(id, label, end_id, start_id, props)` + matching colnames ✓ | This is empirically confirmed by the new EXPLAIN tests: `start_id(r)` → `Output: (r.start_id)::agtype` and `end_id(r)` → `Output: (r.end_id)::agtype`. No swap. Other verified items: - **`_agtype_build_vertex/edge` label param cstring→agtype**: validates the value is a scalar string, errors otherwise; NULL handling intact; memory freed correctly. - **`get_label_name(label_id, graph_oid)`** reads the existing label cache instead of a fresh `systable` scan (the perf win); returns cache memory (callers must not `pfree`). - **`vertex_eq`/`edge_eq`** compare by `id` only — correct graph-entity identity semantics. No `HASHES`/`MERGES` declared, so no hash/merge-join hazard. - **`optimize_accessor_function`**: the uninitialized-`result` concern is not exploitable — `startNode/endNode` on a vertex yields `InvalidAttrNumber`, which hits the `else` `ereport(ERROR)` (correct "must be an edge" message), so `result` is never read uninitialized. - **`agtype_volatile_wrapper`** correctly extended for `GRAPHIDOID`, `VERTEXOID`, `EDGEOID`. - **`agtype_categorize_type` / `datum_to_agtype`** correctly route composites through `vertex_to_agtype`/`edge_to_agtype` when boxing into maps/lists. **Two non-blocking findings:** 1. **`_get_vertex_by_graphid` has no `REVOKE … FROM PUBLIC`.** Largely mitigated because the underlying `get_vertex` performs `pg_class_aclcheck(… ACL_SELECT)`, so a caller lacking SELECT on the label table errors out. It still bypasses RLS row policies, though AGE label tables don't use RLS. Adding the REVOKE would be defense-in-depth. 2. **OID-dependent error text**: `reverse() unsupported argument type 16843` embeds the dynamic `VERTEXOID`. Deterministic in a fresh regress DB, but brittle if catalog OIDs shift. ### Regression-test classification | File | Δ | Classification | Verdict | |---|---|---|---| | `cypher_match.out` | 6 | **reorder only** | Same 6 (and 2) rows; only tie order changed. All ids present before & after. No `ORDER BY`. Benign. | | `pgvector.out` | 4 | **intentional mod** (matches `.sql`) | HNSW index expression now indexes the raw `properties` column instead of `_agtype_build_vertex(...)`. Semantically equivalent. Sound. | | `agtype.out` | 92 | **forced input mod** (matches `.sql`) | Labels changed `$$x$$`→`'"x"'` for the cstring→agtype signature. Result rows byte-identical. No behavior change. | | `expr.out` (early hunks) | ~16 removed | **message mods + reorder** | Error text changes (still error, same semantics, now with `LINE … ^` pointer); 6 `case_statement` hunks are reorder-only (same 6 vertices; row carrying the real computed value keeps its `1`/`6`). Benign. | | `expr.out` (final hunk) | +1099 | **additive** | New EXPLAIN/accessor coverage demonstrating the optimization. | | `cypher_with.out` / `.sql` | +312 / +143 | **additive** | New feature coverage, zero deletions. | **Reorder vetting:** every reordering-only change preserves the complete row set (all ids present before and after) and value-bearing rows retain their values. The reordering is a benign consequence of vertices/edges now being composite `RowExpr`s (different scan/aggregate ordering) — not a masked add, drop, or alteration of rows. **Error-message modifications** all keep the same outcome (the query still errors with the same semantics), e.g. `cannot cast agtype vertex to type int` → `cannot cast type vertex to bigint for column "i"` (PG's native composite-coercion rejection), and accessor errors now carry an added `LINE n: … ^` position from `parser_errposition`. ### Summary The core C logic is correct — including the bug-prone `end_id`/`start_id` composite ordering, which is consistently respected everywhere and proven by the new EXPLAIN tests. No regression-test change masks a correctness defect: changes are either (a) forced input adaptations with identical outputs, (b) intentional documented optimizations, (c) benign output reordering with full row-set preservation, or (d) purely additive new coverage. The only non-blocking nits are the missing `REVOKE` on `_get_vertex_by_graphid` (mitigated by an existing ACL check) and one OID-dependent error string. -- 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]
