Make sure we `Fetch` query result when ready #1996

Y-- · 2025-05-01T14:24:39Z

In the current code, we call Fetch on a query result before checking if there is a result available, which may cause deadlocks since we don't return to the event loop in the BLOCKED and NO_TASKS_AVAILABLE cases.

This PR solves it by returning a special buffer containing one "magic" byte out of the C++ layer, then a null buffer indicating the caller they need to retry fetching.

We tested this with MotherDuck operators which do return "BLOCKED", and this fixes the streaming issue we had.

Ideally, this would test this with a custom "test" extension that reproduces the behavior.

Another way to test it (albeit less ideal), would be to run DuckDB Wasm tests with a special version of DuckDB compiled with FORCE_ASYNC_SINK_SOURCE flag.

carlopi

This is properly hacky, love it.

Left some comments, for you or for me in a bit.

I have a setup to run most duckdb unittester in duckdb-wasm (currently only node) vs various duckdb-wasm interfaces, I need to upstream that, and that can be used to stress test this more.

carlopi · 2025-05-05T09:12:27Z

packages/duckdb-wasm/src/bindings/bindings_base.ts

        }
+
        const res = copyBuffer(this.mod, d, n);
        dropResponseBuffers(this.mod);


Suggested change

dropResponseBuffers(this.mod);

dropResponseBuffers(this.mod);

I am not sure if this needs to be performed, or moved afterwards.

Yeah it may need to be revisited though might be worth a dedicated PR?

I was thinking moving this below the return null might be it, but I need to check.
In any case, this impact only retry cases, that are currently not exposed in the wild, so might be ok-ish.

I think we want it in both "SUCCESS" cases since a buffer is instantiated, otherwise we would leak the (1-byte) retry buffer each time no?

packages/duckdb-wasm/src/parallel/async_connection.ts

carlopi · 2025-05-05T09:16:17Z

packages/duckdb-wasm/src/bindings/bindings_base.ts

+        if (res.length === 1 && res[0] == 84) {
+            return null;
+        }


Alternative would be returning an arrow error with custom error code, unsure if that is properly supported, let me check.

Yeah that's the other option we considered, and I agree it should work just as well. We felt this way was the most straightforward and explicit compared to abuse a specific Arrow error code.

carlopi · 2025-05-05T09:19:20Z

Actually not properly approved, left some comments that I need to double check.

carlopi · 2025-05-06T07:53:24Z

@Y--: I think the best path forward is wrapping Uint8Array in an object that can then hold also additional informations about what kind of buffer is returned and allows more flexibility, this is still needed going forward.

At that point instead of the magic byte a duckdb-wasm level error can be cleanly returned with a payload.

carlopi · 2025-05-06T07:55:54Z

@Y--: I think the best path forward is wrapping Uint8Array in an object that can then hold also additional informations about what kind of buffer is returned and allows more flexibility, this is still needed going forward.

At that point instead of the magic byte a duckdb-wasm level error can be cleanly returned with a payload.

I will set this up and ping back.

Y-- · 2025-05-06T07:57:44Z

@Y--: I think the best path forward is wrapping Uint8Array in an object that can then hold also additional informations about what kind of buffer is returned and allows more flexibility, this is still needed going forward.

At that point instead of the magic byte a duckdb-wasm level error can be cleanly returned with a payload.

I can try to work on that ~ my main concern is to do this preserving the 0-copy transfer as much as possible, but hopefully the Arrow API can help here.

Y-- · 2025-05-06T08:01:22Z

@Y--: I think the best path forward is wrapping Uint8Array in an object that can then hold also additional informations about what kind of buffer is returned and allows more flexibility, this is still needed going forward.
At that point instead of the magic byte a duckdb-wasm level error can be cleanly returned with a payload.

I can try to work on that ~ my main concern is to do this preserving the 0-copy transfer as much as possible, but hopefully the Arrow API can help here.

Sadly it looks like ConcatenateBuffers does copy its input.

But we can use SerializeRecordBatch with a buffer that we manage ourselves instead. That looks reasonably easy, I'll give it a shot later today.

Y-- · 2025-05-06T08:26:04Z

@Y--: I think the best path forward is wrapping Uint8Array in an object that can then hold also additional informations about what kind of buffer is returned and allows more flexibility, this is still needed going forward.
At that point instead of the magic byte a duckdb-wasm level error can be cleanly returned with a payload.

I can try to work on that ~ my main concern is to do this preserving the 0-copy transfer as much as possible, but hopefully the Arrow API can help here.

