* fix(pgvector): skip document column for metadata-only fetches (#1840 follow-up)
Closes the explicit "separate follow-up to keep this low-risk" callout
in PR #1840's description.
For remote pgvector deployments (TLS over WAN), `mempalace_status` and
every other metadata-only consumer was transferring the full `document`
column over the wire even when nothing read it. A single scroll over a
177K-drawer palace on a 175 ms-RTT link moved ~150 MB of document text
plus ~50 MB of metadata; this PR drops that to ~50 MB.
scroll_rows / _scroll gain `with_document: bool = True`. When False,
SELECT projects NULL::text instead of the document column. Positional
_row parser unchanged (record[1] stays the document slot, just receives
NULL). Existing callers default to True and see byte-for-byte identical
behavior.
PgVectorCollection.get_all_metadata override: where=None path goes
single-scroll with with_document=False. Filtered path falls back to base
to keep _matches_where running on array/object metadata values (same
correctness contract as #1840's filtered-path decision).
Tests:
- Update _FakePgVectorClient.scroll_rows to accept with_document; mirror
the NULL-becomes-empty-string semantics when False
- Update 5 existing scroll_calls assertions to include with_document=True
(unchanged intent)
- test_pgvector_get_all_metadata_skips_document_column: assert exactly
one scroll call with with_document=False
- test_pgvector_get_all_metadata_filtered_falls_back_to_base: assert
filtered path preserves with_document=True
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KC5Qsknh2zFRtRvVyjXiTA
* fix(pgvector): extend with_document=False fast path to filtered get_all_metadata
Per gemini-code-assist review feedback on #1892: _matches_where only reads
metadata, so the where=None vs where=set conditional fall-back was unnecessary.
The filtered path can use the same single-scroll with_document=False fast path
and apply the post-filter locally on metadata dicts — extending the wire-byte
win to every get_all_metadata caller, not just unfiltered ones.
Mirrors the pushdown + local _matches_where pattern already used by _rows
in the same file: pushdown when _requires_local_filter is False, post-filter
in Python otherwise. Same correctness contract as #1840's filtered get path.
Renames test_pgvector_get_all_metadata_filtered_falls_back_to_base to
test_pgvector_get_all_metadata_filtered_uses_fast_path and asserts the new
behavior (with_document=False + pushdown forwards the equality filter to SQL).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KC5Qsknh2zFRtRvVyjXiTA
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
A lone UTF-16 surrogate (U+D800-U+DFFF) in transcript content has no UTF-8
encoding, so pgvector's bulk upsert_rows makes psycopg raise UnicodeEncodeError
and the whole mine aborts, leaving later files unmined.
Apply config.strip_lone_surrogates (-> U+FFFD) to id, document, and the
serialized metadata JSON in upsert_rows. json.dumps(ensure_ascii=False) leaves a
metadata surrogate raw in the string, so one pass over the serialized JSON covers
it; NUL, by contrast, json-escapes and must be stripped before serialization
(see #1829). Replace rather than drop, matching ChromaDB's document handling.
Verified end to end against live Postgres + pgvector: before, a surrogate in
document or metadata aborts the mine; after, it ingests and round-trips as U+FFFD.
Fixes#1833
PgVectorCollection.get(limit=, offset=) ignored pagination at the SQL
layer: scroll_rows ran SELECT ... WHERE <where> with no LIMIT/OFFSET, so
get() fetched the whole table and sliced in Python. prefetch_mined_set
pages the whole palace on every mine, so mining was O(N^2) in rows
transferred and Python objects built as the palace grows; every other
paginating caller (exporter, migrate, repair, hallways, closet_llm,
miner, palace_graph) paid the same cost.
Push LIMIT/OFFSET into scroll_rows/_scroll with ORDER BY id (the primary
key) for stable offset pagination. get() uses the pushed path only for an
unfiltered page (no ids, no where/where_document, non-negative bounds); a
filtered get keeps the full-scan path because the metadata @> ... pushdown
is broader than the exact _matches_where re-filter for array/object
values, so that re-filter must run before pagination. Full-scroll callers
pass no bound, so their SQL is unchanged.
Co-authored-by: hrabbach <181709360+hrabbach@users.noreply.github.com>
PostgreSQL cannot store NUL (0x00) in text or jsonb. On the pgvector write path
a NUL in `document` is rejected by psycopg ("PostgreSQL text fields cannot
contain NUL (0x00) bytes") and a NUL in `metadata` becomes a JSON unicode escape
the jsonb cast rejects ("unsupported Unicode escape sequence"). `_execute`
re-wraps either as BackendError and `_mine_impl` re-raises, so the whole mine
exits non-zero and every file after the offending one is left unmined. ChromaDB,
SQLite, and Qdrant store the byte verbatim, so only pgvector hard-fails.
Add a recursive `_strip_nul` helper and apply it to id, document, and metadata
in `_PgVectorClient.upsert_rows`, mirroring the backend-layer sanitization
`_sanitize_documents_for_chromadb` already does for lone surrogates on the same
bulk-ingest paths. ids are SHA-256 hashes and metadata keys are fixed field
names, so the id and key passes are no-ops in practice; only transcript-derived
values change.
Co-authored-by: hrabbach <181709360+hrabbach@users.noreply.github.com>
Three findings from the Copilot review on ec5d1eb:
- pgvector (real correctness bug): table_dimension() read the raw
pg_attribute.atttypmod of the vector(n) column, which is not the bare
dimension, so reopening a stored pgvector palace could raise a false
DimensionMismatchError on the next same-dimension write. Now rounds through
format_type(atttypid, atttypmod) (the type's own typmod_out), which yields
the canonical vector(N) regardless of encoding or pgvector version. The live
roundtrip test now closes + reopens and writes a same-dim vector to guard it.
- chroma (real correctness bug): _lexical_search_via_sqlite() returned
LexicalHit.id as the internal embeddings.id rowid instead of the public
embeddings.embedding_id, so lexical_search -> get(ids=...) did not round-trip
(broke hybrid-search id lookups). Now selects e.embedding_id and maps rowid
-> public id. Existing FTS test schema updated to include embedding_id (real
Chroma schema) and assert the public id; added an end-to-end round-trip test
through a real ChromaBackend collection.
- sqlite_exact (error-message quality): CollectionNotInitializedError was
raised with palace_path instead of the collection name in get_collection and
delete_collection, inconsistent with the other backends and line 287. Now
names the collection; added a regression test.
Earlier first-pass findings (palace.py unknown-backend KeyError, dedup.py
docstring) were already fixed in ec5d1eb.
Adds a second external storage backend (Postgres/pgvector) alongside Qdrant
to prove the BaseBackend/BaseCollection contract generalizes across substrates
(SQL + JSONB containment filters + pgvector `<=>` ranking vs Qdrant's REST/dict
model), and addresses the review feedback on PR #1679.
Backend (mempalace/backends/pgvector.py):
- table-per-(namespace, palace, collection) isolation; advertises
supports_namespace_isolation
- JSONB filter pushdown for the containment subset, local-exact fallback for
$or/$contains/comparisons/where_document
- BM25 lexical search; marker-based mismatch protection
- optional psycopg dependency (lazy import), in-memory fake for CI, live test
gated on MEMPALACE_PGVECTOR_LIVE_URL
- registered in registry/__init__/pyproject entry point + [pgvector] extra;
MEMPALACE_PGVECTOR_DSN / MEMPALACE_PGVECTOR_NAMESPACE config; README docs
Isolation contract (RFC 001):
- PalaceRef/BaseBackend document the per-id MUST and the cross-namespace MUST,
gated on the new supports_namespace_isolation capability token
- runnable conformance suite (tests/_backend_conformance.py,
tests/test_backend_conformance.py); qdrant + pgvector run it via their fakes
Marker fail-loud guard:
- qdrant and pgvector now refuse get_collection when local_path is None instead
of silently opening a remote collection with no mismatch protection
Review fixes:
- palace._open_collection_or_explain handles unknown-backend KeyError as a CLI
state message instead of an escaping stack trace
- dedup.py docstring no longer claims "No API calls" unconditionally (false for
remote backends)
Pim Messelink
Igor Lins e Silva
mvalentsev