Skip to content

Commit 76db2e0

Browse files
committed
Avoid double directory scan when registering a recovered SF ring
SegmentRing.openExisting walked every slot-directory entry to recover segments, then SegmentManager.register immediately re-enumerated the same directory (scanMaxGeneration) to seed the spare file-generation counter, re-allocating a name String plus substring per entry. The recovery scan now tracks the maximum sf-<gen:016x>.sfa generation it observes and carries it on the returned ring (maxRecoveredFileGeneration); register() uses that value and only falls back to the legacy directory rescan for rings that were not built by recovery (FILE_GENERATION_UNKNOWN: fresh start, memory mode, tests), so behavior for non-recovered rings is unchanged. Safety properties, pinned by SegmentRingRecoveryGenerationTest: - the maximum covers every scanned entry, including empty leftovers recovery unlinks and corrupt files it quarantines, so a generation is never reused even for files gone by registration time; - name->generation parsing is a single shared helper (SegmentRing.parseFileGeneration) used by both the recovery scan and the fallback rescan, so the two can never disagree; - freshly minted spares always land strictly above everything on disk -- never at a name whose truncating openCleanRW would destroy a recovered segment. The carried value cannot be stale for its intended (first) registration: recovery and register run on one thread under the engine slot lock, and no spare can be minted into the slot before register wires the ring up. Same-manager re-registration stays safe because advanceFileGeneration only ratchets upward.
1 parent e2fc980 commit 76db2e0

3 files changed

