PostgreSQL dialect refactor - PR1 of 4 - Dialect#276
PostgreSQL dialect refactor - PR1 of 4 - Dialect#276
Conversation
roborev: Combined Review (
|
Introduce a Dialect interface that abstracts SQL generation and driver-
specific behavior, and wire the Store to delegate to it. SQLiteDialect
implements the existing SQLite behavior unchanged. No new dependencies.
Scope:
- Dialect interface (internal/store/dialect.go) covers Now(), Rebind(),
InsertOrIgnore, UpdateOrIgnore, full-text search (upsert/search/delete/
backfill/availability/clear), connection init, schema init, WAL
checkpoint, migration probes, and driver error predicates.
- SQLiteDialect (internal/store/dialect_sqlite.go) returns the same SQL
that was previously hardcoded.
- Store holds a Dialect field and routes calls through it; Open and
OpenReadOnly both construct and InitConn the dialect.
- All datetime('now'), INSERT/UPDATE OR IGNORE/REPLACE, PRAGMA, FTS5,
and isSQLiteError usages in the store package go through the dialect.
- subset.go is intentionally not migrated; it keeps using isSQLiteError
directly and is flagged for follow-up.
- PostgreSQL URLs are rejected by Open() with a clear error.
- Design notes in pg_refactor_docs/ describe the PR1..PR4 plan.
Addressed review feedback:
- InitSchema probes FTSAvailable unconditionally so dialects without a
separate FTS schema file (PostgreSQL) still set the availability flag.
- SearchMessages and SearchMessagesQuery Rebind the composed SQL before
execution, and the FTS order fragment is taken from the dialect
instead of hardcoding "rank".
- OpenReadOnly now calls dialect.InitConn.
- FTSSearchClause drops unused paramIndex; fragments use ? placeholders
and callers Rebind the final query.
- FTSAvailable and FTSNeedsBackfill return bool; the always-nil error
return was discarded by every caller.
- insertInChunks accepts a chunkInsert options struct instead of six
positional parameters.
- FTSUpsertSQL docstring describes the actual SQLite 7-arg contract.
- Duplicate isSQLiteErrorMatch helper removed; dialect predicates share
the canonical isSQLiteError helper that subset.go also uses.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-owned FTS upsert, Rebind in insertInChunks - FTSSearchClause() now returns (join, where, orderBy, orderArgCount). Callers bind the search term orderArgCount additional times so a PG ts_rank() placeholder in ORDER BY binds correctly after Rebind. SQLite returns 0 because "rank" is an implicit FTS5 column. - mergeLabelByName rewritten in portable SQL (DELETE conflicting associations, then plain UPDATE). UpdateOrIgnore dropped from Dialect — it was a SQLite-semantic leak that PG cannot mechanically emulate. - FTSUpsertSQL replaced by FTSUpsert(q, FTSDoc): the dialect now owns both the SQL and the argument shape, so SQLite's rowid duplication stays out of callers and PG is free to do a tsvector column update. - chunkInsert gains a rebind field; all three callers (replaceMessageRecipientsTx, replaceMessageLabelsTx, AddMessageLabels) pass s.dialect.Rebind so composed chunk SQL is renumbered for PG. replaceMessageRecipientsTx takes RecipientSet to stay under the ≤5-positional-parameter limit. - ensureLabelWith / mergeLabelByName no longer take a Dialect parameter now that the UPDATE-OR-IGNORE workaround is gone. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…helpers - SearchMessagesQuery uses a new ftsEnabled bool (len(TextTerms) > 0) as the authoritative signal that FTS is active. The old ftsJoin != "" check skipped the relevance ORDER BY and the non-FTS fallback for dialects whose FTS clause needs no join (PG tsvector). - ensureLabelWith and mergeLabelByName now take a Dialect and route every ?-placeholder statement through d.Rebind, so non-SQLite backends get numbered placeholders. The Phase 1 loop inside EnsureLabelsBatch uses s.dialect.Rebind for the same reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
wesm
force-pushed
the
pr1-branch-dialect-extraction
branch
from
7f47f5c to
e291892
Compare
|
Just rebased and addressed some review findings |
roborev: Combined Review (
|
2 participants
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:
Dialectinterface andSQLiteDialect, moves all SQLite-specific SQL out of the store layer. No new dependencies, no behavior change.PostgreSQLDialect, pgx driver, PG schema with tsvector FTS, and dual-backend test support.Rebind,RETURNING, parameterized FTS), PG-native schema migrations, connection pool config, and full test coverage across both backends.migrate-dbcommand for copying data between SQLite and PostgreSQL in either direction.