PostgreSQL dialect refactor - PR1 of 4 - Dialect

[webgress]

Adding PostgreSQL as an opt-in database backend (SQLite stays the default).
Details in pg_refactor_docs/.
This is split into 4 PRs to make it easier to review:

• PR1 — Dialect extraction (this PR): Introduces a Dialect interface and SQLiteDialect, moves all SQLite-specific SQL out of the store layer. No new dependencies, no behavior change.
• PR2 — PostgreSQL dialect: Adds PostgreSQLDialect, pgx driver, PG schema with tsvector FTS, and dual-backend test support.
• PR3 — PostgreSQL functional: Portable query layer (Rebind, RETURNING, parameterized FTS), PG-native schema migrations, connection pool config, and full test coverage across both backends.
• PR4 — Data migration: migrate-db command for copying data between SQLite and PostgreSQL in either direction.

[roborev-ci]

roborev: Combined Review (7f47f5c)

Verdict: the PR is not ready to merge; it introduces 1 high-severity regression risk and 4 medium-severity portability bugs.

High

• internal/store/store.go:401 (InitSchema())
  Dialects that return "" from SchemaFTS() never mark FTS as available, so search indexing/backfill will silently stay disabled for those backends. This breaks the intended contract for dialects where FTS lives in the main schema.
  Fix: Set FTS capability independently of the SchemaFTS() branch, e.g. by probing via the dialect after schema init.

Medium

• internal/store/api.go:179
SearchMessages() combines dialect-provided FTS SQL with hardcoded ? placeholders for LIMIT/OFFSET, then executes the raw SQL without rebinding. That will fail on non-SQLite dialects.
  Fix: Build the full query with ? placeholders, then run s.Rebind() on the completed SQL before execution.

• internal/store/api.go:249
SearchMessagesQuery() always calls FTSSearchClause(1) even when earlier filters have already consumed parameters. For dialects using numbered placeholders, the FTS predicate will bind the wrong argument when prior conditions exist.
  Fix: Pass the next real parameter index, e.g. len(args)+1.

• internal/store/messages.go (UpsertFTS())
  UpsertFTS() passes 7 arguments to s.db.Exec(), but Dialect.FTSUpsertSQL() is documented to take 6 parameters. That placeholder/argument mismatch is likely to break PostgreSQL integration.
  Fix: Pass exactly the 6 documented parameters, and adjust the SQLite SQL to reuse the first parameter via numbered placeholders if needed.

• internal/store/store.go (OpenReadOnly())
  OpenReadOnly() creates the dialect but does not call dialect.InitConn(db). This is harmless for SQLite but will leave read-only connections for other dialects uninitialized.
  Fix: Call dialect.InitConn(db) immediately after dialect creation and return any error.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[wesm]

Just rebased and addressed some review findings

[roborev-ci]

roborev: Combined Review (e291892)

Verdict: Changes are not ready to merge; the dialect abstraction currently leaves multiple SQL execution paths incompatible with non-SQLite backends.

High

• Placeholder rebinding is applied inconsistently across dialect-aware queries

  • Location: internal/store/messages.go:214, internal/store/messages.go:1157, internal/store/messages.go:1193; internal/store/messages.go (upsertMessageWith, EnsureConversation, UpsertAttachment, backfillFTSBatch); internal/store/sources.go; internal/store/sync.go
  • Problem: Several updated queries still execute with raw ? placeholders after introducing dialect-specific SQL fragments. On PostgreSQL or any non-SQLite backend, these paths will fail unless the statements are passed through Rebind().
  • Fix: Ensure every shared SQL statement is run through d.Rebind/s.dialect.Rebind before execution, or move full statement construction into dialect methods so callers never emit raw placeholders directly.

• InsertOrIgnore() call sites also appear to skip placeholder rebinding

  • Location: internal/store/messages.go (EnsureParticipantsBatch, EnsureConversationParticipant, UpsertReaction, EnsureParticipantByPhone)
  • Problem: The abstraction rewrites the INSERT form, but the resulting SQL still contains ? placeholders and is executed without a corresponding Rebind(). That leaves these paths non-portable for backends that require positional parameters.
  • Fix: Apply Rebind() to the SQL returned from InsertOrIgnore() before executing it.

Medium

• Chunked delete helpers may still generate non-portable IN (...) queries
  • Location: internal/store/messages.go (MarkMessagesDeletedBatch, MarkMessagesDeletedByGmailIDBatch)
  • Problem: execInChunks appears to build dynamic IN (%s) queries without a rebinding step, unlike the refactored insert chunking path. If so, these batch delete/update paths will still break on non-SQLite dialects.
  • Fix: Refactor execInChunks to accept and apply the dialect Rebind() function to the final composed query.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (baf576a)

Verdict: Changes are not ready to merge; 2 medium-severity issues need to be addressed.

Medium

• pg_refactor_docs/msgvault_pg_implementation_prompt.md:3-6
  This adds a repo-resident prompt that tells users to pipe repository-controlled text into Claude Code with --dangerously-skip-permissions. That creates a prompt-injection path where any future PR changing this file, or referenced companion files, could steer a fully privileged agent into exfiltrating secrets, modifying the workspace, or pushing unauthorized changes.
  Suggested fix: Do not recommend feeding repo-controlled content into an agent with dangerous permissions enabled. Move the playbook outside the repo or another trusted location, require explicit human review before use, and keep normal sandbox/approval gates enabled.

• internal/store/messages.go:740
Dialect.InsertOrIgnore is documented as taking a complete SQL statement, but AddMessageLabels passes only a partial prefix (INSERT OR IGNORE INTO ... VALUES ). A PostgreSQL implementation that appends ON CONFLICT DO NOTHING would place it before the generated VALUES tuples, yielding invalid SQL.
  Suggested fix: Add a dedicated method for chunked insert prefixes, such as InsertOrIgnorePrefix(...), or otherwise separate conflict-handling syntax into prefix/suffix handling instead of running a complete-statement rewriter on a partial statement.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (e661000)

Verdict: No medium, high, or critical findings; the reviewed changes look clean.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)