Materialize DuckDB execution results prior to returning tuples to Postgres

[YuweiXiao]

Currently, DuckDB may execute in parallel with the Postgres main thread, meaning DuckDB result chunks are returned and converted to Postgres in a stream. This approach works well for SELECT queries.

However, when experimenting with WRITE in pg_duckdb (e.g., CTAS and #688), I found there were race conditions on Postgres resources like the shared buffer, leading to backend crashes. These races occur bwteen the backend main thread and DuckDB's execution threads. For instance, when the main thread writes to a heap table while DuckDB threads execute a SELECT query (e.g., reading the heap table).

Supporting WRITE would significantly enahce pg_duckdb's usecase, and it already allows CTAS to create new tables with DuckDB-generated data.

Potential solutions to address the issue:

1. Single-Threaded Execution for WRITE operations
   Limit DuckDB to one thread during WRITE, restricting execution to the main thread. While simple, this reduces performance gain, especially for complex ETL jobs.

2. Always Parallel Workers for Reading PG tables
   Use parallel workers (running in separate backend processes) to avoid heap table resource conflicts. However, not all SELECT queries support parallel execution, and other Postgres resources might still be affected.

3. Execution Barrier Between DuckDB and Postgres:
   Introduce a hard barrier where DuckDB completes execution first, then materializes results for Postgres. This resembles the stage-by-stage execution model used in engines like Spark. The implementation would be straightforward, as DuckDB already supports materialization APIs and memory management through spilling.

[JelteF]

I found there were race conditions on Postgres resources like the shared buffer, leading to backend crashes.

Could you go in a bit more detail here? It seems like we're not taking the global lock in enough places then, if that's possible.

[JelteF]

Yeah that's roughly what I meant indeed. But reading the code better, I don't really understand why that would matter. Is it because we pass true as the second argument to PendingQuery? If we don't does that solve your issue too? Because then the result would first materialize, i.e. your option 3.

[YuweiXiao]

Ah, YES! I totally missed the allow_stream_result param! Right now, I'm materializing the result manually, like

if (state->query_results->type == duckdb::QueryResultType::STREAM_RESULT) {
    auto &stream_result = state->query_results->Cast<duckdb::StreamQueryResult>();
    state->materialized_results = stream_result.Materialize();
} else {
    /* Already a materialized result, just cast it */
    state->materialized_results = duckdb::unique_ptr<duckdb::MaterializedQueryResult>(
		  static_cast<duckdb::MaterializedQueryResult *>(state->query_results.release()));
}

Let me try with the allow_stream_result param

[YuweiXiao]

Setting allow_stream_result to false works like a charm! How about we set it to false as the default?

[JelteF]

Yeah sounds good. It would be helpful if you have a reproducing case btw. Because eventually, I'd like for the lock to be taken an released correctly so we can use streaming.

[JelteF]

You wanna make a PR for this btw?