gregfelice commented on PR #2458: URL: https://github.com/apache/age/pull/2458#issuecomment-4876148133
Thanks for the thorough review, @MuhammadTahaNaveed — genuinely useful. Addressed in the latest push: **1. [P1] tableoid generated columns** — Fixed. Good catch. The slot now carries `tts_tableOid = RelationGetRelid(...)` before `ExecComputeStoredGenerated()` in both the insert (`cypher_utils.c`) and update (`cypher_set.c`) paths, matching PostgreSQL's own ExecInsert/ExecUpdate. Added a regression case with `tbl oid GENERATED ALWAYS AS (tableoid) STORED` that asserts the stored value equals the label table's real OID on both CREATE and SET. **2. [P1] PROJECT.yaml** — Removed. That was a stray commit that rode in from a local branch base; the branch has been rebased onto current master so the PR now contains only the fix. **3. [P3] Rebase** — Done, the branch is rebased onto current master and CI is green against the current tree. **B. [P3] MERGE clears slot before the should_insert check** — Fixed. `clear_entity_slot()` is now gated on `should_insert` in both `merge_vertex` and `merge_edge`, so the matched path skips the null-init. Added a matched-MERGE regression case to cover that path. **A. [perf] SET builds a ResultRelInfo per row** — Agreed this is worth doing, but I'd like to keep it out of this bugfix. The per-row `ResultRelInfo` is pre-existing structure, and the generated-column recompute is guarded by `has_generated_stored`, so only labels that actually have a stored generated column pay the cost — no regression for the common case. Caching `ResultRelInfo`/`ri_GeneratedExprsU` per label across `apply_update_list` is a cleaner standalone change (alongside the existing relation/index/RLS caching); filed as #2459 so it doesn't get lost. **4. [P3] Trailing whitespace in the .out** — Left as-is intentionally. pg_regress does an exact byte comparison against psql's output, which emits trailing spaces on table rows; every expected file in `regress/expected/` has them (e.g. `cypher_set.out`). Stripping them would make the expected file no longer match psql's actual output and break the test, and there's no `diff --check`/whitespace gate in CI. So this one I have to decline. Thanks again — the tableoid case in particular was a real gap. -- 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]
