docs: strengthen RAG schema and guidance#374
Conversation
📝 WalkthroughWalkthroughUpdated RAG documentation to enforce database permissions and schema constraints. Changed the embedding column from nullable to Changes
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/services/rag.md (1)
1025-1049:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win"Service Fails to Start" may be misleading for SELECT-privilege issues.
Based on the provided server preflight behavior (role-existence only), lacking
GRANT SELECTon document tables is more likely to cause empty/“No relevant information found” query results than an outright failure to start. Since you also added a separate “Poor Query Results” section for NULL embeddings, it would help to either:
- rename/scope this section to “connection/user/privilege issues affecting queries”, or
- add a short note that SELECT-privilege problems typically surface during querying.
Suggested edit
-### Service Fails to Start +### Service Fails to Start (or Returns Empty Results) To diagnose a service that fails to start, check database connectivity and user permissions. + +Note: If the `connect_as` role exists but lacks `GRANT SELECT` on +the document table(s), the service may still start—queries can +return `"No relevant information found"` instead.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/services/rag.mdaround lines 1025 - 1049, The "Service Fails to Start"
section heading is misleading because the preflight only checks role existence
and missing SELECT privileges usually cause empty/"No relevant information
found" query results rather than startup failure; update the section title
and/or content in docs/services/rag.md (the "Service Fails to Start" heading and
its paragraphs) to either rename it to something like "Connection / User /
Privilege Issues Affecting Queries" or add a short clarifying sentence under the
existing heading stating that missing SELECT privileges on
documents_content_chunks typically surface as poor/empty query results during
runtime rather than preventing the service from starting, and keep the existing
diagnostic commands (\du+ example and has_table_privilege example) as-is.</details> </blockquote></details> </blockquote></details>🧹 Nitpick comments (4)
docs/services/rag.md (4)
77-87: ⚡ Quick winClarify GRANT SELECT guidance: apply to each configured document table and qualify schema/table names.
The new “connect_as user must have SELECT privilege on each document table” section is good, but the example SQL hard-codes
documents_content_chunkswithout a schema and doesn’t explicitly tie the GRANT target topipelines[].tables[].table(which is what users will actually configure).Consider updating the snippet/prose to:
- mention you must grant on every table listed under
pipelines[].tables[], and- use a schema-qualified table name (or at least instruct users to adjust based on their schema).
Suggested edit
If the `connect_as` user is not a database owner or superuser, it must have `SELECT` privilege on each document table. Grant this privilege in the `scripts.post_database_create` array: ```sql -GRANT SELECT ON documents_content_chunks TO app_read_only; +-- Grant SELECT on EVERY table referenced by `pipelines[].tables[]`. +-- If your tables are not in `public`, qualify the schema accordingly. +GRANT SELECT ON public.documents_content_chunks TO app_read_only;</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/services/rag.mdaround lines 77 - 87, Update the guidance to state that
the GRANT must be applied to every table listed under pipelines[].tables[].table
and to use schema-qualified names; replace the hard-coded example by explaining
to run a GRANT SELECT on each configured table (e.g., GRANT SELECT ON
.<table_name> TO <connect_as_user>) and add a short note suggesting to
replace with public if using the default schema.</details> --- `1050-1073`: _⚡ Quick win_ **DELETE remediation is destructive: add a short safety/capacity note.** The “Poor Query Results” section proposes: - `DELETE FROM documents_content_chunks WHERE embedding IS NULL;` That’s correct for remediation, but it can be disruptive on large tables (locks) and is irreversible without backups. A brief note like “back up / test in staging / consider batching for large tables” would reduce operator risk. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/services/rag.mdaround lines 1050 - 1073, Update the "Poor Query
Results" remediation that suggests DELETE FROM documents_content_chunks WHERE
embedding IS NULL to include a short safety/capacity note: advise taking a
backup or testing in staging first, consider running a SELECT COUNT(*) check
and/or deleting in small batched transactions (or using LIMIT with repeated
deletes) to avoid long locks, and remind operators that deletes are irreversible
without backups. Reference the existing section title "Poor Query Results" and
the DELETE statement so the note is inserted immediately after that remediation.</details> --- `1036-1048`: _⚡ Quick win_ **Make `has_table_privilege()` example robust: schema-qualify (or cast to regclass).** `SELECT has_table_privilege('app_read_only', 'documents_content_chunks', 'SELECT');` assumes the table is discoverable via the database search_path. If the document tables live outside `public`, this example can yield `false` (or error), confusing users. Prefer `public.documents_content_chunks` (or instruct users to use their schema), or use `::regclass`. <details> <summary>Suggested edit</summary> ```diff -SELECT has_table_privilege('app_read_only', 'documents_content_chunks', 'SELECT'); +-- Use schema-qualified table name to avoid search_path issues +SELECT has_table_privilege('app_read_only', 'public.documents_content_chunks', 'SELECT');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/services/rag.md` around lines 1036 - 1048, Update the has_table_privilege example to avoid search_path issues by schema-qualifying the table name or casting it to regclass: change the call to use either 'public.documents_content_chunks' (or the user's schema) or 'documents_content_chunks'::regclass when calling has_table_privilege for the user 'app_read_only' (referencing has_table_privilege, documents_content_chunks, app_read_only) so the check reliably returns the correct result regardless of search_path.
193-206: ⚡ Quick winEmbedding
NOT NULLchange: add an explicit “ALTER TABLE ... SET NOT NULL” migration step.You’ve already documented how to find and delete existing NULL embeddings in “Poor Query Results”, and the examples set
embedding vector(N) NOT NULLgoing forward. What’s missing is the final step for existing deployments: after cleanup, users should be guided to enforce the constraint (otherwise NULLs can reappear later).Suggested addition
-- Remove rows missing embeddings DELETE FROM documents_content_chunks WHERE embedding IS NULL; + +-- Then enforce the constraint to prevent future NULL embeddings +ALTER TABLE documents_content_chunks + ALTER COLUMN embedding SET NOT NULL;</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/services/rag.mdaround lines 193 - 206, Add a final migration step that
enforces the non-null constraint on the embedding column after users have
cleaned up any NULL embeddings: create a migration that alters the
documents_content_chunks table to set the embedding column (embedding) to NOT
NULL, and run it only after confirming there are no rows with NULL embeddings so
the constraint will succeed.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In@docs/services/rag.md:
- Around line 1025-1049: The "Service Fails to Start" section heading is
misleading because the preflight only checks role existence and missing SELECT
privileges usually cause empty/"No relevant information found" query results
rather than startup failure; update the section title and/or content in
docs/services/rag.md (the "Service Fails to Start" heading and its paragraphs)
to either rename it to something like "Connection / User / Privilege Issues
Affecting Queries" or add a short clarifying sentence under the existing heading
stating that missing SELECT privileges on documents_content_chunks typically
surface as poor/empty query results during runtime rather than preventing the
service from starting, and keep the existing diagnostic commands (\du+ example
and has_table_privilege example) as-is.
Nitpick comments:
In@docs/services/rag.md:
- Around line 77-87: Update the guidance to state that the GRANT must be applied
to every table listed under pipelines[].tables[].table and to use
schema-qualified names; replace the hard-coded example by explaining to run a
GRANT SELECT on each configured table (e.g., GRANT SELECT ON
.<table_name> TO <connect_as_user>) and add a short note suggesting to
replace with public if using the default schema.- Around line 1050-1073: Update the "Poor Query Results" remediation that
suggests DELETE FROM documents_content_chunks WHERE embedding IS NULL to include
a short safety/capacity note: advise taking a backup or testing in staging
first, consider running a SELECT COUNT(*) check and/or deleting in small batched
transactions (or using LIMIT with repeated deletes) to avoid long locks, and
remind operators that deletes are irreversible without backups. Reference the
existing section title "Poor Query Results" and the DELETE statement so the note
is inserted immediately after that remediation.- Around line 1036-1048: Update the has_table_privilege example to avoid
search_path issues by schema-qualifying the table name or casting it to
regclass: change the call to use either 'public.documents_content_chunks' (or
the user's schema) or 'documents_content_chunks'::regclass when calling
has_table_privilege for the user 'app_read_only' (referencing
has_table_privilege, documents_content_chunks, app_read_only) so the check
reliably returns the correct result regardless of search_path.- Around line 193-206: Add a final migration step that enforces the non-null
constraint on the embedding column after users have cleaned up any NULL
embeddings: create a migration that alters the documents_content_chunks table to
set the embedding column (embedding) to NOT NULL, and run it only after
confirming there are no rows with NULL embeddings so the constraint will
succeed.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `a4ac85c8-6380-4b58-9a55-f1f9c07d79ed` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 1e081a892622594a3142c7d093ce1fbd30bc7a7c and 1d2d32e1c2d184826d7ecf92bb7e9235c997b3b5. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `docs/services/rag.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
tsivaprasad
deleted the
PLAT-584-rag-service-returns-no-relevant-information-found-on-nodes-added-via-database-update
branch
2 participants
Summary
Strengthens the RAG service documentation with clearer schema requirements and guidance, based on issues observed during testing.
Changes
embedding vector(N) NOT NULLin all table schema examples to prevent NULL embeddings at the database levelconnect_asusers requireGRANT SELECTon document tables — omitting this causes the service to silently return"No relevant information found"for all queries\dttable check withhas_table_privilege()in the "Service Fails to Start" troubleshooting sectionTesting
Checklist
Notes for Reviewers
Two silent failure modes documented here were discovered during RAG service testing:
NULL embeddings — inserting rows without the
embeddingcolumn causes the vector search to crash internally and return empty results with no visible error. TheNOT NULLconstraint on the schema prevents this. The corresponding RAG server fix (filtering NULL rows in the SQL query) is in a separate PR in thepgedge-rag-serverrepo.Missing SELECT privilege — if
connect_asis a non-superuser, the RAG container connects successfully but getspermission deniedon the table, which also surfaces as"No relevant information found"with no indication of the cause.