Sadly it looks like ConcatenateBuffers does copy its input.

But we can use SerializeRecordBatch with a buffer that we manage ourselves instead. That looks reasonably easy, I'll give it a shot later today.

Implemented here - 8bfe08b (#1996)

carlopi · 2025-05-06T08:55:14Z

I ended up sketching an alternative here: main...carlopi:duckdb-wasm:refs/heads/wrap_fetch, what do you think? I might clean up and PR that, then you can add your on top

carlopi · 2025-05-07T11:03:10Z

Hi, I ended up with #2000, after which it should be somewhat trivial to add this on top, it should be a matter of adding two enums, one at lib/include/duckdb/web/webdb.h on the C++ side and one at packages/duckdb-wasm/src/status.ts in the typescript layer.

Plus adding the relevant handling in fetchQueryResults such as return null;

Would you mind taking a look, feedback is welcome

Y-- · 2025-05-07T11:44:56Z

Hi, I ended up with #2000, after which it should be somewhat trivial to add this on top, it should be a matter of adding two enums, one at lib/include/duckdb/web/webdb.h on the C++ side and one at packages/duckdb-wasm/src/status.ts in the typescript layer.

Plus adding the relevant handling in fetchQueryResults such as return null;

Would you mind taking a look, feedback is welcome

Had a quick look and it look very reasonable. I would need to look in more detail to make sure we don't copy the buffer un-necessarily, but I assume you already made sure of it?

carlopi · 2025-05-07T11:49:02Z

I would need to look in more detail to make sure we don't copy the buffer un-necessarily, but I assume you already made sure of it?

I will check this myself and verify this is the case. I would be inclined to say so, but the question is reasonable and better to get another look.

Y-- · 2025-05-09T14:07:57Z

Thanks for landing #2000 @carlopi ! I've rebased this PR on it, let me know if it needs more help :-)

carlopi

Thanks!

Y-- force-pushed the yl/ensure-fetch-ready branch 2 times, most recently from ef2f016 to 8cff0fa Compare May 5, 2025 08:49

carlopi approved these changes May 5, 2025

View reviewed changes

carlopi self-requested a review May 5, 2025 09:18

Y-- force-pushed the yl/ensure-fetch-ready branch from dbb95ca to 8bfe08b Compare May 6, 2025 08:24

carlopi mentioned this pull request May 7, 2025

Wrap results of fetchQueryResults via DuckDBWasmResultsWrapper #2000

Merged

Y-- force-pushed the yl/ensure-fetch-ready branch from 8bfe08b to e82805f Compare May 9, 2025 12:22

Y-- added 2 commits May 9, 2025 15:39

Make sure we Fetch query result when ready

a1f8001

Use DuckDBWasmResultsWrapper

a3462d0

Y-- force-pushed the yl/ensure-fetch-ready branch from e82805f to a3462d0 Compare May 9, 2025 14:04

carlopi approved these changes May 10, 2025

View reviewed changes

carlopi merged commit d9e0a4f into duckdb:main May 10, 2025
15 checks passed

	dropResponseBuffers(this.mod);
	dropResponseBuffers(this.mod);

Make sure we Fetch query result when ready #1996

Make sure we Fetch query result when ready #1996

Uh oh!

Conversation

Y-- commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlopi left a comment

Choose a reason for hiding this comment

Uh oh!

carlopi May 5, 2025

Choose a reason for hiding this comment

Uh oh!

Y-- May 5, 2025

Choose a reason for hiding this comment

Uh oh!

carlopi May 5, 2025

Choose a reason for hiding this comment

Uh oh!

Y-- May 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carlopi May 5, 2025

Choose a reason for hiding this comment

Uh oh!

Y-- May 5, 2025

Choose a reason for hiding this comment

Uh oh!

carlopi commented May 5, 2025

Uh oh!

carlopi commented May 6, 2025

Uh oh!

carlopi commented May 6, 2025

Uh oh!

Y-- commented May 6, 2025

Uh oh!

Y-- commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Y-- commented May 6, 2025

Uh oh!

carlopi commented May 6, 2025

Uh oh!

carlopi commented May 7, 2025

Uh oh!

Y-- commented May 7, 2025

Uh oh!

carlopi commented May 7, 2025

Uh oh!

Y-- commented May 9, 2025

Uh oh!

carlopi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Make sure we `Fetch` query result when ready #1996

Make sure we `Fetch` query result when ready #1996

Y-- commented May 1, 2025 •

edited

Loading

Y-- commented May 6, 2025 •

edited

Loading