Support multi-statement transactions #433

JelteF · 2024-11-14T09:08:07Z

This starts supporting multi-statement transactions. It imposes one important restriction that is necessary to ensure the expected ACID guarantees: You cannot write to both a Postgres table and a DuckDB table in the same transaction.

This also means that we do not allow doing DuckDB DDL inside a transaction, because that needs to write both to DuckDB and to the Postgres catalogs.

Either of these restrictions might be removed in a follow up PR. Especifically the DDL restriction is one that we would like to remove (and probably can). But for now this PR only implements the obviously correct case, and any further removal of restrictions will need some design/tradeoffs discussion.

Fixes #419

JelteF · 2024-11-19T13:28:18Z

include/pgduckdb/pgduckdb_table_am.hpp

-#include "postgres.h"
-#include "access/tableam.h"
-}
+#include "pgduckdb/pg/declarations.hpp"


This was needed because we cannot define the XactEvent and SubXactEvent twice, and tableam.h was including xact.h from Postgres. Which would then cause compilation errors if pgduckdb_table_am.hpp was included in the same file as pg/transactions.hpp

test/regression/sql/transactions.sql

JelteF · 2024-11-20T16:16:17Z

test/regression/expected/transaction_errors.out

@@ -1,6 +1,8 @@
 CREATE TABLE foo AS SELECT 'bar'::text AS t;
 BEGIN; SET duckdb.force_execution = true; SELECT t::integer AS t1 FROM foo; ROLLBACK;
-ERROR:  invalid input syntax for type integer: "bar"
+ERROR:  (PGDuckDB/Duckdb_ExecCustomScan) Conversion Error: Could not convert string 'bar' to INT32


Previously this was being executed in Postgres, now it's executed in duckdb again. We still don't crash though.

JelteF · 2024-11-20T16:17:25Z

test/regression/expected/duckdb_recycle.out

+    AS
+$$
+BEGIN
+    CALL duckdb.recycle_ddb();


I changed this to use the CALL syntax, so we can easily detect when recycle_ddb is not run at the top level (like shown here).

JelteF · 2024-11-20T16:18:32Z

test/pycheck/prepared_test.py

@@ -125,8 +125,8 @@ def test_prepared_pipeline(conn: Connection):
        # the duckdb table that should fail because the insert into the heap
        # table opens an implicit transaction.
        with pytest.raises(
-            psycopg.errors.ActiveSqlTransaction,
-            match="DuckDB queries cannot be executed within a pipeline",
+            psycopg.errors.InternalError,


It would be nice if we would do some errorcode conversions between DuckDB and Postgres errors. That seems like a task for a separate PR though.

src/pgduckdb_duckdb.cpp

Only supports multi-statement transactions for non-DDL queries.

Y--

LGTM, tests show that it is doing what we expect. Added a few comments :-)

src/pgduckdb_ddl.cpp

src/pgduckdb_duckdb.cpp

Y-- · 2024-11-27T09:37:21Z

src/pgduckdb_hooks.cpp

@@ -207,8 +203,21 @@ DuckdbPlannerHook_Cpp(Query *parse, const char *query_string, int cursor_options
 			}
 			/* If we can't create a plan, we'll fall back to Postgres */
 		}
