chore(core): adds groundwork for incremental backup + removes undocumented table backup - #6647
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR removes legacy backup functionality, introduces enterprise-only incremental backup support, adds checkpoint recovery configuration with symbol/index rebuilding, implements DataID UUID management with binary I/O, and adds configuration change notification infrastructure for watchers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR involves significant structural changes across checkpoint recovery, DataID management, configuration API, and test infrastructure. The changes are heterogeneous—including binary I/O refactoring, new restoration orchestration logic, API signature updates, configuration removal/addition, and extensive test rewrites—requiring separate reasoning for checkpoint/recovery logic, DataID persistence, config API surface, and test coverage updates. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
| */ | ||
| public boolean isInitialized() { | ||
| return !Uuid.isNull(id.getLo(), id.getHi()); | ||
| public synchronized boolean initialize(long lo, long hi) { |
There was a problem hiding this comment.
confusing semantics, if this is used on startup then perhaps it should not be "synchronized". If it has to be "synchronized" for whatever readon, then "isInitialized" should be synchronized also
There was a problem hiding this comment.
removed synchronized
| @Override | ||
| public void close() { | ||
| futures.clear(); | ||
| executor.shutdownNow(); |
There was a problem hiding this comment.
this does not wait for executor to shutdown
There was a problem hiding this comment.
I'd say it's better not to wait for the termination here. I would rather my DB process exit quickly on recovery error rather than waiting for the symbol indexes to be rebuilt. Normal (non-exceptional) execution uses recoveryAgent.finalizeParallelTasks() to wait.
| } | ||
| } | ||
|
|
||
| public MemoryMARW getMemFile() { |
There was a problem hiding this comment.
document as used in enterprise
| return tableMetadata; | ||
| } | ||
|
|
||
| public TxReader getTableTxReader() { |
| .$(", iteratorMaxTs<").$ts(driver, iteratorMaxTs) | ||
| .$(", iteratorStep=").$(step) | ||
| .$(", iteratorMinTs=[").$ts(driver, iteratorMinTs) | ||
| .$(", ").$ts(driver, iteratorMaxTs) |
There was a problem hiding this comment.
ok, changed to less human-readable but standard
| /** | ||
| * Creates sequencer metadata files from Table Structure object | ||
| */ | ||
| public void createMetaFile(TableStructure tableStruct, Path path, int pathLen, int tableId, int structureVersion) { |
There was a problem hiding this comment.
document why this is unused in OSS
|
|
||
| void authorizeCopyCancel(SecurityContext cancellingSecurityContext); | ||
|
|
||
| void authorizeDatabaseBackup(); |
There was a problem hiding this comment.
document why this is unused in OSS, dangling methods are fragile
There was a problem hiding this comment.
I suppressed the warning on the interface level; there are many unused methods here for obvious reason
|
|
||
| void authorizeDatabaseSnapshot(); | ||
|
|
||
| void authorizeHttp(); |
There was a problem hiding this comment.
supressed, whole file is not needed
| protected static final LowerCaseCharSequenceHashSet KEYWORDS = new LowerCaseCharSequenceHashSet(); | ||
| private static final LowerCaseCharSequenceHashSet TIMESTAMP_PART_SET = new LowerCaseCharSequenceHashSet(); | ||
|
|
||
| public static boolean isAbortKeyword(CharSequence tok) { |
| long squashFileFd = configuration.getFilesFacade().openRO(path.$()); | ||
| Assert.assertTrue("Expected .squash_ts file to exist after squash counter overflow", squashFileFd != -1); | ||
|
|
||
| if (squashFileFd != -1) { |
| } | ||
| } | ||
|
|
||
| private int getSquashCount(String tableName, String partitionDate) { |
There was a problem hiding this comment.
useless parameterization, same value for all calls
There was a problem hiding this comment.
Some IntelliJ warnings just have to be disabled for the project. This is one of them. If many tests by coincidence use the same table name, why this method should have the table name hardcoded?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
core/src/test/java/io/questdb/test/fuzz/FuzzTransactionGenerator.java (1)
204-218: LGTM! Consider consistency with the query branch.The change correctly marks symbol validation transactions as rollback since
FuzzValidateSymbolFilterOperationis a read-only operation that doesn't add a seqTxn (returns false from apply()).The
wantToQuerybranch (lines 212-218) implementsFuzzQueryOperation, which is also read-only and always returns false from apply(). For consistency, consider whetherFuzzQueryOperationshould similarly setrollback = true.core/src/main/java/io/questdb/griffin/SqlCompilerImpl.java (1)
2410-2428:checkpointCreate(..., false)needs clearer intent (boolean-at-callsite risk).
The new overload is fine, but the nakedfalseis a footgun if the meaning ever flips/misread; consider a named constant / enum / overload, or at least a localfinal boolean <meaningfulName> = false;. Also consider aligningresetTimer()with the legacy path by doing it only when!validationOnly.core/src/test/java/io/questdb/test/QuestDBTestNode.java (1)
27-31: Remove unusedPropertyKeyimport.The migration to
ConfigPropertyKeyin this file is complete. ThePropertyKeyimport on line 31 is no longer used and should be removed.pkg/ami/marketplace/assets/server.conf (1)
1093-1093: Remove trailing single quote character.There's a stray single quote
'at the end of the file which appears to be a typo or copy-paste artifact.Proposed fix
-'core/src/main/java/io/questdb/cairo/DatabaseCheckpointAgent.java (3)
92-105:walPurgeJobRunLockshould bevolatile(or set/get under the same lock).
Line 93-95, 130-132:setWalPurgeJobRunLock()is unsynchronized andwalPurgeJobRunLockis non-volatile, but it’s read inside checkpoint create/release paths. That’s a visibility race: checkpointing may “miss” the lock and allow WAL purge to run during checkpoint creation.Proposed fix
- private SimpleWaitingLock walPurgeJobRunLock = null; + private volatile SimpleWaitingLock walPurgeJobRunLock = null;Also applies to: 130-132
117-128: Resource lifecycle:txReaderis never freed inclose().
Line 124: You added a long-livedTxReaderfield (Line 92-105) but don’t free it on close; this risks native/resource leakage depending onTxReaderinternals.Proposed fix
public void close() { lock.lock(); try { Misc.free(path); Misc.free(metadata); Misc.free(tableNameRegistryStore); + Misc.free(txReader); Misc.freeObjList(scoreboards); } finally { lock.unlock(); } }
195-564: View checkpoint copy can produce inconsistent artifacts when seqTxn mismatches (lastTxnstays-1).
Line 343-393: Iftrdr.seqTxn != viewDefinition.getSeqTxn(), the code currently skips dumping the view_txnbut still writestxn_seq/_txnwithlastTxn = -1. This looks like it can create a partially inconsistent checkpoint for views altered concurrently; better to retry the loop until_txnand view definition agree (or skip the view entirely).Proposed fix (retry on mismatch)
// Write table _txn file to checkpoint. Read it safely, it can be changing on view alters. try (var trdr = txReader.ofRO( path.of(configuration.getDbRoot()).concat(tableToken).concat(TXN_FILE_NAME).$(), ColumnType.TIMESTAMP_MICRO, PartitionBy.NOT_APPLICABLE )) { TableUtils.safeReadTxn( trdr, configuration.getMillisecondClock(), configuration.getSpinLockTimeout() ); - if (trdr.seqTxn == viewDefinition.getSeqTxn()) { - lastTxn = trdr.seqTxn; - // Dump _txn file to checkpoint - path.of(checkpointRoot) - .concat(configuration.getDbDirectory()).concat(tableToken) - .concat(TXN_FILE_NAME) - .$(); - mem.smallFile(ff, path.$(), MemoryTag.MMAP_DEFAULT); - trdr.dumpTo(mem); - mem.close(false); - } + if (trdr.seqTxn != viewDefinition.getSeqTxn()) { + LOG.info().$("retrying, view txn out of date [view=").$(tableToken) + .$(", expectedSeqTxn=").$(viewDefinition.getSeqTxn()) + .$(", actualSeqTxn=").$(trdr.seqTxn) + .I$(); + circuitBreaker.statefulThrowExceptionIfTrippedNoThrottle(); + continue; + } + lastTxn = trdr.seqTxn; + // Dump _txn file to checkpoint + path.of(checkpointRoot) + .concat(configuration.getDbDirectory()).concat(tableToken) + .concat(TXN_FILE_NAME) + .$(); + mem.smallFile(ff, path.$(), MemoryTag.MMAP_DEFAULT); + trdr.dumpTo(mem); + mem.close(false); }core/src/main/java/io/questdb/cairo/mv/MatViewRefreshJob.java (1)
1454-1469: Fatal-mode log message contradicts behavior (rethrow vs “falling back”).When
isMatViewRefreshMissingWalFilesFatal()is true, the code rethrowsex, so there is no full-refresh fallback. Consider adjusting the log wording to avoid confusion (and/or scope the fatal behavior to the expected “missing WAL files” cases only).Proposed tweak (log message)
- LOG.critical().$("could not read WAL transactions, falling back to full refresh [view=").$(viewToken) + LOG.critical().$("could not read WAL transactions; mat view refresh configured as fatal [view=").$(viewToken) .$(", ex=").$safe(ex.getFlyweightMessage()) .$(", errno=").$(ex.getErrno()) .I$(); throw ex;
🤖 Fix all issues with AI agents
In `@core/src/main/java/io/questdb/cairo/security/AllowAllSecurityContext.java`:
- Around line 172-174: Remove the dead branches in SecurityContextTest that
check for the now-removed authorizeTableBackup method and any conditional logic
tied to that name; update the test to rely on the current SecurityContext API
(authorizeDatabaseBackup) or remove the obsolete checks entirely so the test no
longer references authorizeTableBackup, ensuring coverage uses the new
authorizeDatabaseBackup behavior via existing
AllowAllSecurityContext/ReadOnlySecurityContext instances or explicit
assertions.
In `@core/src/main/java/io/questdb/cairo/TableReaderMetadata.java`:
- Around line 68-69: writerColumnCount is only updated in readFromMem(), so
after metadata transitions getWriterColumnCount() can return stale data; update
writerColumnCount inside applyTransition0(...) (the code path used by
applyTransition() and applyTransitionFrom()) to recompute and set the current
writer column count whenever metadata is rebuilt. Locate applyTransition0(...),
compute the writer column count from the new metadata (same logic used in
readFromMem()) and assign it to the writerColumnCount field so transitions keep
the value in sync with getWriterColumnCount().
In `@core/src/main/java/io/questdb/cairo/TableSnapshotRestore.java`:
- Around line 457-461: In TableSnapshotRestore (the block handling ff.copy
failure) capture ff.errno() into a local variable immediately after the copy
fails, then use that saved errno to check Files.isErrnoFileDoesNotExist(err)
instead of calling ff.errno() again; remove the redundant ff.exists(srcPath.$())
check (or, if you still want the existence guard, call ff.exists only after
saving errno and use both conditions). This prevents the race where ff.exists()
overwrites ff.errno() and ensures the optional-file logic uses the original
errno value.
In `@core/src/main/java/io/questdb/cairo/TableWriter.java`:
- Around line 10106-10114: The helper method
squashSplitPartitions_updateSquashTimestampFile can call ff.close(-1) when open
fails and also lacks a finally/try-with-resources to guarantee the file is
closed if an exception occurs; update that method to (1) use a
try-with-resources or an explicit try/finally so the file descriptor is closed
only if it was successfully opened, (2) guard any ff.close(...) call with a
check that the file descriptor/handle is valid (>=0), and (3) catch and log IO
exceptions from open/write so they don't propagate; leave the call-site in
TableWriter.squashSplitPartitions as-is.
In `@core/src/test/java/io/questdb/test/cairo/mv/MatViewTest.java`:
- Around line 3167-3171: The test sets
DEBUG_MAT_VIEW_REFRESH_MISSING_WAL_FILES_FATAL globally before calling
assertMemoryLeak, which can leak into other tests; update
testIncrementalRefreshRecoversWhenWalSegmentIsGone to scope the flag change by
either moving the
setProperty(PropertyKey.DEBUG_MAT_VIEW_REFRESH_MISSING_WAL_FILES_FATAL, "false")
call inside the lambda passed to assertMemoryLeak (so it’s only in effect for
that test body) or capture the previous value and restore it in a finally block
after the test body; reference the test method name and the PropertyKey constant
when making the change, and consider adding a paired test that sets the flag to
"true" to assert fatal behavior if needed.
In `@core/src/test/java/io/questdb/test/DynamicPropServerConfigurationTest.java`:
- Around line 138-213: The test testConfigChangeListener is brittle because it
asserts exact watch IDs (watchId == 0 and watchId2 == 1); change assertions
around serverMain.getEngine().getConfigReloader().watch(...) so they only check
that returned IDs are non-negative (>= 0) and that the second ID differs from
the first (watchId2 != watchId) rather than exact values, and update any
comments accordingly; also consider wrapping assertions that rely on async
notifications of configChangedCalledCounter in TestUtils.assertEventually(...)
to avoid flakiness when callbacks are asynchronous.
🧹 Nitpick comments (23)
core/src/test/java/io/questdb/test/cutlass/http/ScoreboardHandlingTest.java (1)
40-48: Remove unusedbackupRootfield and initialization.The
backupRootfield and its initialization insetUpStatic()are now dead code since the backup-related environment variable and DDL step were removed. Clean this up to complete the backup removal.♻️ Suggested cleanup
public class ScoreboardHandlingTest extends AbstractBootstrapTest { - static String backupRoot; - `@BeforeClass` public static void setUpStatic() throws Exception { AbstractBootstrapTest.setUpStatic(); - unchecked(() -> { - backupRoot = temp.newFolder("backups").getAbsolutePath(); - }); }core/rust/qdb-core/src/col_driver/mapped.rs (1)
137-197: Consider adding tests for versioned path handling.The existing tests cover the non-versioned
openwrapper, but there's no test coverage for:
build_file_extensionwithSome(version)open_versionedwith a non-NoneversionSince this is groundwork for incremental backup, testing the versioned paths would help ensure the extension formatting (
d.{version},i.{version}) and path handling work correctly.Example test additions
#[test] fn test_build_file_extension_no_version() { let ext = MappedColumn::build_file_extension("d", None); assert_eq!(ext.as_ref(), "d"); } #[test] fn test_build_file_extension_with_version() { let ext = MappedColumn::build_file_extension("d", Some(123)); assert_eq!(ext.as_ref(), "d.123"); }You would also need test data files with versioned extensions (e.g.,
col_name.d.1) to testopen_versionedwith a version.core/src/main/java/io/questdb/griffin/SqlCompilerImpl.java (1)
4818-4822: Update enterprise-only BACKUP error message to clarify OSS backup syntax.The current message "please use SNAPSHOT backup" is outdated and vague. According to QuestDB documentation, SNAPSHOT commands are deprecated; the OSS equivalent is
CHECKPOINT CREATE(followed byCHECKPOINT RELEASEafter backup completes).Suggested revision:
Proposed diff (clearer guidance)
protected void compileBackup(SqlExecutionContext executionContext, `@Transient` CharSequence sqlText) throws SqlException { - throw SqlException.$(lexer.lastTokenPosition(), - "incremental backup is supported in QuestDB enterprise version only, please use SNAPSHOT backup" - ); + throw SqlException.$( + lexer.lastTokenPosition(), + "BACKUP is supported in QuestDB Enterprise only; use CHECKPOINT CREATE/RELEASE for backups in OSS" + ); }This replaces the deprecated SNAPSHOT reference and explicitly names the OSS workflow:
CHECKPOINT CREATEto prepare, thenCHECKPOINT RELEASEto complete the backup.core/src/test/java/io/questdb/test/cutlass/http/JsonExecuteApiFuzzTest.java (1)
83-86: Test update matches new backup semantics; consider de-brittling exact error text if it’s not a stable API.Changing from table backup to
backup databaseand asserting the enterprise-only incremental backup error looks consistent with the PR objective. If the exact wording is not guaranteed stable, consider asserting on a smaller invariant substring (or an error code/tag, if available) to reduce churn.core/src/main/java/io/questdb/cairo/TableUtils.java (1)
165-165: Consider referencingTXN_FILE_NAMEinstead of duplicating the value.
CHECKPOINT_SEQ_TXN_FILE_NAMEduplicates the value ofTXN_FILE_NAME(Line 164). If this is intentional for semantic clarity, consider defining it as a reference to avoid drift:- public static final String CHECKPOINT_SEQ_TXN_FILE_NAME = "_txn"; + public static final String CHECKPOINT_SEQ_TXN_FILE_NAME = TXN_FILE_NAME;If these are expected to diverge in the future, please add a comment explaining the distinction.
core/src/main/java/io/questdb/cairo/wal/seq/SequencerMetadata.java (1)
163-170: Consider documenting thattableTokenremains null after this method.The
copyFrom(tableStruct, tableId)call resets the metadata state includingtableTokento null, and this method never sets it. If callers subsequently usegetTableToken(), they will receive null. If this is intentional for backup/restore scenarios, consider adding a Javadoc note to clarify the post-condition.core/src/test/java/io/questdb/test/griffin/engine/functions/catalogue/CurrentDataIDFunctionFactoryTest.java (1)
83-97: Remove commented-out dead code.This commented-out test references APIs that no longer exist (e.g.,
engine.getDataID().set()). Consider removing it entirely rather than leaving it as comments, or converting it to a valid test if the functionality is still needed.core/src/main/java/io/questdb/preferences/SettingsStore.java (1)
86-93: Consider addingsynchronizedfor consistency with other methods accessingpreferencesMap.While this method is marked
@TestOnly, it readspreferencesMapwithout synchronization, whereas other methods that access this field (exportPreferences,save,persistTo) are synchronized. This inconsistency could cause visibility issues if the test runs concurrent operations.♻️ Suggested fix
`@TestOnly` - public void observe(`@NotNull` PreferencesUpdateListener listener) { + public synchronized void observe(`@NotNull` PreferencesUpdateListener listener) { listener.update(preferencesMap); }core/src/main/java/io/questdb/cairo/DataID.java (1)
94-96: Returning mutable internal state exposes DataID to external modification.The
Uuidclass has a publicof(long lo, long hi)method that allows mutation. By returning the internalidobject directly, callers can modify the DataID's state without going through the synchronizedchange()orinitialize()methods.Consider whether this is intentional for performance, or if a defensive copy would be safer:
♻️ Defensive copy option
public Uuid get() { - return id; + return new Uuid(id.getLo(), id.getHi()); }core/src/main/java/io/questdb/PropServerConfiguration.java (1)
183-186: Improve checkpoint recovery threadpool validation errors (include the bad values).The wiring + getters look good; the validation would be easier to troubleshoot if exceptions included the offending values (and optionally the min/max pair when
min > max).Proposed tweak (more actionable exceptions)
- if (checkpointRecoveryThreadpoolMinRaw < 2 || checkpointRecoveryThreadpoolMinRaw > 32) { - throw new ServerConfigurationException(PropertyKey.CAIRO_CHECKPOINT_RECOVERY_THREADPOOL_MIN.getPropertyPath() + " must be between 2 and 32"); - } - if (checkpointRecoveryThreadpoolMaxRaw < 2 || checkpointRecoveryThreadpoolMaxRaw > 32) { - throw new ServerConfigurationException(PropertyKey.CAIRO_CHECKPOINT_RECOVERY_THREADPOOL_MAX.getPropertyPath() + " must be between 2 and 32"); - } - if (checkpointRecoveryThreadpoolMinRaw > checkpointRecoveryThreadpoolMaxRaw) { - throw new ServerConfigurationException(PropertyKey.CAIRO_CHECKPOINT_RECOVERY_THREADPOOL_MIN.getPropertyPath() + " must be less than or equal to " + PropertyKey.CAIRO_CHECKPOINT_RECOVERY_THREADPOOL_MAX.getPropertyPath()); - } + if (checkpointRecoveryThreadpoolMinRaw < 2 || checkpointRecoveryThreadpoolMinRaw > 32) { + throw new ServerConfigurationException(PropertyKey.CAIRO_CHECKPOINT_RECOVERY_THREADPOOL_MIN.getPropertyPath() + + " must be between 2 and 32 [value=" + checkpointRecoveryThreadpoolMinRaw + ']'); + } + if (checkpointRecoveryThreadpoolMaxRaw < 2 || checkpointRecoveryThreadpoolMaxRaw > 32) { + throw new ServerConfigurationException(PropertyKey.CAIRO_CHECKPOINT_RECOVERY_THREADPOOL_MAX.getPropertyPath() + + " must be between 2 and 32 [value=" + checkpointRecoveryThreadpoolMaxRaw + ']'); + } + if (checkpointRecoveryThreadpoolMinRaw > checkpointRecoveryThreadpoolMaxRaw) { + throw new ServerConfigurationException(PropertyKey.CAIRO_CHECKPOINT_RECOVERY_THREADPOOL_MIN.getPropertyPath() + + " must be <= " + PropertyKey.CAIRO_CHECKPOINT_RECOVERY_THREADPOOL_MAX.getPropertyPath() + + " [min=" + checkpointRecoveryThreadpoolMinRaw + ", max=" + checkpointRecoveryThreadpoolMaxRaw + ']'); + }Also applies to: 953-966, 3086-3098
core/src/test/java/io/questdb/test/cairo/o3/O3SquashPartitionTest.java (1)
239-254: Redundant condition and potential resource handling improvement.The
if (squashFileFd != -1)check at line 247 is redundant since the assertion at line 245 would have already failed ifsquashFileFd == -1. Consider simplifying:♻️ Suggested cleanup
path.concat(TableUtils.PARTITION_LAST_SQUASH_TIMESTAMP_FILE).$(); long squashFileFd = configuration.getFilesFacade().openRO(path.$()); Assert.assertTrue("Expected .squash_ts file to exist after squash counter overflow", squashFileFd != -1); - - if (squashFileFd != -1) { - long squashTimestamp = configuration.getFilesFacade().readNonNegativeLong(squashFileFd, 0); - Assert.assertTrue("Expected valid squash timestamp, got: " + squashTimestamp, squashTimestamp > 0); - configuration.getFilesFacade().close(squashFileFd); - } + try { + long squashTimestamp = configuration.getFilesFacade().readNonNegativeLong(squashFileFd, 0); + Assert.assertTrue("Expected valid squash timestamp, got: " + squashTimestamp, squashTimestamp > 0); + } finally { + configuration.getFilesFacade().close(squashFileFd); + }core/src/main/java/io/questdb/ConfigReloader.java (1)
68-106: Consider simplifyingnextWatchIdfrom AtomicLong to plain long.Since all methods in
WatchRegistryaresynchronized, theAtomicLongfornextWatchIdprovides no additional thread-safety benefit. A plainlongwould suffice and be simpler.Proposed simplification
class WatchRegistry { public static final long UNREGISTERED = -1; - private final AtomicLong nextWatchId = new AtomicLong(UNREGISTERED + 1); + private long nextWatchId = UNREGISTERED + 1; private final LongObjHashMap<Listener> watchers = new LongObjHashMap<>(); private ObjHashSet<String> changedKeys = null; // ... public synchronized long watch(ConfigReloader.Listener listener) { - final long watchId = nextWatchId.getAndIncrement(); + final long watchId = nextWatchId++; watchers.put(watchId, listener); return watchId; }core/src/test/java/io/questdb/test/cairo/DataIDTest.java (2)
86-120: Restart test is good; consider snapshotting lo/hi instead of holding a mutableUuidreference.
Line 91-108:DataID.get()returns the internalUuid(mutable). It’s fine here since you only read it, but capturinglong lo/hiimmediately would make the test more future-proof.
184-210: RFC 4122 big-endian on-disk bytes are validated nicely; add an explicit length assert for clearer failures.
Line 206-209:Arrays.copyOf(actual, 16)hides “wrong length” vs “wrong contents”; assertingactual.length == 16first would improve diagnostics.core/src/main/java/io/questdb/cairo/CairoConfigurationWrapper.java (1)
1414-1417:isMatViewRefreshMissingWalFilesFatal()delegation is fine; consider failing fast ifdelegateis unset.
Line 1414-1417: If this wrapper can be constructed with a null delegate (Line 54-57) and accessed beforesetDelegate(), you’ll still get an NPE—might be worth a clearer failure mode ingetDelegate().core/src/test/java/io/questdb/test/griffin/CheckpointTest.java (3)
2244-2260:copyDirectoryis fine for test fixtures; consider copying attributes only if you hit permissions issues.
Line 2244-2259: Works as a simple “clone install root” helper for these tests.
2262-2291: Bitmap index file poking is powerful but brittle (magic offsets + unsorted file selection).
- Line 2262-2290 / 2869-2934:
MAX_VALUE_OFFSET = 37Land pickingkeyFiles[0]can become fragile if file format or directory listing order changes.- Recommend (a) prefer existing constants/helpers for header offsets if available, and (b) pick the newest
.k*by txn suffix / mtime, or assert only one key file exists for the test table.Also applies to: 2822-2941
2507-2607: These rebuild vs preserve tests likely assume index usage; make that assumption explicit to avoid planner-driven flakes.
Line 2594-2603: If the planner ever stops using the symbol index forsym = 'SYM'on small tables, the “should throw” branch could stop throwing. Consider asserting the plan uses the index (e.g., viaEXPLAIN) before expecting the corruption failure.Also applies to: 2609-2728
core/src/main/java/io/questdb/cairo/DatabaseCheckpointAgent.java (1)
139-177: Orphan-dir removal + preferences restore helpers are good; tighten logs and (optionally) directory filtering.
- Line 160-169: The log line seems to miss the closing
]and doesn’t include the candidate name; also consider filtering removals to table dirs (e.g.,*~) if non-table dirs can exist underdbRoot.- Line 183-191: On copy failure, logging src/dst would help diagnosis.
Also applies to: 179-193
core/src/main/java/io/questdb/cairo/TableSnapshotRestore.java (3)
104-112: Consider awaiting termination aftershutdownNow().
shutdownNow()attempts to stop running tasks but doesn't wait for them to complete. If parallel tasks are still writing index files or symbol files whenclose()returns, callers may encounter partially written files.♻️ Suggested improvement
`@Override` public void close() { futures.clear(); executor.shutdownNow(); + try { + if (!executor.awaitTermination(5, java.util.concurrent.TimeUnit.SECONDS)) { + LOG.error().$("executor did not terminate in time").$(); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } tableMetadata = Misc.free(tableMetadata); txWriter = Misc.free(txWriter); columnVersionReader = Misc.free(columnVersionReader); memFile = Misc.free(memFile); }
161-187: Clear futures list after processing to prevent re-processing.If
finalizeParallelTasks()is called twice, the same futures will be awaited again (which will return immediately but wastes cycles). Also, prefer!futures.isEmpty()overfutures.size() > 0for clarity.♻️ Suggested fix
public void finalizeParallelTasks() { - if (futures.size() > 0) { + if (!futures.isEmpty()) { LOG.info().$("awaiting ").$(futures.size()).$(" parallel tasks to complete").I$(); } for (int i = 0, n = futures.size(); i < n; i++) { try { futures.getQuick(i).get(); } catch (InterruptedException e) { // ... existing handling ... } catch (ExecutionException e) { // ... existing handling ... } } + futures.clear(); }
838-900: Consider cachinggetIndexedParquetColumnIndexresults to avoid repeated calls.The method calls
getIndexedParquetColumnIndexup to 3 times per column (lines 843, 865, 910). While correctness is maintained, this could be optimized by caching results from the first pass.This is a minor optimization that could be addressed in a follow-up if performance profiling indicates it's worthwhile. The current implementation prioritizes readability with distinct passes for: 1) logging, 2) resource setup, 3) columnTop collection.
core/src/main/java/io/questdb/cairo/TxWriter.java (1)
241-255: incrementPartitionSquashCounter(): consider adding an assert and reusing bumpPartitionTableVersion().This reduces duplication and will catch accidental misuse (bad partitionIndex) earlier in dev/test builds.
Proposed refactor
public boolean incrementPartitionSquashCounter(int partitionIndex) { final int partitionRawIndex = partitionIndex * LONGS_PER_TX_ATTACHED_PARTITION; + assert partitionIndex >= 0 && partitionRawIndex + LONGS_PER_TX_ATTACHED_PARTITION <= attachedPartitions.size(); int partitionSquashCounter = getPartitionSquashCountByRawIndex(partitionRawIndex); if (partitionSquashCounter == PARTITION_SQUASH_COUNTER_MAX) { // This means 16bit unsigned value is overflown. // Return false so that the caller can fall back to an alternative way to track squashes. return false; } setPartitionSquashCounterByRawIndex(partitionRawIndex, (short) (partitionSquashCounter + 1)); // Bump versions to make sure that incremental txn update will save the change // and incremental txn read will read it - recordStructureVersion++; - partitionTableVersion++; + bumpPartitionTableVersion(); return true; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
core/rust/qdb-core/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (68)
.gitignoreartifacts/tsbs/ilp_perf/README.txtartifacts/tsbs/ilp_perf/amazon.txtartifacts/tsbs/ilp_perf/patrick.cpu.txtartifacts/tsbs/ilp_perf/questbook.cpu.txtartifacts/tsbs/ilp_perf/questdb-ec2.txtcore/rust/qdb-core/src/col_driver/mapped.rscore/rust/qdb-core/src/col_type.rscore/src/main/java/io/questdb/Bootstrap.javacore/src/main/java/io/questdb/ConfigReloader.javacore/src/main/java/io/questdb/DynamicPropServerConfiguration.javacore/src/main/java/io/questdb/PropServerConfiguration.javacore/src/main/java/io/questdb/PropertyKey.javacore/src/main/java/io/questdb/ServerMain.javacore/src/main/java/io/questdb/cairo/CairoConfiguration.javacore/src/main/java/io/questdb/cairo/CairoConfigurationWrapper.javacore/src/main/java/io/questdb/cairo/CairoEngine.javacore/src/main/java/io/questdb/cairo/DataID.javacore/src/main/java/io/questdb/cairo/DatabaseCheckpointAgent.javacore/src/main/java/io/questdb/cairo/DefaultCairoConfiguration.javacore/src/main/java/io/questdb/cairo/SecurityContext.javacore/src/main/java/io/questdb/cairo/SymbolMapUtil.javacore/src/main/java/io/questdb/cairo/TableNameRegistry.javacore/src/main/java/io/questdb/cairo/TableReaderMetadata.javacore/src/main/java/io/questdb/cairo/TableSnapshotRestore.javacore/src/main/java/io/questdb/cairo/TableUtils.javacore/src/main/java/io/questdb/cairo/TableWriter.javacore/src/main/java/io/questdb/cairo/TxReader.javacore/src/main/java/io/questdb/cairo/TxWriter.javacore/src/main/java/io/questdb/cairo/mv/MatViewRefreshJob.javacore/src/main/java/io/questdb/cairo/security/AllowAllSecurityContext.javacore/src/main/java/io/questdb/cairo/security/ReadOnlySecurityContext.javacore/src/main/java/io/questdb/cairo/wal/ApplyWal2TableJob.javacore/src/main/java/io/questdb/cairo/wal/seq/SequencerMetadata.javacore/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessor.javacore/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.javacore/src/main/java/io/questdb/cutlass/text/ParallelCsvFileImporter.javacore/src/main/java/io/questdb/griffin/CompiledQuery.javacore/src/main/java/io/questdb/griffin/CompiledQueryImpl.javacore/src/main/java/io/questdb/griffin/SqlCompilerImpl.javacore/src/main/java/io/questdb/griffin/SqlKeywords.javacore/src/main/java/io/questdb/preferences/SettingsStore.javacore/src/main/resources/io/questdb/site/conf/server.confcore/src/test/java/io/questdb/test/AbstractCairoTest.javacore/src/test/java/io/questdb/test/DynamicPropServerConfigurationTest.javacore/src/test/java/io/questdb/test/PropServerConfigurationTest.javacore/src/test/java/io/questdb/test/QuestDBTestNode.javacore/src/test/java/io/questdb/test/ServerMainTest.javacore/src/test/java/io/questdb/test/cairo/DataIDTest.javacore/src/test/java/io/questdb/test/cairo/Overrides.javacore/src/test/java/io/questdb/test/cairo/TableReaderMetadataTest.javacore/src/test/java/io/questdb/test/cairo/TxnTest.javacore/src/test/java/io/questdb/test/cairo/mv/MatViewFuzzTest.javacore/src/test/java/io/questdb/test/cairo/mv/MatViewTest.javacore/src/test/java/io/questdb/test/cairo/o3/O3PartitionPurgeTest.javacore/src/test/java/io/questdb/test/cairo/o3/O3SquashPartitionTest.javacore/src/test/java/io/questdb/test/cutlass/http/JsonExecuteApiFuzzTest.javacore/src/test/java/io/questdb/test/cutlass/http/ScoreboardHandlingTest.javacore/src/test/java/io/questdb/test/cutlass/http/SqlValidationTest.javacore/src/test/java/io/questdb/test/cutlass/pgwire/PGSecurityTest.javacore/src/test/java/io/questdb/test/fuzz/FuzzTransactionGenerator.javacore/src/test/java/io/questdb/test/griffin/CheckpointTest.javacore/src/test/java/io/questdb/test/griffin/SecurityTest.javacore/src/test/java/io/questdb/test/griffin/TableBackupTest.javacore/src/test/java/io/questdb/test/griffin/engine/functions/catalogue/CurrentDataIDFunctionFactoryTest.javacore/src/test/java/io/questdb/test/sqllogictest/AbstractSqllogicTestRunner.javacore/src/test/resources/sqllogictest/test/sql/test_alter_backup_wal_table.testpkg/ami/marketplace/assets/server.conf
💤 Files with no reviewable changes (13)
- artifacts/tsbs/ilp_perf/README.txt
- core/src/test/java/io/questdb/test/cutlass/pgwire/PGSecurityTest.java
- artifacts/tsbs/ilp_perf/amazon.txt
- core/src/main/java/io/questdb/griffin/CompiledQueryImpl.java
- artifacts/tsbs/ilp_perf/questdb-ec2.txt
- core/src/test/java/io/questdb/test/sqllogictest/AbstractSqllogicTestRunner.java
- artifacts/tsbs/ilp_perf/patrick.cpu.txt
- core/src/test/java/io/questdb/test/AbstractCairoTest.java
- core/src/main/java/io/questdb/Bootstrap.java
- core/src/test/java/io/questdb/test/griffin/SecurityTest.java
- core/src/test/resources/sqllogictest/test/sql/test_alter_backup_wal_table.test
- core/src/test/java/io/questdb/test/griffin/TableBackupTest.java
- artifacts/tsbs/ilp_perf/questbook.cpu.txt
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-19T12:21:00.062Z
Learnt from: jerrinot
Repo: questdb/questdb PR: 6413
File: core/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.java:11982-12002
Timestamp: 2025-11-19T12:21:00.062Z
Learning: QuestDB Java tests use a deterministic random seed. The test utilities (e.g., io.questdb.test.tools.TestUtils and io.questdb.std.Rnd) produce reproducible sequences, so rnd_* functions (including rnd_uuid4) yield deterministic outputs across runs. Do not flag tests in core/src/test/** that assert against values produced by rnd_* as flaky due to randomness.
Applied to files:
core/src/test/java/io/questdb/test/griffin/engine/functions/catalogue/CurrentDataIDFunctionFactoryTest.javacore/src/main/java/io/questdb/ServerMain.javacore/src/test/java/io/questdb/test/griffin/CheckpointTest.javacore/src/test/java/io/questdb/test/cairo/DataIDTest.javacore/src/main/java/io/questdb/cairo/DataID.javacore/src/test/java/io/questdb/test/DynamicPropServerConfigurationTest.javacore/src/main/java/io/questdb/cairo/CairoEngine.java
📚 Learning: 2025-11-07T00:59:31.522Z
Learnt from: bluestreak01
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2025-11-07T00:59:31.522Z
Learning: In QuestDB's Cairo engine, transaction (_txn) files have a strong invariant: they are never truncated below TX_BASE_HEADER_SIZE. Once created, they are either fully formed (size >= header size) or completely removed along with the entire table directory when the table is dropped.
Applied to files:
core/src/main/java/io/questdb/cairo/TableUtils.javacore/src/main/java/io/questdb/cairo/wal/ApplyWal2TableJob.javacore/src/test/java/io/questdb/test/cairo/TxnTest.javacore/src/main/java/io/questdb/cairo/DatabaseCheckpointAgent.java
📚 Learning: 2025-11-07T00:59:31.522Z
Learnt from: bluestreak01
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2025-11-07T00:59:31.522Z
Learning: When checking for transaction file validity in QuestDB, use `ff.length(path) >= TX_BASE_HEADER_SIZE` instead of `ff.exists(path)`. The length check provides stronger guarantees under extreme system load by ensuring the file system catalog is synchronized and the file is fully formed, preventing SIGBUS errors when memory mapping the file.
Applied to files:
core/src/main/java/io/questdb/cairo/TableUtils.java
📚 Learning: 2026-01-07T19:04:55.646Z
Learnt from: CR
Repo: questdb/questdb PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-07T19:04:55.646Z
Learning: Store data by column for compression and vectorized operations in the cairo storage engine
Applied to files:
core/src/main/java/io/questdb/ServerMain.javacore/src/test/java/io/questdb/test/cairo/o3/O3SquashPartitionTest.java
📚 Learning: 2026-01-08T21:35:17.240Z
Learnt from: glasstiger
Repo: questdb/questdb PR: 6612
File: core/src/main/java/io/questdb/cairo/TableNameRegistryStore.java:313-325
Timestamp: 2026-01-08T21:35:17.240Z
Learning: In TableNameRegistryStore.java's readTableId() method, NumericException from getTableIdFromTableDir() for view directories is intentionally allowed to propagate and fail database startup. This fail-fast behavior is correct because corrupted view directory names indicate database tampering, and views with table id 0 would not show obvious failures. Regular tables without _meta files return 0 to allow database startup for resilience.
Applied to files:
core/src/main/java/io/questdb/cairo/DatabaseCheckpointAgent.javacore/src/test/java/io/questdb/test/griffin/CheckpointTest.java
🧬 Code graph analysis (14)
core/rust/qdb-core/src/col_type.rs (1)
core/rust/qdbr/src/parquet/mod.rs (2)
into_type(37-37)into_type(41-43)
core/src/test/java/io/questdb/test/griffin/engine/functions/catalogue/CurrentDataIDFunctionFactoryTest.java (2)
core/src/main/java/io/questdb/cairo/DataID.java (1)
DataID(46-220)core/src/test/java/io/questdb/test/AbstractBootstrapTest.java (1)
AbstractBootstrapTest(58-318)
core/rust/qdb-core/src/col_driver/mapped.rs (1)
core/rust/qdb-core/src/col_type.rs (1)
fixed_size(108-140)
core/src/main/java/io/questdb/ServerMain.java (2)
core/src/main/java/io/questdb/cairo/DataID.java (1)
DataID(46-220)core/src/main/java/io/questdb/std/Uuid.java (1)
Uuid(32-250)
core/src/test/java/io/questdb/test/cairo/TxnTest.java (1)
core/src/main/java/io/questdb/cairo/TxReader.java (1)
TxReader(47-842)
core/src/test/java/io/questdb/test/griffin/CheckpointTest.java (5)
core/src/main/java/io/questdb/cairo/BitmapIndexUtils.java (1)
BitmapIndexUtils(33-268)core/src/main/java/io/questdb/preferences/SettingsStore.java (1)
SettingsStore(27-191)core/src/main/java/io/questdb/cairo/mv/MatViewState.java (1)
MatViewState(50-452)core/src/main/java/io/questdb/cairo/vm/Vm.java (1)
Vm(39-128)core/rust/qdbr/src/parquet_read/meta.rs (1)
read(37-157)
core/src/test/java/io/questdb/test/cairo/DataIDTest.java (2)
core/src/main/java/io/questdb/cairo/DataID.java (1)
DataID(46-220)core/src/main/java/io/questdb/std/Uuid.java (1)
Uuid(32-250)
core/src/main/java/io/questdb/griffin/SqlCompilerImpl.java (1)
core/src/main/java/io/questdb/griffin/SqlException.java (1)
SqlException(37-273)
core/src/main/java/io/questdb/cairo/SymbolMapUtil.java (1)
core/src/main/java/io/questdb/cairo/TableUtils.java (1)
TableUtils(88-2180)
core/src/main/java/io/questdb/cairo/DataID.java (4)
core/src/main/java/io/questdb/std/Unsafe.java (1)
Unsafe(40-542)core/src/main/java/io/questdb/std/Uuid.java (1)
Uuid(32-250)core/src/main/java/io/questdb/std/Numbers.java (1)
Numbers(44-3523)core/src/main/java/io/questdb/std/MemoryTag.java (1)
MemoryTag(27-184)
core/src/main/java/io/questdb/cairo/TableSnapshotRestore.java (4)
core/src/main/java/io/questdb/cairo/wal/WalUtils.java (1)
WalUtils(46-259)core/src/main/java/io/questdb/cairo/TableUtils.java (1)
TableUtils(88-2180)core/src/main/java/io/questdb/cairo/BitmapIndexUtils.java (1)
BitmapIndexUtils(33-268)core/src/main/java/io/questdb/cairo/BitmapIndexWriter.java (1)
BitmapIndexWriter(45-584)
core/src/main/java/io/questdb/cairo/TableWriter.java (1)
core/src/main/java/io/questdb/cairo/TableUtils.java (1)
TableUtils(88-2180)
core/src/main/java/io/questdb/ConfigReloader.java (1)
core/src/main/java/io/questdb/std/LongObjHashMap.java (1)
LongObjHashMap(29-135)
core/src/main/java/io/questdb/cairo/CairoEngine.java (2)
core/src/main/java/io/questdb/std/Rnd.java (1)
Rnd(39-491)core/src/main/java/io/questdb/ConfigReloader.java (1)
WatchRegistry(68-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (35)
- GitHub Check: New pull request (Coverage Report Coverage Report)
- GitHub Check: New pull request (Hosted Running tests on windows-other-2)
- GitHub Check: New pull request (Hosted Running tests on windows-other-1)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-2)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-1)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
- GitHub Check: New pull request (Trigger Enterprise CI Trigger Enterprise Pipeline)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
- GitHub Check: New pull request (Check Changes Check changes)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
[PR Coverage check]😍 pass : 637 / 891 (71.49%) file detail
|
4 participants
Same as #6558, which was merged too early and reverted
Tandem PR: https://github.com/questdb/questdb-enterprise/pull/647
Removes the undocumented table backup.