feat(age): close drawer-sync gap with unique index + delete-through - #223
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses synchronization gaps between the relational mempalace_drawers table and the Apache AGE graph. By introducing a unique index on drawer IDs and implementing a delete-through hook, the system now ensures data consistency and prevents the accumulation of duplicate or orphaned nodes in the graph. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a delete-through hook to propagate drawer deletions from the relational Postgres database to the Apache AGE graph, preventing orphan Drawer nodes. It also adds a unique index on Drawer.properties->>'id' during graph bootstrap to close a concurrent write race. The review feedback highlights two critical issues: first, a potential data drift bug in PostgresCollection.delete where the hook could be notified of deletions that never occurred if both ids and where filters are used; second, a performance bottleneck in _ensure_drawer_unique_index which performs an expensive sequential scan on every startup instead of checking if the index already exists first.
| # hook can propagate them to AGE. For id-only deletes this is just | ||
| # ``ids``; for where-based deletes we have to query first because | ||
| # the predicate refers to rows that won't exist post-DELETE. | ||
| deleted_ids = list(ids) if ids else None |
There was a problem hiding this comment.
If both ids and where are provided, the DELETE statement will only remove the subset of ids that also match the where filter. However, deleted_ids is currently set to the entire ids list, causing the delete-through hook to be notified of deletions that never actually occurred in the relational database. This leads to data drift between the relational table and the AGE graph.
Restricting the fast-path assignment to cases where where is absent ensures we query the database to resolve the exact matching IDs when a filter is present.
| deleted_ids = list(ids) if ids else None | |
| deleted_ids = list(ids) if (ids and not where) else None |
| cur.execute( | ||
| f"SELECT COUNT(*) - COUNT(DISTINCT (properties::text)::jsonb->>'id') " | ||
| f'FROM "{graph}"."Drawer"' | ||
| ) | ||
| dup_excess = cur.fetchone()[0] |
There was a problem hiding this comment.
The _ensure_drawer_unique_index method runs on every graph bootstrap (which occurs during every initialization of KnowledgeGraphAGE). Performing a sequential scan and COUNT(DISTINCT) on the Drawer table on every startup is extremely expensive and will severely degrade startup performance as the graph grows.
Checking if the drawer_id_unique index already exists in pg_indexes first allows us to return early and completely avoid this expensive query on subsequent startups.
| cur.execute( | |
| f"SELECT COUNT(*) - COUNT(DISTINCT (properties::text)::jsonb->>'id') " | |
| f'FROM "{graph}"."Drawer"' | |
| ) | |
| dup_excess = cur.fetchone()[0] | |
| cur.execute( | |
| "SELECT 1 FROM pg_indexes WHERE schemaname = %s AND indexname = 'drawer_id_unique'", | |
| (graph,), | |
| ) | |
| if cur.fetchone() is not None: | |
| return | |
| cur.execute( | |
| f"SELECT COUNT(*) - COUNT(DISTINCT (properties::text)::jsonb->>'id') " | |
| f'FROM "{graph}"."Drawer"' | |
| ) | |
| dup_excess = cur.fetchone()[0] |
Paired hygiene fixes so AGE stays consistent with mempalace_drawers: * Drawer.id unique index (_ensure_drawer_unique_index) runs at graph bootstrap. AGE MERGE has a read-then-create gap that isn't atomic; concurrent writers can both miss and CREATE, leaving duplicate Drawer nodes (844 accumulated on familiar before manual cleanup). The index makes the second CREATE fail at the Postgres layer. * Delete-through hook (make_age_deletethrough) mirrors the writethrough on the delete path. PostgresCollection.delete() now resolves ids before deleting, then invokes kg.delete_drawers() so AGE nodes go away with their source rows. Without this, deletes orphaned Drawer nodes (254 accumulated since 2026-05-22). Together: no new dupes can land, and deletes propagate cleanly. Tests: 7 new tests cover make_age_deletethrough / make_null_deletethrough / make_deletethrough_from_env. 1 fixture update for _ensure_graph()'s extra fetchone calls. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Bringing this PR in line with the ruff 0.15.14 pin landed via the v3.3.6 sync (chore: sync upstream v3.3.6): - knowledge_graph_age.py + test_age_kg_units.py: f-string quote normalization (single → double in f-string outer) - kg_writethrough.py: collapse short logger.warning call to single line - knowledge_graph_age.py: add missing `import logging` + module `logger`, fixes F821 on the new logger.debug call introduced in the index installer (skip-when-dupes-exist defensive branch) No behavior change.
6fda6c6 to
3dd3969
Compare
- README: 3250 → 3257 tests pass (added by PR #224 / kg_triple_worker) - website/reference/python-api/{backends/postgres,kg_writethrough,knowledge_graph_age}.md: regenerated from current docstrings via scripts/render-api-docs.py
Stale after PR #224's kg_triple_worker source/docstring edits landed on main.
Summary
Closes the drawer-sync gap between the relational
mempalace_drawerstable and the AGE graph. Two paired hygiene fixes plus a small follow-up cleanup.1. Drawer.id unique index (
_ensure_drawer_unique_index)Runs on graph bootstrap, installs a unique B-tree on
properties->>'id'. AGEMERGE (d:Drawer {id: X})has a read-then-create gap that isn't atomic — two concurrent writers both miss andCREATE, producing duplicate Drawer nodes. The index makes the secondCREATEfail at the Postgres layer. 844 dupes accumulated on familiar.jphe.in before manual cleanup + index install.The installer is defensive:
2. Delete-through hook (
make_age_deletethrough)Mirrors the writethrough on the delete path.
PostgresCollection.delete()now resolves doomed ids before the SQL DELETE, then invokeskg.delete_drawers()so the AGE Drawer nodes (and their incidentMENTIONSedges) go away with the source rows. Without this, every drawer delete leaked an orphan Drawer node into the graph; 254 orphans accumulated on familiar before manual cleanup.3. Ruff 0.15.14 format + missing logger (chore commit)
Forward-compat with the v3.3.6 ruff pin (
#222):knowledge_graph_age.py+test_age_kg_units.pylogger.warningto single lineimport logging+ moduleloggerinknowledge_graph_age.py— fixes a latent F821 on the newlogger.debugcall in the defensive index-installer branch. This bug pre-exists in thea9e15b8commit; would have surfaced under any lint run.Test plan
ruff check+ruff format --checkpass on mempalace/ and tests/test_age_kg_units.pymake_age_deletethrough/make_null_deletethrough/make_deletethrough_from_env)Note on the missing 3rd commit
The original task listed three local commits:
ea7f733(docs),a9e15b8(code),15fed7e(changelog).ea7f733was a state-refresh that's already been further superseded onorigin/main(drawer count claims, backfill status, daemon-host references all overtaken by 5+ newer commits). Cherry-picking it would have meant reverting newer doc updates. The current values (365K+ drawers, 3234 tests, daemon on familiar) are preserved in this PR via the changelog-cherry-pick conflict resolution.PR ordering
Best reviewed after #222 (upstream v3.3.6 sync) merges so the ruff 0.15.14 pin matches the format in this PR. Should rebase cleanly.