+		if (parse->commandType != CMD_SELECT && pgduckdb::DuckdbDidWrites() && pgduckdb::IsInTransactionBlock(true)) {


Reading statements like this, I feel it might be useful to either nest PG APIs in a sub-namespace pg or to include PG in their name like PGIsInTransactionBlock.
Since we're bridging two databases, it is helpful to be extra-pedentic when dealing with their APIs.

What do you think?

Agreed, I was thinking the same. Done now, let me know what you think. We should probably update the other code in the pg directory in a follow up PR for consistency.

This looks really good, much clearer. And agreed to update the rest of the repo to follow the same philosophy.

src/pgduckdb_planner.cpp

Y-- · 2024-11-27T09:48:47Z

src/pgduckdb_xact.cpp

+ */
+bool
+IsInTransactionBlock() {
+	return IsInTransactionBlock(top_level_statement);


I assume this is safe to be called without a lock on top_level_statement because this will only be called in PG hooks, not in DDB threads?
(same for duckdb_command_id and potential elog(ERROR below?)

I introduced a pgduckdb::pg and pgduckdb::ddb namespace now. I moved this to the pgduckdb::pg namespace. I think we should write down assumptions about using functions from the pgduckdb::pg namespace. The two main assumptions are imo:

They should be called from a Postgres hook, or while holding the global PG lock from a duckdb thread.

They do not throw elog errors, only exceptions

Yeah I think that makes total sense.

Y--

LGTM! 🚀

JelteF · 2024-11-27T16:38:39Z

I merged this a bit quicker than I originally intended, mainly to fix the broken build on the main branch due to this PG change: https://www.postgresql.org/message-id/flat/CAGECzQQS1Oun40BssQDmuHc86teXqr%2BLOndy5NSVdFce9evZvQ%40mail.gmail.com#cf1398dd28d67be9fce7e75096343f07

Remove the lockdowns introduced in duckdb/pg_duckdb#433 since the same restriction doesn't apply to us. In our case, Postgres remains the source of truth for columnstore tables, and DuckDB serves purely as the execution engine for reading and writing to those tables.

JelteF force-pushed the transactions branch 7 times, most recently from 703bfda to 37fd2da Compare November 19, 2024 13:24

JelteF commented Nov 19, 2024

View reviewed changes

JelteF force-pushed the transactions branch 3 times, most recently from e2955e1 to 3cf0422 Compare November 20, 2024 16:02

JelteF commented Nov 20, 2024

View reviewed changes

test/regression/sql/transactions.sql Outdated Show resolved Hide resolved

JelteF commented Nov 20, 2024

View reviewed changes

JelteF force-pushed the transactions branch 3 times, most recently from 375a90e to 8319881 Compare November 20, 2024 16:53

JelteF commented Nov 20, 2024

View reviewed changes

src/pgduckdb_duckdb.cpp Outdated Show resolved Hide resolved

JelteF force-pushed the transactions branch 8 times, most recently from 0b49790 to fcf7a01 Compare November 26, 2024 15:49

Support multi-statement transactions

fd185bf

Only supports multi-statement transactions for non-DDL queries.

JelteF force-pushed the transactions branch from fcf7a01 to fd185bf Compare November 26, 2024 16:09

JelteF marked this pull request as ready for review November 26, 2024 16:09

JelteF requested review from Y-- and mkaruza November 26, 2024 16:09

Y-- reviewed Nov 27, 2024

View reviewed changes

Apply review feedback & add pg and ddb subnamespaces

504a315

JelteF force-pushed the transactions branch from 14ae220 to 504a315 Compare November 27, 2024 11:29

Y-- approved these changes Nov 27, 2024

View reviewed changes

Don't run InvokeCPPFunc if extension is not registered

03590d5

JelteF force-pushed the transactions branch from 84591da to 03590d5 Compare November 27, 2024 14:17

JelteF enabled auto-merge (squash) November 27, 2024 15:38

Y-- approved these changes Nov 27, 2024

View reviewed changes

JelteF merged commit 3f0eab7 into main Nov 27, 2024
5 checks passed

JelteF deleted the transactions branch November 27, 2024 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multi-statement transactions #433

Support multi-statement transactions #433

JelteF commented Nov 14, 2024 •

edited

Loading

JelteF Nov 19, 2024

JelteF Nov 20, 2024

JelteF Nov 20, 2024

JelteF Nov 20, 2024

Y-- left a comment

Y-- Nov 27, 2024

JelteF Nov 27, 2024 •

edited

Loading

Y-- Nov 27, 2024

Y-- Nov 27, 2024

JelteF Nov 27, 2024

Y-- Nov 27, 2024

Y-- left a comment

JelteF commented Nov 27, 2024

Support multi-statement transactions #433

Support multi-statement transactions #433

Conversation

JelteF commented Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Y-- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelteF Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Y-- left a comment

Choose a reason for hiding this comment

JelteF commented Nov 27, 2024

JelteF commented Nov 14, 2024 •

edited

Loading

JelteF Nov 27, 2024 •

edited

Loading