Skip to content

always increment finishedQueries counter if incrementing allQueries#13786

Open
jkt-signal wants to merge 1 commit into
apple:release-7.3from
jkt-signal:balanced-storage-counters
Open

always increment finishedQueries counter if incrementing allQueries#13786
jkt-signal wants to merge 1 commit into
apple:release-7.3from
jkt-signal:balanced-storage-counters

Conversation

@jkt-signal

Copy link
Copy Markdown

Storage server processes publish a pair of counters total_queries and finished_queries (as well as a derived special counter query_queue_max) tracking how many read operations have been started and finished (and the high-water-mark for the number of pending queries—those that have been started, but not finished—in a status-fetch interval).

Unfortunately, the counter underlying total_queries was incremented at the beginning of one of several actors, and the matching increment to finished_queries was done at the end, with several intervening waits. If the actor was cancelled (or experienced another non-returnable error), finished_queries was not incremented, resulting in a permanent drift between total_queries and all_queries and apparently endless growth in query_queue_max.

The easiest way to demonstrate this behavior is to have two different clients alternate between setting watches and updating values for the same keys—for example, run two simultaneous instances of this simple Java program against a single database, and monitor its status to see query_queue_max on storage servers go up permanently every time you run the test.

To solve this problem, we add a simple class CountedSection to flow/Stats.h to bump two counters in RAII-enforced balance; once a local CountedSection has bumped the first counter, it will always eventually bump the second one whether it is destroyed as a result of normal exit or exception. (Of course, if you store one in a heap object and leak it, you will still experience drift, so, uh, don't do that.) CountedSection is made move-only so you can use it in actor state, since state variables seem to always be default-constructed and then assigned to their initializers even if the initialization comes before the first wait in an actor.

One of the actors involved actually updates a second counter pair, getMappedRangeQueries and finishedGetMappedRangeQueries, in a way vulnerable to the same problem, so we use CountedSection to address that as well, though I have not seen any instances of these getting out of sync (entirely due to lack of trying).

Some other metrics updated at the ends of the affected actors—mostly latency metrics, but also some tagged size distributions—are not addressed by this change. Arguably they should not be, since including latency and response size of interrupted queries may significantly throw off the statistics in a not-necessarily-useful way.

Leaving this as an RFC because I'm not sufficiently familiar with the FoundationDB testing setup to write a good test; the Java program linked above is a great demonstration but is not trivially adapted to an integration test (and presumably we'd like tests of the fdbserver itself to be C++, not dependent on the Java bindings). Advice for how to test the change greatly appreciated!

This PR is against the 7.3 branch, rather than main, because all the relevant actors have been rewritten to use C++20 standard coroutines in the interim, and while I strongly suspect the same bug applies equally in main, I was not able to confirm this because main does not currently build a usable fdb for me. Porting forward to main should be relatively trivial and I'm happy to do so if requested.

@jkt-signal

Copy link
Copy Markdown
Author

(Unfortunately, I don't appear to have the rights to apply the RFC label as suggested by the commit process document, so I'm marking this PR ready for review instead.)

@jkt-signal
jkt-signal marked this pull request as ready for review July 24, 2026 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant