From 89275f6c1e7b09504711e9da37fff080bebb5ea8 Mon Sep 17 00:00:00 2001 From: Sasha Krassovsky Date: Fri, 29 Sep 2023 10:39:28 -0800 Subject: [PATCH] Fix invalid database resulting from failed DROP DB (#5423) ## Problem If the control plane happened to respond to a DROP DATABASE request with a non-200 response, we'd abort the DROP DATABASE transaction in the usual spot. However, Postgres for some reason actually performs the drop inside of `standard_ProcessUtility`. As such, the database was left in a weird state after aborting the transaction. We had test coverage of a failed CREATE DATABASE but not a failed DROP DATABASE. ## Summary of changes Since DROP DATABASE can't be inside of a transaction block, we can just forward the DDL changes to the control plane inside of `ProcessUtility_hook`, and if we respond with 500 bail out of `ProcessUtility` before we perform the drop. This change also adds a test, which reproduced the invalid database issue before the fix was applied. --- pgxn/neon/control_plane_connector.c | 7 +++++++ test_runner/regress/test_ddl_forwarding.py | 8 ++++++++ 2 files changed, 15 insertions(+) diff --git a/pgxn/neon/control_plane_connector.c b/pgxn/neon/control_plane_connector.c index debbbce117..8b0035b8e8 100644 --- a/pgxn/neon/control_plane_connector.c +++ b/pgxn/neon/control_plane_connector.c @@ -741,6 +741,13 @@ NeonProcessUtility( break; case T_DropdbStmt: HandleDropDb(castNode(DropdbStmt, parseTree)); + /* + * We do this here to hack around the fact that Postgres performs the drop + * INSIDE of standard_ProcessUtility, which means that if we try to + * abort the drop normally it'll be too late. DROP DATABASE can't be inside + * of a transaction block anyway, so this should be fine to do. + */ + NeonXactCallback(XACT_EVENT_PRE_COMMIT, NULL); break; case T_CreateRoleStmt: HandleCreateRole(castNode(CreateRoleStmt, parseTree)); diff --git a/test_runner/regress/test_ddl_forwarding.py b/test_runner/regress/test_ddl_forwarding.py index 740e489759..d4cf1b4739 100644 --- a/test_runner/regress/test_ddl_forwarding.py +++ b/test_runner/regress/test_ddl_forwarding.py @@ -211,4 +211,12 @@ def test_ddl_forwarding(ddl: DdlForwardingContext): ddl.wait() ddl.failures(False) + cur.execute("CREATE DATABASE failure WITH OWNER=cork") + ddl.wait() + with pytest.raises(psycopg2.InternalError): + ddl.failures(True) + cur.execute("DROP DATABASE failure") + ddl.wait() + ddl.pg.connect(dbname="failure") # Ensure we can connect after a failed drop + conn.close()