crdv7 commented on issue #2588: URL: https://github.com/apache/age/issues/2588#issuecomment-6761504752
Thanks for the detailed diagnostics. **[#2351](https://github.com/apache/age/pull/2351) is more directly relevant to this issue than [#2517](https://github.com/apache/age/pull/2517) : it copies the label names collected during DELETE initialization, keeping them valid after the catalog scan ends.** ### Label names outlive the scan that provides them In [`get_all_edge_labels_per_graph()` on PG17/v1.7.0-rc0](https://github.com/apache/age/blob/e1467f12e0b1d15dd35d3ab93f057a7112d425b8/src/backend/catalog/ag_label.c#L290), the code stores the label-name pointers without copying their contents: ```c ExecStoreHeapTuple(tuple, slot, false); datum = slot_getattr(slot, Anum_ag_label_name, &isNull); label = DatumGetName(datum); labels = lappend(labels, label); ``` These names refer to data in the scanned tuples. Neither `DatumGetName()` nor `lappend()` copies the name, and this heap-tuple slot does not hold an independent buffer pin. After the scan releases its pins, the buffers can be reused for other pages. However, `css->edge_labels` retains those addresses until DELETE shutdown: ```text begin_cypher_delete() → get_all_edge_labels_per_graph() → retain label-name pointers ... execute the query; buffers may be reused ... end_cypher_delete() → check_for_connected_edges() → create_entity_result_rel_info() → parserOpenTable() ``` See [`cypher_delete.c`](https://github.com/apache/age/blob/e1467f12e0b1d15dd35d3ab93f057a7112d425b8/src/backend/executor/cypher_delete.c#L193) and [`create_entity_result_rel_info()`](https://github.com/apache/age/blob/e1467f12e0b1d15dd35d3ab93f057a7112d425b8/src/backend/executor/cypher_utils.c#L61). This can explain why `ag_label` remains correct while the relation name contains unrelated data. In this version, the shutdown check also runs for an edge-only DELETE: it does not skip opening the edge-label tables when the deleted-vertex set is empty. ### The relevant change is in #2351 [#2351](https://github.com/apache/age/pull/2351) added name copying in **both the index-scan and sequential-scan paths**: ```c lval = (Name) palloc(NAMEDATALEN); namestrcpy(lval, NameStr(*label)); labels = lappend(labels, lval); ``` See [commit `b3a00eea`](https://github.com/apache/age/commit/b3a00eea32c3faa93c43e0d91c0a75671e48ce65). The important part for this defect is copying the name, rather than using an index. [#2517](https://github.com/apache/age/pull/2517) addresses other writable-clause memory-lifetime issues; it does not change this label-name collection code. ### Minimal PG17 change The name-copying change can be applied independently to the PG17 1.7.0 base, without bringing in the other index optimizations from #2351: ```diff - labels = lappend(labels, label); + /* The scan does not keep the underlying buffer pinned after return. */ + labels = lappend(labels, pstrdup(NameStr(*label))); ``` This gives the list its own copy of each name, so the names remain valid after the scan ends. It addresses this specific lifetime defect, but does not establish that every error in your workload has the same cause. ### The hash-table cleanup issue is separate Your observation about `delete_vertex_htab` being under `TopMemoryContext` is valid. However, [`DELETE_VERTEX_HTAB_SIZE` is 1,000,000](https://github.com/apache/age/blob/e1467f12e0b1d15dd35d3ab93f057a7112d425b8/src/include/executor/cypher_utils.h#L48), which is passed to `hash_create()` as an initial capacity estimate—not an actual entry count. A large context alone therefore does not establish repeated leakage. There is nevertheless an error-path cleanup defect: ```c static void end_cypher_delete(CustomScanState *node) { check_for_connected_edges(node); hash_destroy(((cypher_delete_custom_scan_state *)node)->vertex_id_htab); ExecEndNode(node->ss.ps.lefttree); } ``` If `check_for_connected_edges()` raises an error, the subsequent `hash_destroy()` is skipped. This table is created without `HASH_CONTEXT`, so PostgreSQL allocates its private memory context under `TopMemoryContext`; normal query or transaction cleanup does not reclaim it. In the failure path described above, the invalid label name can trigger the error and leave the hash table allocated. Fixing the name lifetime does not fix cleanup for other errors that bypass `hash_destroy()`. I have not measured accumulation across repeated failures or established how much of the reported memory growth this explains. ### Backport and remaining observations In [#2450](https://github.com/apache/age/issues/2450), @jrgemignani described an intention to bring fixes to older PostgreSQL branches, down to PG14, without an ETA, and @MuhammadTahaNaveed offered to help with releases, starting with AGE 1.8.0 for PG15. The copy-only change above is a focused workaround to evaluate on your existing version. **Whether to backport this change, which branches to include, and when to release it are decisions for the AGE maintainers.** I cannot confirm an official backport or release schedule. -- 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]
