Repository navigation
feat(qwp): add error positions, timings, row counts and paging to queries - #81
Open
glasstiger wants to merge 20 commits into
Open
glasstiger wants to merge 20 commits into
glasstiger wants to merge 20 commits into
Conversation
QwpEgressQuery.cancel() and grantCredit(), and the same methods of the
queryViews() handles, return promises. Once the session had handled a
query's terminal response, they threw QwpProtocolError ("QWP response
references inactive request ID") synchronously instead, before any
promise existed, so a .catch() on the result never ran. A cancel() from
an AbortSignal listener or a timer that fired after the query completed
became an uncaught exception, which ends a Node process. A session that
had failed or was closing threw its failure the same way. Node and
browsers were both affected: the fault is in the shared egress session.
- cancel() on a query that has ended resolves without sending anything,
even if its session has since failed or closed.
- grantCredit() on a query that has ended resolves without sending
anything: it is owed no credit. The amount is checked first, whatever
the query's state, and an invalid one rejects with a RangeError.
- The session's cancel(), abandon(), grantCredit() and rejectView()
never throw. A request that has left the session is a no-op, and a
failed or closed session rejects with its failure. requireActive()
and its QwpProtocolError now serve only server messages, where an
unknown request ID is a protocol fault.
Callers will notice that these methods no longer throw: each returns a
promise that resolves or rejects. grantCredit() also stops sending
CREDIT for a query whose consumer has retired while it still drains.
The methods return promises and none of the old behaviour was
documented, so this restores their contract: it is a bug fix, not a
breaking change.
Document the behaviour in the TSDoc and QWP.md, and regenerate the API
reference as CONTRIBUTING.md requires. Most generated-file changes are
TypeDoc source-link, line-number and reflection-ID refreshes.
Fixes #76
CONTRIBUTING.md counted any change to the behaviour of a published API, its wire bytes or the errors it throws as breaking, "even when the new behaviour is a fix". Ordinary bug fixes were marked breaking under it, #78 among them, and each one called for a major release: data that fits within max_buf_size no longer refused, or a promise-returning method that threw synchronously and now rejects. Define a breaking change by the API's contract instead -- what code that relies on its documentation and types has to change -- in a Breaking Changes section that the release and commit guidance both point to. A fix that brings the implementation back to its contract is a plain fix:, and its commit and pull request describe what callers will notice. The wording of an error message is not part of the contract.
…query alone The test for control calls naming a request that has left the session made them only once no query was active, so this.active was undefined whether or not controlledQuery() compared request IDs. Dropping that comparison passed every QWP test, although session.cancel(oldId) then cancelled the query running now and session.abandon(oldId) retired it. Start the next query before those calls, and check that it is still running and that its QUERY_REQUEST is the only frame sent.
…ol calls Mutating the late-control-call fix found regressions that every QWP test still passed: - The session's control calls for a request that has left it ran only while the next query was running, so controlledQuery() could throw, or read requestId off an undefined active query, on an idle session. They now also run on an idle session, and include rejectView(). - No test gave grantCredit() a bad amount on a running query, so the check could be confined to ended queries, left to the CREDIT encoder, or relaxed to accept numeric strings. A new test checks that each bad amount rejects with RangeError, never throws synchronously, sends no frame and leaves the query running. - rejectView() on a closed or failed session, and the queryViews() controls once their session has closed, went untested.
A browser's WebSocket.close() accepts only 1000 and 3000-4999 and throws InvalidAccessError for anything else. The client closes with 1001 in QwpClient.close(), and with 1002 or 1011 on protocol errors, failed sends, failed reconnects and a cancelled query that does not drain. In a browser that close threw, the fallback found no terminate() to call, and the connection settled as closed with a synthetic 1006 while the native socket stayed open. Pooled query sockets outlived QwpClient.close(), and a reconnect: false session whose cancel did not drain kept receiving the cancelled result until the page unloaded. closeSocket() now retries a close that a socket without terminate() refuses, first with 1000 and the same reason, then with no code at all. A WHATWG socket validates its arguments before it changes state, so the retry is safe. ws accepts these codes and still sends them unchanged. A close that ws refuses is still terminated at once, because ws enters CLOSING before it validates and ignores any later close(). A browser still cannot drop a connection itself. Without flow control QuestDB reads the close only once the query has ended, so in a browser the query runs until then or until the browser abandons the closing handshake, after 60 seconds in Chromium. QWP.md now says so. Fixes #77
The comment said the close codes "still reach a Node server", but the server is QuestDB; `ws` is the Node client. It now says that from Node the codes still reach the server. Comment-only change.
The test that keeps a `ws` socket from being retried closed with 1000. That skips the retry with 1000 which startClosingHandshake() gives a refused code other than 1000, so moving the terminate() check after that retry still passed all of test/qwp. With `ws` that change postpones terminate() by the whole close deadline: `ws` enters CLOSING before it validates, and the retried close() sends nothing. The test now also closes with 1011, and fails on that variant. Test-only change.
…close In a browser, a failed ingress negotiation waited for its close(1002) before rethrowing. Since the refused 1002 is retried with 1000 and waits for the server's reply, the connect deadline could fire first and turn the terminal QwpDurableAckUnavailableError into a retryable upgrade timeout. Start the close without waiting for it, as Node's upgrade-validation failures do.
…ries QuestDB can now report over QWP egress what /exec reports besides the rows (questdb/questdb#8356): where an error points in the SQL text, the query's timings and JIT use, and the size of a result read one page at a time. The server advertises this with a new SERVER_INFO capability, QUERY_DETAILS, and each query opts in. Node.js and browsers both get it: the code is in the shared egress session. - query() and queryViews() take errorPosition, stats, countRows, skipRows and maxRows. QwpEgressQueryError carries position, the UTF-16 offset into the SQL string, and the bare message; completions carry stats (QwpQueryStats), and a result's completion carries rowCount, the size of the whole result whatever the window left out. - On an endpoint without QUERY_DETAILS the reports are dropped, as resetDictionary is without QUERY_FLAGS, so they are safe during a rolling upgrade. A row window is not dropped, because such a server would stream the whole result: the query rejects with the new QwpEgressRowWindowUnsupportedError before anything is sent, and a query that failover would replay on such an endpoint fails with it instead of being replayed. - encodeQwpQueryRequest() takes the new QWP_QUERY_FLAG_* bits and the skipRows and maxRows values two of them carry, and requires each value exactly when its flag is set. decodeQwpEgressMessage() takes the request's flags and reads the trailers they add to RESULT_END, EXEC_DONE and QUERY_ERROR. - The reconnecting egress transport reads only the kind and request ID of each frame. It cannot know the request's flags, so a RESULT_END with trailers looked malformed to it, and the query would have been replayed until the reconnect budget ran out. The session decodes and validates the trailers. A query without the new options sends the same request and reads the same responses as before. Document the options in QWP.md and the READMEs, and regenerate the API reference as CONTRIBUTING.md requires. Most generated-file changes are TypeDoc source-link, line-number and reflection-ID refreshes.
Every changed line of the query-details change already ran under the suite, but six plausible regressions in it still passed. The new assertions each fail one of them: - An error position of zero, the start of the SQL text and what QuestDB reports for an error that names no position, is reported, both by the decoder and on QwpEgressQueryError. Other negative values than -1 carry no position either. - A plain query after a detailed one is sent without flags, and its baseline RESULT_END decodes without trailers. - A row-window query that fails over to an endpoint with QUERY_DETAILS is replayed with the same window, flags and values alike, and the replacement's trailers decode. Only an endpoint without the capability makes it fail instead. - A detailed RESULT_END still queued when the session closes is reported as the closed session, not as a malformed frame that closes the connection again with 1002. - Values of neither numeric type are rejected as row-window values and as query flags, and the request ID keeps its uint64 bound.
The query-details change gave decodeQwpEgressMessage() an optional second parameter, queryFlags. map() and Array.from() pass the element index there, so frames.map(decodeQwpEgressMessage), which works with 5.0.0, read each index as query flags: from index 4 on, a valid RESULT_END or EXEC_DONE failed to decode as truncated, because the decoder looked for statistics and row counts the frame does not carry. TypeScript accepts the call, even under --strict. decodeQwpEgressMessage() keeps the single parameter 5.0.0 published, and still rejects a terminal frame that carries trailers. The new decodeQwpEgressMessageWithQueryFlags(bytes, queryFlags) decodes the responses to a request sent with query-details flags, and the query session uses it. An options object as the second parameter would avoid the misread too, but tsc rejects frames.map() with it, so existing callers would no longer compile. Document the new function in QWP.md, and regenerate the API reference as CONTRIBUTING.md requires. Most generated-file changes are TypeDoc source-link refreshes.
Collaborator
Author
Review: approveNo Critical, Moderate or Minor issues found, and the test gate passes. Scope: this PR's own commits, TestsTen targeted one-line breaks of the production code. Nine fail at least one of the new tests:
The tenth survives: recording Checked at
|
The query details took SERVER_INFO capability 0x08 and query flag 0x02, which per-query timeouts already use in released clients: c-questdb-client 7.2.0 and later, and the Python client 6.0.0 and later, read capability 0x08 as "the server honours a per-query timeout" and then send flag 0x02 followed by timeout_ms. A server with the query details would have read that flag as an error-position request, ignored the timeout and answered with a QUERY_ERROR those clients cannot decode. QUERY_DETAILS moves to capability 0x10, and the flags to 0x04 (ERROR_POSITION), 0x08 (STATS), 0x10 (COUNT_ROWS), 0x20 (SKIP_ROWS) and 0x40 (MAX_ROWS). The server and the Java client move to the same bits. A test pins the allocation: every capability and flag is a bit of its own, and none takes the timeout bits. Another sends a query to a server that advertises per-query timeouts but not the query details, which gets none of the details and refuses a row window. Regenerate the API reference, which shows the constant values, as CONTRIBUTING.md requires. Most generated-file changes are TypeDoc source-link refreshes.
The errorPosition docs said an error that does not point into the SQL text, "such as a cancellation", carries no position. That holds for the cancellation cancel() sends, which QuestDB answers with position -1, but not for one on the server: an error the SQL engine raises without a position of its own reports 0, the start of the text, as /exec does, and a query timeout on the server and CANCEL QUERY are among them. The client passes that 0 on, so where the docs promised no position, an application highlighting error.position marked the start of the SQL. QWP.md and the TSDoc of errorPosition, QwpEgressQueryError.position and QwpQueryErrorMessage.position now say which errors report 0 and which carry no position. Regenerate the API reference as CONTRIBUTING.md requires. Most generated-file changes are TypeDoc source-link refreshes.
The comment said index 4 sets STATS, which held while STATS was 0x04. cd427fc moved it to 0x08, so index 4 is now ERROR_POSITION, which RESULT_END and EXEC_DONE ignore, and the frames a two-parameter decodeQwpEgressMessage() would misread are indexes 8 to 15. Comment-only change.
Collaborator
Author
Review: approveScope: PR head The earlier review comment here was written at
CriticalNone. ModerateNone. TestsThe test gate passes, with no coverage gaps. I broke the production code in four places at
Checked at
|
QWP.md and the TSDoc of skipRows and maxRows said that statements ignore the row window, next to the rule that a query with a window fails on an endpoint without QUERY_DETAILS. The client cannot tell a statement from a query before sending it, so on such an endpoint a statement with a positive skipRows or any maxRows fails with QwpEgressRowWindowUnsupportedError too, and nothing said so. The docs now say that a server with QUERY_DETAILS ignores the window for statements, and that a statement is refused the same way as a query on one without it. QwpEgressRowWindowUnsupportedError says so as well. Regenerate the API reference as CONTRIBUTING.md requires. Most generated-file changes are TypeDoc source-link refreshes.
A terminal response that arrived before the socket closed, but that a full batch pool kept the session from validating, decides whether its query is replayed: settleLostQuery() leaves the query to it while it waits. Since 38d2063, the session's rejection of such a terminal -- one carrying trailers its request did not ask for -- goes to the reconnect already under way, which replays the query. Nothing settled the query again first, so one that no longer needed a server was replayed too: its consumer had retired (iterator.return(), a lease release) or its timeout had expired while the terminal waited. The replacement endpoint ran it only to be sent its CANCEL, the session's next query waited for that drain -- up to cancelDrainTimeoutMs, then one more reconnect -- and a timed-out query reported its timeout one grace period late. Before this branch the transport rejected such a terminal on arrival, and the query was dropped. The session now settles the query again once recoverProtocolFailure() returns, as the reconnect that a rejection found while connected does through onConnectionLost. A query that no longer needs a server leaves the replay; one still running is replayed as before. Only a peer that sends trailers the request did not ask for reaches this path.
countNanos was documented as the time spent counting "the rows a row window left out", which reads as covering the rows skipRows leaves out, as rowCount's "left out" does. QuestDB times only the count of the rows past the end of the window, which it makes when maxRows stops the stream before the result set ends, and reports zero whenever the stream reads every row. A window of skipRows alone therefore always reports zero: the skipped rows are counted as they are skipped, and that time falls within executeNanos. The TSDoc and QWP.md now say so. Regenerate the API reference as CONTRIBUTING.md requires. Most generated-file changes are TypeDoc source-link refreshes.
Collaborator
Author
Review: approveScope: PR head CriticalNone. ModerateNone. Minor
TestsThe test gate passes, with no coverage gaps. The fix commits' tests catch their regressions:
Checked at
|
This branch has not been deployed
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
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.
Summary
Adds the client side of questdb/questdb#8356, which lets a QWP query report what
/execreports besides the rows: where an error points in the SQL text, the query's timings and JIT use, and the size of a result read one page at a time. The server advertises the feature with a newSERVER_INFOcapability,QUERY_DETAILS, and each query opts in. The new bits leave capability0x08and flag0x02to per-query timeouts, which the released C and Python clients already use. A query without the new options sends the same request and reads the same responses as before. The Node.js and browser clients both get it: the code is in the shared egress session.Changes
query()andqueryViews()takeerrorPosition,stats,countRows,skipRowsandmaxRows.QwpEgressQueryError.positionis the UTF-16 offset into the SQL string, which is a JavaScript string index, and the message is then bare, without QuestDB's[n]prefix.stats(QwpQueryStats: compile, execute, count and client-wait nanoseconds, andjitCompiled). A result's completion also carriesrowCount, the size of the whole result whatever the window left out.QUERY_DETAILSthe reports are dropped, asresetDictionaryis withoutQUERY_FLAGS, so they are safe during a rolling upgrade. A row window is not dropped, because such a server would stream every row:QwpEgressRowWindowUnsupportedErrorbefore anything is sent, and so does a statement, which the client cannot tell from a query before sending it;encodeQwpQueryRequest()takes the newQWP_QUERY_FLAG_*bits and theskipRowsandmaxRowsvalues two of them carry, and requires each value exactly when its flag is set. The newdecodeQwpEgressMessageWithQueryFlags(bytes, queryFlags)reads the trailers the flags add toRESULT_END,EXEC_DONEandQUERY_ERROR.decodeQwpEgressMessage(bytes)keeps its single parameter, soframes.map(decodeQwpEgressMessage)still works:map()passes each index as a second argument, which aqueryFlagsparameter would read as flags.RESULT_ENDwith trailers looked malformed to it, and the query would have been replayed until the reconnect budget ran out. The session decodes and validates the trailers.RESULT_ENDwhose totals do not match its batches, or, now that the transport leaves trailers to the session, one with trailers the request did not ask for. Such a rejection used to fail the session withQwpSendClosedError("QWP WebSocket is not open"). It now goes to the reconnect already under way, which replays the query as it does after a protocol error found while connected, and it is charged to the samefailoverMaxAttemptsandfailoverMaxDurationMsbudget. This applies to every query, with or without the new options.QWP.mdand the READMEs, and regenerates the API reference. Most generated-file changes are TypeDoc source-link refreshes.Public API
Additions only:
QwpEgressQueryOptions:errorPosition,stats,countRows,skipRows,maxRows.QwpEgressQueryError.position,QwpResultEndMessage.rowCountand.stats,QwpExecDoneMessage.stats,QwpQueryErrorMessage.position,QwpQueryRequest.skipRowsand.maxRows.QwpEgressRowWindowUnsupportedError,QwpQueryStats,QWP_EGRESS_CAPABILITY.QUERY_DETAILS, andQWP_QUERY_FLAG_ERROR_POSITION,_STATS,_COUNT_ROWS,_SKIP_ROWSand_MAX_ROWS.decodeQwpEgressMessageWithQueryFlags(), which decodes the responses to a request sent with query flags.encodeQwpQueryRequest()now rejectsqueryFlagsthat set the skip or max bit without the matching value. Those bits had no meaning before, and a server that knows them would read the end of the request as the value. It also rejects aqueryFlagsthat is neither a number nor a bigint, such as a string, which 5.0.0 coerced.Validation
New codec, session and failover tests cover byte-level request and trailer encoding and decoding, capability gating, the bits left to per-query timeouts, the unsupported-window error, replays across endpoints with and without
QUERY_DETAILS, trailers passing through the reconnecting transport, and a terminal response rejected after the socket has dropped.Coverage: under V8 coverage, every changed production line and branch runs.
Mutation checks: each of these changes fails at least one test: reverting the transport change or the replay withdrawal, dropping an error position of zero, keeping a detailed query's flags for the next plain query, always withdrawing a row-window replay, clearing the flags before a queued response is decoded, dropping the request-ID range check, giving
decodeQwpEgressMessage()back a second parameter, moving the query details back onto the per-query timeout bits, and, for a rejection that arrives during a reconnect, failing the session as before, skipping the recovery charge, keeping the stale messages or waiting for the reconnect.Passed: 1,123 QWP and docs tests, 245 other unit tests, 39 distribution tests and 12 real-Chromium tests.
Passed: all typechecks, including
typecheck:distwith TypeScript 4.9, ESLint, Prettier andcheck:packages.Passed: an end-to-end run of the built Node.js package, with default options, against a server built from feat(qwp): add error positions, timings, row counts and paging to QWP queries questdb#8356. Error positions and messages match
/exec, including for non-ASCII SQL. Pages and row counts are correct for table scans,ORDER BY ts DESC, filters andmaxRows: 0, and statistics arrive on results and statements.Not run: the Docker-based ILP integration tests, which this change does not touch.
Merge order
This branch is stacked on #80 and #79 (merged in), so their commits appear in this PR until they land on
main.