Skip to content

fix(qwp): fix QWP query clients failing or reporting an invalid row count on TRUNCATE, SET and similar statements, and QWP senders timing out on close() - #7770

Open
bluestreak01 wants to merge 4 commits into
masterfrom
fix/qwp-exec-done-negative-rows-affected
Open

bluestreak01 wants to merge 4 commits into
masterfrom
fix/qwp-exec-done-negative-rows-affected

Conversation

@bluestreak01

@bluestreak01 bluestreak01 commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Tandem: questdb/java-questdb-client#107, questdb/questdb-enterprise#1265

This PR bumps the java-questdb-client submodule to the client PR's branch, for the second fix below. The Enterprise companion bumps the questdb submodule to this branch and adds a test for the access-control statements. Merge order: the client PR first, then re-point java-questdb-client here at its squash commit and merge this PR, then re-point the Enterprise submodule at this PR's squash commit.

Problem

QWP egress answers a non-SELECT statement with an EXEC_DONE frame that carries the statement's op_type and rows_affected. For every statement the compiler executes at parse time, the server put -1 into rows_affected. That covers TRUNCATE, RENAME TABLE, SET / RESET, BEGIN / COMMIT / ROLLBACK, DEALLOCATE, VACUUM, CHECKPOINT CREATE / RELEASE, REFRESH MATERIALIZED VIEW, ALTER TABLE ... SUSPEND WAL / RESUME WAL, and in Enterprise the access-control statements (CREATE USER, GRANT, REVOKE, ...). CREATE, DROP, ALTER, INSERT and UPDATE were not affected.

rows_affected is an unsigned LEB128 varint, so -1 went out as ten bytes (FF x9, 01). The clients handled it as follows:

Client Decodes rows_affected as Result for these statements
Rust, C, C++ u64 18446744073709551615
Node.js unsigned bigint 18446744073709551615n
Java, .NET signed 64-bit -1
Go int64, rejecting values above 2^63 - 1 Exec fails with "varint overflow" after the server has applied the statement, and the client latches the connection as broken

The QWP egress protocol spec and the documentation of every client state 0 for statements without a row count.

Cause

The catch-all branch of QwpEgressUpgradeProcessor.executeNonSelect() read CompiledQuery.getAffectedRowsCount(). CompiledQueryImpl only ever assigned -1 to the backing field, and nothing else called the getter.

Change

  • executeNonSelect() leaves rows_affected at 0 for these statements, as its CREATE / DROP / ALTER branches already did.
  • The PR removes CompiledQuery.getAffectedRowsCount() and its field. They only ever produced -1, and egress was their only caller. Enterprise does not reference them.
  • The QwpEgressFrameWriter.writeExecDone() Javadoc states that rows_affected must not be negative.

Compatibility and limitations

  • The wire format and the protocol version stay the same, and no client needs a change: the server now matches the documented behaviour.
  • Java and .NET applications now see 0 instead of -1 for these statements. Code that treated -1 as "not applicable" observes the change; op_type remains the documented way to tell DDL from DML.
  • Servers released before this fix keep sending -1. Clients connected to them keep seeing the old values, and the Go client keeps failing these statements against them unless it learns to tolerate the old encoding.
  • The writer neither asserts nor clamps rows_affected. QuestDB runs with -ea in production, and one path can still produce a negative count: OperationFutureImpl reads an asynchronously completed UPDATE's 64-bit row count with Unsafe.getInt(), so 2^31 rows or more come back truncated, on every protocol. An assertion would turn that wrong count into a QUERY_ERROR after the write was applied. This PR does not change that path.

