-
Notifications
You must be signed in to change notification settings - Fork 1
Fix code correctness: schema trigger, error types, dead code, assertions #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,15 @@ | ||
| -- Obsidian Graph MCP Server - PostgreSQL Schema | ||
| -- Obsidian Graph - PostgreSQL Schema | ||
| -- | ||
| -- This schema is designed for storing whole Obsidian notes (not chunked documents) | ||
| -- with vector embeddings for semantic search and graph analysis. | ||
| -- Stores notes (whole or chunked) with vector embeddings for | ||
| -- semantic search, graph analysis, and hub/orphan detection. | ||
|
|
||
| -- Enable pgvector extension | ||
| CREATE EXTENSION IF NOT EXISTS vector; | ||
|
|
||
| -- Migration: remove trigger that overwrites file mtime with DB timestamp | ||
| DROP TRIGGER IF EXISTS trigger_update_notes_modified_at ON notes; | ||
| DROP FUNCTION IF EXISTS update_modified_at(); | ||
|
Comment on lines
+10
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 schema.sql DROP TRIGGER runs before CREATE TABLE, failing on fresh databases
Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| -- Main notes table with vector embeddings | ||
| -- Supports chunking for large notes (voyage-context-3 pattern) | ||
| CREATE TABLE IF NOT EXISTS notes ( | ||
|
|
@@ -42,18 +46,3 @@ CREATE INDEX IF NOT EXISTS idx_notes_modified_at ON notes(modified_at); | |
| CREATE INDEX IF NOT EXISTS idx_notes_connection_count ON notes(connection_count DESC); | ||
| CREATE INDEX IF NOT EXISTS idx_notes_last_indexed_at ON notes(last_indexed_at); | ||
| CREATE INDEX IF NOT EXISTS idx_notes_chunk_index ON notes(chunk_index); | ||
|
|
||
| -- Function to update modified_at timestamp automatically | ||
| CREATE OR REPLACE FUNCTION update_modified_at() | ||
| RETURNS TRIGGER AS $$ | ||
| BEGIN | ||
| NEW.modified_at = CURRENT_TIMESTAMP; | ||
| RETURN NEW; | ||
| END; | ||
| $$ LANGUAGE plpgsql; | ||
|
|
||
| -- Trigger to auto-update modified_at on note updates | ||
| CREATE TRIGGER trigger_update_notes_modified_at | ||
| BEFORE UPDATE ON notes | ||
| FOR EACH ROW | ||
| EXECUTE FUNCTION update_modified_at(); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -125,6 +125,12 @@ async def initialize(self) -> None: | |||||||||||||||||||||||
| if not table_exists: | ||||||||||||||||||||||||
| logger.warning("Notes table does not exist yet (will be created by schema.sql)") | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| # Migration: remove trigger that overwrites file mtime (existing databases) | ||||||||||||||||||||||||
| await conn.execute( | ||||||||||||||||||||||||
| "DROP TRIGGER IF EXISTS trigger_update_notes_modified_at ON notes" | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| await conn.execute("DROP FUNCTION IF EXISTS update_modified_at()") | ||||||||||||||||||||||||
|
Comment on lines
+128
to
+132
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 initialize() runs DROP TRIGGER on potentially non-existent table The migration code at lines 129-132 runs
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| logger.info(f"PostgreSQL connected: {self.max_connections} max connections") | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| except asyncpg.PostgresError as e: | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Schema migration DROP TRIGGER is unreachable for existing databases
The
DROP TRIGGER IF EXISTSmigration atsrc/schema.sql:10-11is placed inside the init schema file, which is mounted atdocker-entrypoint-initdb.d/schema.sql(docker-compose.yml:54). Docker's PostgreSQL entrypoint only executes init scripts on first database creation (when the data volume is empty). For existing databases that already have the trigger, this migration never runs.This means existing deployments will retain the old
trigger_update_notes_modified_attrigger, which overwritesmodified_atwithCURRENT_TIMESTAMPon every UPDATE — including the_do_refreshbatch UPDATE insrc/hub_analyzer.py:225-244. This causes all notes'modified_atto be silently reset to the DB timestamp whenever connection counts are refreshed, making the orphaned notes'modified_atdisplay meaningless.Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.