Lines changed: 378 additions & 13 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,25 @@ public void register(SegmentRing ring, String dir, AckWatermark watermark) {
418418
// spare at sf-0000000000000000.sfa — and openCleanRW would truncate the
419419
// user's existing active file out from under the I/O loop, scrambling
420420
// the in-flight mmap. Memory-mode rings have no dir; nothing to scan.
421-
long minNextGeneration = dir == null ? -1L : scanMaxGeneration(dir) + 1L;
421+
//
422+
// A ring produced by SegmentRing.openExisting already carries the
423+
// generation maximum from the full directory scan recovery just
424+
// performed -- prefer it over re-enumerating the directory here.
425+
// The recovered value cannot be stale: the engine holds the slot
426+
// lock, and no spare can be minted into this slot before this call
427+
// wires the ring up. It also cannot be too low after a deregister/
428+
// re-register cycle: advanceFileGeneration only ever ratchets the
429+
// counter upwards. Rings not built by recovery (fresh start, tests)
430+
// report FILE_GENERATION_UNKNOWN and keep the legacy rescan.
431+
long minNextGeneration;
432+
if (dir == null) {
433+
minNextGeneration = -1L;
434+
} else {
435+
long recoveredMax = ring.maxRecoveredFileGeneration();
436+
minNextGeneration = (recoveredMax != SegmentRing.FILE_GENERATION_UNKNOWN
437+
? recoveredMax
438+
: scanMaxGeneration(dir)) + 1L;
439+
}
422440
Runnable managerWakeup = this::wakeWorker;
423441
RingEntry e = new RingEntry(ring, dir, watermark);
424442
// ObjList.add either throws before storing e or makes the entry visible.
@@ -501,7 +519,11 @@ public void wakeWorker() {
501519
/**
502520
* Returns the highest hex-encoded generation across {@code sf-<gen>.sfa}
503521
* files in {@code dir}, or {@code -1} if none exist. Skips files that
504-
* don't match the pattern (e.g. the legacy {@code sf-initial.sfa}).
522+
* don't match the pattern (e.g. the legacy {@code sf-initial.sfa});
523+
* name matching is delegated to {@link SegmentRing#parseFileGeneration}
524+
* so this fallback scan and the recovery scan can never disagree.
525+
* Fallback only: {@link #register(SegmentRing, String, AckWatermark)}
526+
* calls this just for rings that don't carry a recovery-scanned maximum.
505527
*/
506528
private static long scanMaxGeneration(String dir) {
507529
long max = -1L;
@@ -518,17 +540,8 @@ private static long scanMaxGeneration(String dir) {
518540
while (rc > 0) {
519541
String name = Files.utf8ToString(Files.findName(find));
520542
rc = Files.findNext(find);
521-
if (name == null || !name.startsWith("sf-") || !name.endsWith(".sfa")) {
522-
continue;
523-
}
524-
String hex = name.substring(3, name.length() - 4);
525-
if (hex.length() != 16) continue;
526-
try {
527-
long gen = Long.parseUnsignedLong(hex, 16);
528-
if (gen > max) max = gen;
529-
} catch (NumberFormatException ignored) {
530-
// sf-initial.sfa or non-hex — skip
531-
}
543+
long gen = SegmentRing.parseFileGeneration(name);
544+
if (gen > max) max = gen;
532545
}
533546
} finally {
534547
Files.findClose(find);

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,14 @@ public final class SegmentRing implements QuietCloseable {
6060

6161
/** Sentinel: append failed because no hot spare was available to rotate into. */
6262
public static final long BACKPRESSURE_NO_SPARE = -1L;
63+
/**
64+
* {@link #maxRecoveredFileGeneration()} value for rings that were not
65+
* produced by a directory-scanning recovery (constructor-built rings:
66+
* fresh start, memory mode, tests). {@code Long.MIN_VALUE} cannot collide
67+
* with a real scan result, whose minimum is {@code -1} (scanned, but no
68+
* generation-named files found).
69+
*/
70+
public static final long FILE_GENERATION_UNKNOWN = Long.MIN_VALUE;
6371
/** Sentinel: append failed because the payload doesn't fit in a fresh segment. */
6472
public static final long PAYLOAD_TOO_LARGE = -2L;
6573
private static final Logger LOG = LoggerFactory.getLogger(SegmentRing.class);
@@ -106,6 +114,11 @@ public final class SegmentRing implements QuietCloseable {
106114
// manager only notices on its next polling tick -- fine on average,
107115
// but the worst-case wait is the full poll interval. Producer-thread-only.
108116
private Runnable managerWakeup;
117+
// Highest sf-<gen:016x>.sfa file generation observed by the openExisting
118+
// directory scan, or FILE_GENERATION_UNKNOWN for constructor-built rings.
119+
// Written once by openExisting before the ring is published to the
120+
// caller; read-only afterwards (see maxRecoveredFileGeneration()).
121+
private long maxRecoveredFileGeneration = FILE_GENERATION_UNKNOWN;
109122
private long nextSeq;
110123
private volatile long publishedFsn;
111124
// Plain (producer-thread-only) flag; set to true the first time we ask
@@ -170,6 +183,11 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) {
170183
return null;
171184
}
172185
ObjList<MmapSegment> opened = new ObjList<>();
186+
// Highest sf-<gen:016x>.sfa generation seen during the scan; -1 when
187+
// none matched. Carried on the returned ring so SegmentManager's
188+
// register() can seed its spare file-generation counter without
189+
// re-enumerating the directory this scan just walked.
190+
long maxFileGeneration = -1L;
173191
long find = Files.findFirst(sfDir);
174192
if (find < 0) {
175193
// Fail closed: an enumeration failure (permissions, transient
@@ -196,6 +214,16 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) {
196214
int rc = 1;
197215
while (rc > 0) {
198216
String name = Files.utf8ToString(Files.findName(find));
217+
// Track the highest file generation over EVERY entry the
218+
// scan visits, before the unlink/quarantine below can
219+
// remove it. Including removed files keeps the bound
220+
// conservative -- a generation is never reused, so a new
221+
// spare can never collide with a quarantined twin's
222+
// <name>.sfa.corrupt on a future quarantine rename.
223+
long gen = parseFileGeneration(name);
224+
if (gen > maxFileGeneration) {
225+
maxFileGeneration = gen;
226+
}
199227
if (name != null && name.endsWith(".sfa")) {
200228
String path = sfDir + "/" + name;
201229
MmapSegment seg = null;
@@ -355,6 +383,7 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) {
355383
for (int i = 0, n = opened.size(); i < n; i++) {
356384
ring.sealedSegments.add(opened.get(i));
357385
}
386+
ring.maxRecoveredFileGeneration = maxFileGeneration;
358387
return ring;
359388
} catch (Throwable t) {
360389
// Close every recovered MmapSegment that's still in `opened`.
@@ -374,6 +403,31 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) {
374403
}
375404
}
376405

406+
/**
407+
* Parses the spare-file generation out of a {@code sf-<gen:016x>.sfa}
408+
* file name; returns {@code -1} for anything else ({@code sf-initial.sfa},
409+
* non-hex, wrong hex length, {@code null}). Single source of truth shared
410+
* by the recovery scan above and the segment manager's fallback directory
411+
* scan (scanMaxGeneration) so the two can never disagree on which file
412+
* names carry a generation. Note: startsWith + endsWith together imply a
413+
* minimum length of 7 (the prefix and suffix cannot overlap), so the
414+
* substring below cannot go out of bounds.
415+
*/
416+
static long parseFileGeneration(String name) {
417+
if (name == null || !name.startsWith("sf-") || !name.endsWith(".sfa")) {
418+
return -1L;
419+
}
420+
String hex = name.substring(3, name.length() - 4);
421+
if (hex.length() != 16) {
422+
return -1L;
423+
}
424+
try {
425+
return Long.parseUnsignedLong(hex, 16);
426+
} catch (NumberFormatException e) {
427+
return -1L;
428+
}
429+
}
430+
377431
/**
378432
* Highest FSN that the server has ACK'd. Read by the segment manager to
379433
* decide which sealed segments are safe to munmap + unlink.
@@ -643,6 +697,28 @@ public long maxBytesPerSegment() {
643697
return maxBytesPerSegment;
644698
}
645699

700+
/**
701+
* Highest {@code sf-<gen:016x>.sfa} file generation observed during the
702+
* {@link #openExisting} directory scan: {@code -1} when the scan matched
703+
* no generation-named files, or {@link #FILE_GENERATION_UNKNOWN} when
704+
* this ring was not produced by recovery at all (constructor-built:
705+
* fresh start, memory mode, tests). The maximum is taken over every
706+
* entry the scan visited -- including empty leftovers it unlinked and
707+
* corrupt files it quarantined -- so it is a safe upper bound for the
708+
* segment manager's spare file-generation counter even though those
709+
* files are gone by the time the ring is registered.
710+
* <p>
711+
* Valid only for seeding the ring's FIRST registration: it is a
712+
* construction-time snapshot, so once any manager has minted spares
713+
* into the slot it is stale. Re-registering with the SAME manager stays
714+
* safe regardless -- its generation counter only ratchets upward -- but
715+
* a hypothetical hand-off to a different manager instance must rescan
716+
* the directory instead of trusting this value.
717+
*/
718+
public long maxRecoveredFileGeneration() {
719+
return maxRecoveredFileGeneration;
720+
}
721+
646722
/** True when the segment manager should prepare and install a fresh spare. */
647723
public boolean needsHotSpare() {
648724
return hotSpare == null;

0 commit comments

Comments
 (0)