[#874] Grant the offline tools a read-only JDBC transaction instead of refusing it - #880
Open
vharseko wants to merge 1 commit into
Open
Conversation
maximthomas
approved these changes
Aug 19, 2026
…ransaction instead of refusing it export-ldif, verify-index and backendstat open a root container of their own, in READ_ONLY mode, whenever the backend is not already open - a stopped server or a disabled backend. RootContainer.open() asks the storage for a write transaction even in that mode, since that is where it opens the compressed schema and the entry containers, so JDBCStorage refusing to construct one failed all three tools before they read anything. The mode check moves from the constructor of WriteableTransactionTransactionImpl to the mutating operations - openTree(..., true), clearTree, deleteTree, put, update and delete. That is the shape the other three storages of this server already have: PDBStorage.ReadOnlyStorageImpl, JEStorage.ReadOnlyTransactionImpl and CASStorage.checkReadOnly(). The mode is captured once per transaction, since ImporterImpl reopens the storage READ_WRITE under its caller, and it also drives isReadOnly, so a cursor such a transaction opens refuses delete() as well. VLVIndex upgraded an untrusted index of an empty backend to trusted from its constructor, regardless of the access mode. That write moves to afterOpen(txn, createOnDemand), guarded exactly as DefaultIndex.afterOpen() already guards its own: it would otherwise fail the same three tools on an empty backend carrying a VLV index, on PDB and JE too. testReadOnly() caught none of this - it expects a ReadOnlyStorageException, and a storage that fails the open throws one too, from RootContainer.open() rather than from the write it means to be checking. testOfflineToolsOpenBackendReadOnly now runs export-ldif and verify-index against a closed backend for every pluggable backend, and testReadOnlyTransactionReadsButRefusesWrites asserts that the JDBC read-only transaction serves openTree(..., false), read, openCursor and getRecordCount while refusing every mutation.
vharseko
force-pushed
the
issues/874-offline-tools-jdbc-readonly
branch
from
August 20, 2026 10:36
e48d4d1 to
23d63bf
Compare
Member
Author
|
Rebased onto master (0b9c0f6), which had moved on under the JDBC backend since this branch was cut (#886 catalog lookup, #866 table stamping, #867 SQL Server upsert). Conflicts and how they were resolved:
Also checked that the write path master added meanwhile -
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
export-ldif,verify-indexandbackendstatopen a backend read-only when the server is not already holding it open — a stopped server, or a disabled backend.JDBCStoragerefused to hand out a write transaction in that mode, andRootContainer.open()always asks for one, so all three tools failed on a JDBC backend before they read anything:RootContainer.open()asks for a write transaction even inREAD_ONLYmode because that is where it opens the compressed schema and the entry containers. What it actually does through that transaction in read-only mode is onlyopenTree(name, false),read,openCursorandgetRecordCount—PersistentCompressedSchema.load()getsshouldCreate = false,EntryContainer.open()passesshouldCreate = accessMode.isWriteable()down, andDefaultIndex.afterOpen()already guards its own write withcreateOnDemand.Only the offline case is affected. The online task path reuses the root container the running server holds, which is
READ_WRITE, so an onlineexport-ldiftask works.Change
The mode check moves from the constructor of
WriteableTransactionTransactionImplto the mutating operations —openTree(..., true),clearTree,deleteTree,put,updateanddelete. That is the shape the other three storages of this server already have; JDBC was the only outlier of the four:PDBStorageReadOnlyStorageImpl—openTree(createOnDemand)and the mutators throwJEStorageReadOnlyTransactionImpl— sameCASStorageTransactionImpl.checkReadOnly()per operationJDBCStorageThe mode is captured once per transaction rather than read per operation: the access mode of the storage is mutable state —
ImporterImplreopens itREAD_WRITEunder its caller — and a transaction has to keep the mode it was created with. It also drivesisReadOnly, so a cursor such a transaction opens refusesdelete()as well;PDBStorage.ReadOnlyStorageImpl.openCursor()delegates to its writeable implementation and leaves that hole open.VLVIndexno longer writes from its constructor. It upgraded an untrusted index of an empty backend to trusted there, regardless of the access mode, so the same three tools would still fail on an empty backend carrying a VLV index — on PDB and JE too, not only JDBC. The write moves toafterOpen(txn, createOnDemand), guarded exactly asDefaultIndex.afterOpen()already guards its own.EntryContainer.open()and the VLV add-listener both callopen(txn, true)right after constructing the index, and the listener readsisTrusted()only after that call, so the upgrade still happens where it did.Tests
testReadOnly()caught none of this, and could not: it expects aReadOnlyStorageException, and a storage that fails the open throws one too — fromRootContainer.open()rather than from the write the test means to be checking. It was green on the broken code for the wrong reason, and after this change it finally exercises theputit was written for.testOfflineToolsOpenBackendReadOnly(PluggableBackendImplTestCase, so it covers pdb / jeb / jdbc / cassandra at once) — closes the backend and runsexport-ldifandverify-indexagainst it, which is exactly the offline path.testReadOnlyTransactionReadsButRefusesWrites(backends/jdbc/TestCase) — a read-only transaction servesopenTree(..., false),read,openCursorandgetRecordCount, and refusesopenTree(..., true),put,update,delete,deleteTreeandcursor.delete(); a re-open afterwards asserts nothing reached the database.Verified that both tests fail without the fix: on PostgreSQL the suite goes 38/40 with exactly those two red, carrying the stack trace above.
All suites pass locally, no skips:
(The counts above are from the branch as first written; rebased onto
masterthe JDBC suites lose the one test that belongs to #867, and PgSql + PDB + JE re-run at 109/109.)Out of scope
Two neighbouring gaps this does not close, both worth their own issue:
JDBCStorage.listTrees()returns the JVM-localtree2tablememo rather than the tables in the database, sobackendstat list-raw-dbs/dump-raw-dbsee only the trees whose names happened to be hashed in this process.CASStorage.listTrees()returns an empty set outright (TODO). Recovering the real mapping needs the tree name stored on the table, which is what Stamp JDBC backend tables with their tree name and refresh optimizer statistics after import #866 adds.PDBStorageandJEStoragehaveReadOnlyEmpty*implementations for that case; JDBC has no analogue.Fixes #874