Test plan

  • QwpEgressDdlExecTest: executeDdl() now asserts 0 rows affected for every DDL the class runs, and testParseTimeExecutedStatements runs 13 parse-time statements and checks the op type and 0 rows affected for each. On Windows it skips CHECKPOINT CREATE, which the server rejects there ("Checkpoint is not supported on Windows"); CHECKPOINT RELEASE needs no prior checkpoint and still runs on every platform. Without the fix, testRenameTable and testParseTimeExecutedStatements fail with expected:<0> but was:<-1>.
  • Two INSERTs that went through executeDdl() now go through executeExec() and assert their row counts.
  • All 33 QwpEgress*Test classes (365 tests) and CompiledQueryTypeCodeTest pass locally. testParseTimeExecutedStatements passed five runs with different fragmentation seeds.
  • Enterprise compiles against this change and its 115 QWP tests pass. An Enterprise test (questdb/questdb-enterprise#1265) that covers CREATE / ALTER / DROP of users, groups and service accounts, GRANT, REVOKE and ADD / REMOVE USER fails with -1 without this fix and passes with it.

Second fix: QWP senders timing out in close() and drain()

Problem

This branch's linux-other CI leg failed once with a 300 s close() drain timeout in QwpSenderE2ETest.testConcurrentSenders_sameTable_doubleToDecimal (build 276712). The egress change above does not cause it: the Java QWP sender could discard the server's ACK of a frame it had just sent. When that frame was the last one, close() and drain() waited for their full timeout and then reported unacknowledged data, although the server had committed every row. The server's DEBUG log from that run shows it sent the ACK 64 µs after receiving the frame.

Cause

The sender's SegmentRing.appendOrFsn() made a frame's bytes visible to its I/O thread before it published the frame's FSN. The I/O thread sends a frame as soon as its bytes are visible, and SegmentRing.acknowledge() clamps every ACK at the published FSN, so an ACK that arrived between the two stores was capped one frame short and dropped. Server ACKs are cumulative and only follow new frames, so after the final frame nothing re-delivered it. The window spans a few instructions and needs the producer thread descheduled inside it: the hang appeared once in 305 PR builds and 51 macwin builds since 2026-09-07.

Change

Test plan

  • The client PR adds two SegmentRingTest tests that fail on the previous client and pass with the fix, and the full client suite passes (3,477 tests).
  • Against the fixed client, all 1,555 tests in cutlass/qwp pass, including QwpSenderE2ETest (137).
  • With a 20 ms producer pause injected at the old race point, 16 of 16 senders in a QwpSenderE2ETest-style run hung on the previous client. With the same pause at either point of the new order, 0 of 48 hung and every row arrived.

QWP egress replied to every statement the compiler executes at parse
time with rows_affected = -1 in EXEC_DONE: TRUNCATE, RENAME TABLE,
SET, BEGIN / COMMIT / ROLLBACK, VACUUM, CHECKPOINT, REFRESH
MATERIALIZED VIEW, WAL SUSPEND / RESUME, and in Enterprise the access
control statements. The catch-all branch of executeNonSelect() read
CompiledQuery.getAffectedRowsCount(), which CompiledQueryImpl only
ever set to -1.

rows_affected is an unsigned LEB128 varint, so -1 went out as ten
bytes. Clients that decode it as u64 (Rust, C/C++, Node.js) read
18446744073709551615, Java and .NET read -1, and the Go client
rejected the frame as a varint overflow: Exec() failed with a
transport error after the server had applied the statement, and the
client latched the connection as broken. The protocol spec and the
client docs state 0 for statements without a row count.

executeNonSelect() now leaves rows_affected at 0 for these statements,
as its CREATE / DROP / ALTER branches already did. The commit removes
CompiledQuery.getAffectedRowsCount(), whose only caller was this
branch, and makes QwpEgressFrameWriter.writeExecDone() assert that
rows_affected is not negative.

QwpEgressDdlExecTest now asserts 0 rows affected for every DDL it
runs and covers the parse-time statement families explicitly.
QuestDB runs with -ea in production (questdb.sh, docker-entrypoint.sh),
so the assertion was not test-only. When it fired, handleQueryRequest()'s
catch-all turned the AssertionError into a QUERY_ERROR after the
statement had been applied. One path can still produce a negative
count: OperationFutureImpl reads an asynchronously completed UPDATE's
64-bit row count with Unsafe.getInt(), so 2^31 rows or more come back
truncated. The assertion would have turned that wrong count into an
error for a committed write.

The writeExecDone() Javadoc still states that rows_affected must not
be negative, and QwpEgressDdlExecTest checks the values clients see.
@bluestreak01 bluestreak01 added Bug Incorrect or unexpected behavior QWP labels Oct 7, 2026
@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository: questdb/questdb/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0eaebc6f-add8-468b-b14d-ee0a7f240f39

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

The server rejects CHECKPOINT CREATE on Windows, which lacks the sync()
system call it relies on, so testParseTimeExecutedStatements failed there
with a QUERY_ERROR. Guard only CREATE: CHECKPOINT RELEASE needs no prior
CREATE and still runs on every platform.
Points the submodule at java-questdb-client 50d12216
(questdb/java-questdb-client#107). The QWP sender's segment ring made
a frame's bytes visible to its I/O thread before it published the
frame's FSN, so an ACK the server sent inside that window was clamped
one frame short and dropped. After the final frame nothing
re-delivered it, and close() and drain() waited out their full
timeout although the server had committed every row. This branch's
linux-other CI leg hit it as a 300 s close() drain timeout in
QwpSenderE2ETest.testConcurrentSenders_sameTable_doubleToDecimal.

The client now publishes the FSN before the frame becomes visible.
Once the client PR merges, this pointer moves to its squashed commit
on main.
@bluestreak01 bluestreak01 changed the title fix(qwp): fix QWP query clients failing or reporting an invalid row count on TRUNCATE, SET and similar statements fix(qwp): fix QWP query clients failing or reporting an invalid row count on TRUNCATE, SET and similar statements, and QWP senders timing out on close() Oct 7, 2026
@bluestreak01

Copy link
Copy Markdown
Member Author

I found no blocking issues in any of the three PRs, and nothing at Moderate or Minor either. ENT, OSS and client are all approve.

Reviewed PR #1265 at level 3 in tandem mode, with #7770 and the nested questdb/java-questdb-client#107. Revisions: ENT 6abd2381 (base d37022c7), OSS a5c0d3a4 (merge-base 6c635564), client 50d12216 (base a7e7db3d). CI on all three PRs was still pending when I reviewed them.

Submodule scope

PR titles and descriptions: titles follow Conventional Commits with user-facing descriptions, and the labels match. Nothing to fix.

Findings

None at Critical, Moderate or Minor.

Test coverage

The test gate passes with no coverage gaps. Runs at the reviewed revisions:

  • Client SegmentRingTest:
    • The two new tests and testAcknowledgeClampsAtPublishedFsn pass at head.
    • With SegmentRing.java and MmapSegment.java reverted to a7e7db3d in a scratch worktree, both new tests fail (expected:<1228> but was:<1227>; ACK of visible FSN 49 was clamped to 48). The concurrent test failed 10 of 10 runs.
    • With only the two stores swapped back, keeping the wakeup after both, the single-threaded test passes. The concurrent test still failed 10 of 10 runs on a multi-core host, so it does guard the store order.
  • OSS and ENT tests: QwpEgressDdlExecTest (10 of 10), QwpEgressAclExecDoneTest (1 of 1) and ColdStorageTestUtilsTest (3 of 3) pass at head, built with -P local-client.
  • Not run against base: the OSS and ENT egress tests. The failure without the fix is clear from source: base CompiledQueryImpl only ever assigns affectedRowsCount = -1, and executeDdl and assertExecDone both assert 0L.

Summary

  • Verdict: ENT approve, OSS approve, client approve.
  • Severity count: 0 Critical, 0 Moderate, 0 Minor; 0 in the diff, 0 breaking unchanged callers.
  • Callers checked: every caller of the code each PR changes.
    • OSS and ENT: the removed CompiledQuery.getAffectedRowsCount() has no remaining callers or implementors.
    • Client: every reader of publishedFsn, publishedOffset() and frameCount, and every acknowledge caller.
    • ENT cold storage: all users of listBucketRelative() and bucketHasDataParquet().

Behaviour changes to be aware of (deliberate, not defects):

  1. Java and .NET clients now see 0 instead of -1 for statements the compiler runs at parse time (TRUNCATE, SET, the ACL statements and similar). The OSS PR documents this, and it matches the client javadoc ("0 for pure DDL"). Nothing in OSS or ENT main code, or in the pinned client, branches on -1.
  2. publishedFsn can briefly run one frame ahead of the bytes the I/O thread can see. Nothing relies on the old order:
    • The send path and reconnect positioning use publishedOffset() and frameCount, whose relative order is unchanged.
    • The I/O thread reads publishedFsn only to bound error-report ranges. The other readers run on the producer thread or with no concurrent producer.
    • ACKs are capped at the last sent frame (nextWireSeq - 1) before reaching the ring, so a real ACK can no longer be clamped away.
  3. Store-and-forward rules: the client change adds no error surfacing, reconnect budget or hard failure. It removes a close()/drain() timeout caused by a dropped ACK.

The scratch worktree has been removed, and all three checkouts are clean at the reviewed SHAs.

@bluestreak01 bluestreak01 added the QUEUED FOR MERGE Approved PR in the merge queue. Do not merge master into this PR. label Oct 7, 2026
@ideoma

ideoma commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

[PR Coverage check]

😍 pass : 0 / 0 (0%)

@sklarsa

sklarsa commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

⚠️ Enterprise CI Failed

The light enterprise test suite failed for this PR.

Build: View Details
Tested Commit: a5c0d3a4757901245c5d801d048453d665300529

Please investigate the failure before merging.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Incorrect or unexpected behavior QUEUED FOR MERGE Approved PR in the merge queue. Do not merge master into this PR. QWP tandem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants