CMR-11083: Switch a SQL transaction to read only#2381
Conversation
📝 WalkthroughWalkthroughBatch concept queries in the search module now run inside transactions explicitly set to read-committed isolation and marked read-only; logging around batch transaction start and per-batch result counts was added. No public APIs or function signatures changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
![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)
metadata-db-app/src/cmr/metadata_db/data/oracle/search.clj (1)
249-264:⚠️ Potential issue | 🔴 CriticalThe read-only transaction is bypassed — query executes on
dbinstead ofconn.Line 264 passes
db(the original datasource) tosu/queryinstead ofconn(the transactional connection bound bywith-db-transaction). This means the query runs outside the transaction, so the:isolation :read-committed :read-only? trueoptions are not applied to the actual SQL execution.The same issue occurs at line 301 in
find-concepts-in-batches-with-stmt, butfind-concepts-in-table(line 226) correctly demonstrates the proper pattern by passingconntosu/querywithin the transaction block.Proposed fix
- batch-result (su/query db stmt)] + batch-result (su/query conn stmt)]
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)
metadata-db-app/src/cmr/metadata_db/data/oracle/search.clj (1)
249-267:⚠️ Potential issue | 🔴 CriticalCritical bug: Query bypasses the read-only transaction — uses
dbinstead ofconnon line 267.Line 267 executes
(su/query db stmt)using the raw datasourcedb, not the transaction-bound connectionconnestablished on line 250. The:read-only? true :isolation :read-committedoptions have no effect on the actual query. This defeats the entire purpose of this PR.For comparison, every other transaction block in this file (lines 196, 226, 367) correctly uses
connfor queries. Lines 192–196 and 217–226 show the correct pattern: they declare[conn db]with transaction options and executesu/query conn stmt.Proposed fix
batch-result (su/query db stmt)] + batch-result (su/query conn stmt)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metadata-db-app/src/cmr/metadata_db/data/oracle/search.clj` around lines 249 - 267, The query inside the j/with-db-transaction is using the raw datasource `db` instead of the transaction-bound `conn`, so the read-only and isolation options are ignored; change the call to use `conn` (i.e., replace the `su/query db stmt` invocation with `su/query conn stmt`) so the SQL statement built in `stmt` executes within the `j/with-db-transaction` transaction boundary (symbols to update: j/with-db-transaction, conn, db, su/query, stmt).
🧹 Nitpick comments (2)
metadata-db-app/src/cmr/metadata_db/data/oracle/search.clj (2)
301-315: Consider applying the same read-only transaction treatment tofind-concepts-in-batches-with-stmt.This sibling function (not changed in this PR) has the same pattern: it opens a transaction but queries with
dbinstead ofconn(line 312), and lacks the:read-only?/:isolationoptions. If the read-only transaction fix addresses transaction-closure errors forfind-concepts-in-batches, this function likely needs it too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metadata-db-app/src/cmr/metadata_db/data/oracle/search.clj` around lines 301 - 315, The sibling function find-concepts-in-batches-with-stmt opens a transaction but still executes queries against db and does not mark the tx read-only; mirror the fix from find-batch by changing the j/with-db-transaction call in find-concepts-in-batches-with-stmt to bind [conn db] with options {:read-only? true :isolation :read-committed} (or the same options used in find-batch), run the su/query against conn (or pass conn as the DB argument used elsewhere), and ensure subsequent mapping calls use conn (e.g., oc/db-result->concept-map with conn) so the query runs inside the read-only transaction.
253-275: Logging atinfolevel for every batch may be noisy during bulk indexing.During bulk granule indexing (bootstrap), this will produce two
infolog lines per batch. If there are thousands of batches, consider usingdebuginstead, or at least for the per-batch result count log (lines 268–275), to reduce log volume in production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metadata-db-app/src/cmr/metadata_db/data/oracle/search.clj` around lines 253 - 275, The two per-batch info logs in the find-concepts-in-batches block are too noisy; change the second info call that logs batch-result count to use debug instead so per-batch counts are emitted at debug level (replace the (info (format ... (count batch-result) ...)) with a debug-level log), keeping the initial informational context log as-is if desired; reference the (info ...) that formats "Found %d results in batch for provider [%s] concept type ..." and the batch-result/provider-id/concept-type/start-index/batch-size symbols to locate the exact call to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@metadata-db-app/src/cmr/metadata_db/data/oracle/search.clj`:
- Around line 249-267: The query inside the j/with-db-transaction is using the
raw datasource `db` instead of the transaction-bound `conn`, so the read-only
and isolation options are ignored; change the call to use `conn` (i.e., replace
the `su/query db stmt` invocation with `su/query conn stmt`) so the SQL
statement built in `stmt` executes within the `j/with-db-transaction`
transaction boundary (symbols to update: j/with-db-transaction, conn, db,
su/query, stmt).
---
Nitpick comments:
In `@metadata-db-app/src/cmr/metadata_db/data/oracle/search.clj`:
- Around line 301-315: The sibling function find-concepts-in-batches-with-stmt
opens a transaction but still executes queries against db and does not mark the
tx read-only; mirror the fix from find-batch by changing the
j/with-db-transaction call in find-concepts-in-batches-with-stmt to bind [conn
db] with options {:read-only? true :isolation :read-committed} (or the same
options used in find-batch), run the su/query against conn (or pass conn as the
DB argument used elsewhere), and ensure subsequent mapping calls use conn (e.g.,
oc/db-result->concept-map with conn) so the query runs inside the read-only
transaction.
- Around line 253-275: The two per-batch info logs in the
find-concepts-in-batches block are too noisy; change the second info call that
logs batch-result count to use debug instead so per-batch counts are emitted at
debug level (replace the (info (format ... (count batch-result) ...)) with a
debug-level log), keeping the initial informational context log as-is if
desired; reference the (info ...) that formats "Found %d results in batch for
provider [%s] concept type ..." and the
batch-result/provider-id/concept-type/start-index/batch-size symbols to locate
the exact call to change.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2381 +/- ##
==========================================
- Coverage 58.04% 58.02% -0.02%
==========================================
Files 1065 1065
Lines 72992 72998 +6
Branches 2119 2117 -2
==========================================
- Hits 42365 42356 -9
- Misses 28659 28668 +9
- Partials 1968 1974 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5 participants
Overview
What is the objective?
Change the search transaction in find-concepts-in-batches to readonly and test. This change is because the transaction causes an error if it takes to long and SQL closes it. This happens when doing a bulk index of granules.
What are the changes?
Added ":read-committed :read-only? true" to the transaction call as is documented on line and in the macro documentation for the library (see defmacro with-db-transaction).
What areas of the application does this impact?
This error happens when running bulk index through bootstrap
Required Checklist
Additional Checklist
Summary by CodeRabbit