Repository navigation
U8 backup accessors + the Java version break, G13 measured, D0 validated, B1 resolved, and the mechanical five - #209
Merged
Merged
Conversation
setBackupReadCount, setBackupReadSleep, setBackupSize and setBackupWriteDirect
existed and looked correct, but were WRITE-ONLY: each stored to a private field
and nothing ever pushed the value into DB_ENV->set_backup_config. Calling them
had no effect at all. The getters returned the Java field, so an accessor
round-trip test would have passed, and compilation obviously could not see it.
Fixed by wiring all four into configureEnvironment's diff, alongside their peers,
and by reading them back in the from-handle path. Two details read out of
src/env/env_backup.c rather than assumed:
- set_backup_config calls __env_backup_alloc itself, so the setters do NOT
require a BackupHandler to have been installed first and can be applied like
any other setting.
- get_backup_config returns EINVAL when no backup handle exists, and a handle
exists exactly when a BackupHandler was installed -- so the read-back is
conditioned on that rather than swallowing an exception.
Found while testing this, and far worse: the Java binding has been UNLOADABLE
since the CalVer transition. DbConstants.java is a committed generated file and
still carried
DB_VERSION_MAJOR = 5; DB_VERSION_MINOR = 3; DB_VERSION_PATCH = 29;
so db_javaJNI's version handshake threw on every single Environment open:
java.lang.RuntimeException: Berkeley DB library version 2026.0.9
doesn't match Java class library version 5.3.29
Regenerating with dist/s_java_const fixes it. The root cause is that
s_java_const is NOT invoked by dist/s_all, so nothing regenerates this file --
the same shape as build_windows/db.h, which IS covered. That gap is why five
months of CalVer releases shipped a Java binding that could not open an
environment.
Test: test/db/run_u8_backup_config.sh (+ U8Check.java), registered in
test/MANIFEST, SKIPs without a JDK or JNI library. It opens a real environment,
sets values unlike any default, and reads each back THROUGH THE C LAYER via
Environment.getConfig() -- the only check that can distinguish "stored in a Java
field" from "reached the library".
Teeth verified: removing just the read_count push makes the test report
"read_count reached the library (4096, got 0)", which is precisely the pre-fix
behaviour.
DbConstants.java is generated from src/dbinc/db.in and dist/RELEASE by dist/s_java_const, and it is COMMITTED. Nothing regenerated it for five months, so after the CalVer transition it still declared DB_VERSION 5.3.29 while the library reported 2026.0.9 -- and db_javaJNI does a version handshake at class-init, so every Environment open threw and the Java binding was effectively unloadable in every release since v2026.04. src/dbinc_auto/ has had a blocking drift gate for precisely this class of mistake since the ABI work. The Java side did not, which is the whole reason the breakage survived that long: s_java_const IS reachable from s_all (via s_java), but s_all is not run in CI, so 'regenerable' and 'actually regenerated' were different things and only one of them was checked. The new step regenerates and diffs, blocking on any difference, with an error message naming the command to re-run.
G12/G13 have never measured anything because the measuring job needs a
self-hosted big-box runner that does not exist. Ran it once manually on EC2.
Dedicated c6id.16xlarge, 64 vCPU Xeon 8375C, THP off, 5 alternating reps per
arm, 20s each, TPROC-C:
t=8 median 2,090,294 tpm cv 2.93% n=5
t=32 median 485,962 tpm cv 0.78% n=5
delta -76.8% tolerance 8.8% (3x the t=8 cv, floored at 5%)
VERDICT scale-shape FAIL ... failing_steps=t32:-76.8%
The coefficients of variation are the important part: 2.93% and 0.78% over five
alternating reps means the -76.8% step is nowhere near the noise, so this is a
reproducible negative-scaling defect rather than a measurement artefact. The
gate's own --self-test passes in BOTH directions on the same machine, so the
failure is a statement about the engine and not about the harness.
Two consequences worth being explicit about:
- G13 cannot become a blocking gate until the defect is fixed. The honest reason
performance is not gated is not 'no runner available' -- it is that the gate
would fail immediately and legitimately.
- The shape is the one P1 and P4 already attacked (peak near t=2-8, long
decline). The remaining measured lever is P5's batching, design D0, which RFC
0008 puts at +189% at t=32 for 4 rows/txn.
Raw data committed as test/bench/G13-SCALE-SHAPE-2026-10.tsv so the next run has
a comparable baseline.
…w, breaks at 4
You asked me to validate D0 before accepting it. Building it means a new log
record type, a recovery function, log_verify work and a DB_LOGVERSION bump, so
the premise is worth testing first with existing knobs. test/bench/d0_probe.c
does that on a dedicated 64 vCPU c6id.16xlarge.
Two of D0's claims hold:
records-per-row is 3.093 at 1 row/txn, measured independently, matching RFC
0008's 3.10.
batching raises throughput as predicted -- 72,341 -> 97,812 -> 107,625 rows/s
at 1/4/8 rows per txn, as records/row falls 3.09 -> 2.34 -> 2.22, i.e. +49%.
One does not. D0's premise, inherited from RFC 0008's Finding 3, is that latch
ACQUISITIONS dominate and the bytes moved inside the critical section are noise.
A control arm that stores an EMPTY data item -- same record count (2.27 vs 2.34),
fewer bytes -- gives +86.8% at 4 rows/txn:
4 rows/txn, 100-byte data 97.8k rows/s (3 reps: 98455 97400 97472)
4 rows/txn, empty data 182.7k rows/s (3 reps: 181185 181332 185533)
That is LARGER than the +35% batching itself buys at the same point. At 1 row per
transaction the identical control is only +5.9%, which is exactly where RFC
0008's handoff-cost finding was measured -- so the original finding was not
wrong, it was measured at the one batch size where bytes genuinely are noise.
Conclusion: bytes under the latch become dominant precisely at the batch sizes
D0's own projection depends on, so halving the record count will not deliver the
projected gain by itself. This is a design constraint rather than a refutation:
the win has to come from one record carrying both items BY REFERENCE to a single
page image, not from merging two payload copies into one record. A naive
combined record that still copies key and data separately would land much nearer
+35% than +189%.
Harness note: the first multi-row run hung silently. That was my bug, not the
engine's -- 32 threads touching 2+ keys per transaction with NO deadlock detector
configured. set_lk_detect(DB_LOCK_YOUNGEST) fixed it, and aborted transactions
are not counted as progress.
The instruction was to fix the correctness bug and merge perf/bhpin-r1 if we feel the branch should be merged. It should not, and the evidence for that was already on master: PIN-REMEASURE-2026-09.md re-measured all four parked perf branches from behind the cursor-mutex wall that had invalidated their original verdicts, and bhpin-r1 came back as 'REGRESSION as it stands (and its old verdict was vacuous)' -- 1.55x/1.36x slower where its option actually fires. So the latent correctness bug sat on top of a change that is not worth having even when correct. Fixing it would have meant spending effort making a measured regression safe to merge. Deleted perf/bhpin-r1 and the three other parked branches the same re-measurement disposed of: perf/mpool-pin and perf/lock-readpath (both NULL, 0.957-1.033x and 0.964-1.019x across 12 cells against a +/-4.8% noise floor) and perf/rsnap-ml (confirmed regression, 0.063x at private/batch/32). Nothing is lost. The report, the three A/B harness scripts (pinrm_bhpin_ab.sh, pinrm_bhpin_fires.sh, pinrm_bhpin_why.sh) and every raw profile under test/bench/results/pin-remeasure-2026-09/ are on master, which is where a negative result belongs -- in the tree with its evidence, not in a branch nobody will revisit.
T2 -- ASan "attempting free on address which was not malloc()-ed" at
test_lock_matrix.c:423. Not a stray pointer, an API-ownership mistake: lock_vec
allocates objlist->data with __os_malloc, and __os_malloc prepends a
db_allocinfo_t header and returns an OFFSET pointer (optionally via
DB_GLOBAL(j_malloc)), so the address a caller sees was never returned by libc
malloc. The engine's own callers use __os_free (txn.c:1015, :1540), which is
internal and unreachable from a public-API test program. The harness now
documents the ownership and does not free. After: 0 ASan errors, rc=0.
T6 -- test/lockmatrix/run.sh had CC=${CC:-clang} with no fallback, so the tier
could not run at all where clang is absent. Now prefers $CC, then clang, gcc,
cc, and VERIFIES the choice by compiling with -fsanitize=address rather than by
checking the binary exists -- a gcc without libasan passes `command -v' and then
fails at link. With nothing suitable it SKIPs with a reason. All three paths
tested: clang when present, gcc when clang is shimmed to fail, SKIP when all
fail.
U2 -- s_chk_message_id resolved MSG_DIR relative to the CALLER's cwd, so run
from dist/ every path missed, grep matched nothing, and it exited 0 having
checked no files. That is the shape that let duplicate message ids reach a
release while a checker for exactly that existed. Both MSG_DIR and gen_msg.awk
are now resolved against the script's own location, and a missing directory is a
hard error. Verified identical output (28 [EXPECTED] lines) from four different
working directories.
U4 -- s_tags probed ctags capabilities against ../../src/db/db.c, a path that
has never existed in this repository. Every probe failed silently, so flags
stayed empty and ctags always ran without -d -t -w. Probes src/db/db_iface.c
now, and warns if that is missing too.
U5 -- s_crypto ran ed(1) against docs/index.html to flip a "this release DOES
include cryptography" sentence; this fork has no such page. Dormant is not
correct: it is an export-restriction tool, so a silent failure on the day it
runs for real would ship a tarball whose docs still claim crypto is present. Now
conditional, and says what to do if such a statement is added to docs_src/.
Contributor
Coccinelle convention checksNo new violations. ✅ Resolved since baseline (3) -- update dist/cocci/baseline.txt to lock these in. |
Contributor
ABI diff vs
|
Two corrections to my own first attempt at T2. First, the diagnosis was incomplete. __os_malloc only prepends its db_allocinfo_t header and returns an offset pointer under #ifdef DIAGNOSTIC (src/os/os_alloc.c:268-271). So libc free() on lock_vec's objlist->data is invalid in a diagnostic build and perfectly valid in a production one -- which is why I saw the bad free against my own --enable-diagnostic ASan library while the tier's gate build (--enable-debug, DIAGNOSTIC unset) had never reported it. The tracker row said 'ASan reports a bad free' without that condition, and the condition is the whole reason it was hard to place. Second, my fix was wrong. I removed the free() and wrote a comment arguing that leaking a few DBT-sized blocks in a short-lived harness was the right trade. LeakSanitizer disagreed and failed the tier in CI -- correctly, because this tier runs ASan with leak detection on, so 'do not free' is not available as an option. The comment sounded reasonable and was checkable in about a minute; I should have run the tier runner rather than only the bare binary. The actual fix is __os_free, the deallocator that matches the allocator, reached by including db_int.h -- exactly what test/c/test_log_verify.c already does for the same reason -- plus two more -I entries in run.sh so db_int.h's own include of dbinc/queue.h resolves. Verified in BOTH builds this time: run.sh rc=0 with '0 check(s) failed' on the gate build, and 0 ASan/LSan errors against a --enable-diagnostic ASan library.
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
1 participant
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.
Everything you asked for, except C# — which turned out to need a decision rather than code (below).
U8 — the four backup accessors were write-only
Added, as asked. But they weren't missing —
setBackupReadCountand its three siblings existed and stored to a private field that nothing ever read. Calling them had no effect. The getters returned the Java field, so an accessor round-trip test would have passed.DB_HEAP_FULLsatisfiesJper your call, sopubdef.inis unchanged there.And a much worse find: the Java binding has been unloadable since the CalVer transition.
DbConstants.javais a committed generated file still declaringDB_VERSION 5.3.29, anddb_javaJNIdoes a version handshake at class-init, so everyEnvironmentopen threw:Regenerating fixes it. Added a blocking drift gate —
src/dbinc_auto/has had one for this exact class of mistake; the Java side did not, which is why five months of releases shipped a binding that could not open an environment.U9 — C# needs your decision, not my code
lang/csharpdoes not exist. We deleted it ina77b2119d, whose message says "C# is dropped as a supported binding" — 2,634 files, ~154k lines. You said C# is still supported, which directly contradicts that commit, so I stopped rather than guess. See below.G13 — measured once on 64 cores, and the gate fails
−76.8% against an 8.8% tolerance. Those cv figures are the point: this is a reproducible negative-scaling defect, not noise. The gate's own
--self-testpasses both directions on the same box.So the honest reason perf isn't gated is not "no runner" — it's that the gate would fail immediately and correctly.
P5/D0 — validated, and qualified
Two claims hold: records/row is 3.093 (matching RFC 0008's 3.10), and batching gives +49% (72k→108k rows/s at 1→8 rows/txn).
One does not. A control arm with an empty data item — same record count, fewer bytes — gives +86.8% at 4 rows/txn, larger than the +35% batching itself buys there. At 1 row/txn the same control is only +5.9%, which is where RFC 0008's handoff finding was measured.
Bytes under the latch dominate exactly at the batch sizes D0's projection relies on. Not a refutation — a design constraint: the win must come from one record referencing a single page image, not two payload copies merged.
B1 — resolved by deletion
You said fix and merge if we feel it should be merged. It shouldn't:
PIN-REMEASURE-2026-09.mdalready measured it as a regression (1.55×/1.36× slower where it fires) with a vacuous original verdict. Fixing the correctness bug would be making a measured regression safe. Deleted it and the three other parked negative-result branches; all reports, harnesses and raw profiles stay on master.The mechanical five
T2
__os_mallocreturns an offset pointer, so the harness'sfree()was invalid (engine callers use__os_free, unreachable from a public-API test). T6 compiler detection now verifies by compiling with-fsanitize=address. U2 paths resolved against the script, verified identical output from four cwds. U4/U5 dead paths fixed — U5 matters because it's an export-restriction tool that would fail silently on the day it runs.