This is an automated email from the ASF dual-hosted git repository. reshke pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/cloudberry.git
commit a4d181e67d5a1a6633219206f4f17632eac0d0d8 Author: Kirill Reshke <[email protected]> AuthorDate: Wed Aug 19 07:38:54 2026 +0500 Re-apply DROP DATABASE getting interrupted fix and adapt for MPP In CBDB, DROP database is hazardous in few kill-9 scenarios. Most of them are already fixed in PostgreSQL, so bring this fix to our fork. Additional change here required to move DROP DATABASE on QE dispatch AFTER marking database as invalid on QD, otherwise we can get in case where QD did not mark database as invalid, and allows connections, while QE already removed datadirs. FAULT-INJECTION test hits this (without code fix) This is re-apply of https://git.postgresql.org/cgit/postgresql.git/commit/?id=c66a7d75e652801043ece99b6a8f89fd9513eaaa with additional cbdb fixes & tests Until now, when DROP DATABASE got interrupted in the wrong moment, the removal of the pg_database row would also roll back, even though some irreversible steps have already been taken. E.g. DropDatabaseBuffers() might have thrown out dirty buffers, or files could have been unlinked. But we continued to allow connections to such a corrupted database. To fix this, mark databases invalid with an in-place update, just before starting to perform irreversible steps. As we can't add a new column in the back branches, we use pg_database.datconnlimit = -2 for this purpose. An invalid database cannot be connected to anymore, but can still be dropped. Unfortunately we can't easily add output to psql's \l to indicate that some database is invalid, it doesn't fit in any of the existing columns. Add tests verifying that a interrupted DROP DATABASE is handled correctly in the backend and in various tools. In cloudberry, we also move segment DDL dispach after pg_database tuple transaction commit, otherwise, if killed, segments will already drop thier datadirs, while QD not. Reported-by: Evgeny Morozov <[email protected]> Author: Andres Freund <[email protected]> Co-authored-by: reshke <[email protected]> (Cloudberry part) Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Thomas Munro <[email protected]> Discussion: https://postgr.es/m/[email protected] Discussion: https://postgr.es/m/[email protected] Backpatch: 11-, bug present in all supported versions --- src/backend/commands/dbcommands.c | 87 +++++++++++++--------- src/backend/utils/init/postinit.c | 29 ++++++++ src/bin/pg_dump/t/002_pg_dump.pl | 3 + .../expected/dropdb_crash_before_remove_tuple.out | 75 +++++++++++++++++++ .../isolation2/expected/dropdb_crash_hazards.out | 73 ++++++++++++++++++ src/test/isolation2/isolation2_crash_schedule | 1 + .../sql/dropdb_crash_before_remove_tuple.sql | 48 ++++++++++++ src/test/isolation2/sql/dropdb_crash_hazards.sql | 48 ++++++++++++ 8 files changed, 330 insertions(+), 34 deletions(-) diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index 48cc17ea4f2..2afd694a967 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -1687,7 +1687,9 @@ dropdb(const char *dbname, bool missing_ok, bool force) Oid db_id = InvalidOid; bool db_istemplate = true; Relation pgdbrel; - HeapTuple tup; int notherbackends; + HeapTuple tup; + Form_pg_database datform; + int notherbackends; int npreparedxacts; int nslots, nslots_active; @@ -1817,39 +1819,6 @@ dropdb(const char *dbname, bool missing_ok, bool force) dbname), errdetail_busy_db(notherbackends, npreparedxacts))); - /* - * Free the database on the segDBs - */ - if (Gp_role == GP_ROLE_DISPATCH) - { - StringInfoData buffer; - - initStringInfo(&buffer); - - appendStringInfo(&buffer, "DROP DATABASE IF EXISTS %s", quote_identifier(dbname)); - - /* - * Do the DROP DATABASE as part of a distributed transaction. - */ - CdbDispatchCommand(buffer.data, - DF_CANCEL_ON_ERROR| - DF_NEED_TWO_PHASE| - DF_WITH_SNAPSHOT, - NULL); - pfree(buffer.data); - } - - /* - * Remove the database's tuple from pg_database. - */ - tup = SearchSysCache1(DATABASEOID, ObjectIdGetDatum(db_id)); - if (!HeapTupleIsValid(tup)) - elog(ERROR, "cache lookup failed for database %u", db_id); - - CatalogTupleDelete(pgdbrel, &tup->t_self); - - ReleaseSysCache(tup); - /* * Delete any comments or security labels associated with the database. */ @@ -1878,6 +1847,56 @@ dropdb(const char *dbname, bool missing_ok, bool force) */ pgstat_drop_database(db_id); + tup = SearchSysCacheCopy1(DATABASEOID, ObjectIdGetDatum(db_id)); + if (!HeapTupleIsValid(tup)) + elog(ERROR, "cache lookup failed for database %u", db_id); + datform = (Form_pg_database) GETSTRUCT(tup); + + /* + * Except for the deletion of the catalog row, subsequent actions are not + * transactional (consider DropDatabaseBuffers() discarding modified + * buffers). But we might crash or get interrupted below. To prevent + * accesses to a database with invalid contents, mark the database as + * invalid using an in-place update. + * + * We need to flush the WAL before continuing, to guarantee the + * modification is durable before performing irreversible filesystem + * operations. + */ + datform->datconnlimit = DATCONNLIMIT_INVALID_DB; + heap_inplace_update(pgdbrel, tup); + XLogFlush(XactLastRecEnd); + + /* + * Also delete the tuple - transactionally. If this transaction commits, + * the row will be gone, but if we fail, dropdb() can be invoked again. + */ + CatalogTupleDelete(pgdbrel, &tup->t_self); + + SIMPLE_FAULT_INJECTOR("after_dbdrop_tuple_update_tuple"); + + /* + * Free the database on the segDBs + */ + if (Gp_role == GP_ROLE_DISPATCH) + { + StringInfoData buffer; + + initStringInfo(&buffer); + + appendStringInfo(&buffer, "DROP DATABASE IF EXISTS %s", quote_identifier(dbname)); + + /* + * Do the DROP DATABASE as part of a distributed transaction. + */ + CdbDispatchCommand(buffer.data, + DF_CANCEL_ON_ERROR| + DF_NEED_TWO_PHASE| + DF_WITH_SNAPSHOT, + NULL); + pfree(buffer.data); + } + /* * Drop db-specific replication slots. */ diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index b769c3b249f..d02f33158ef 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -1285,6 +1285,35 @@ InitPostgres(const char *in_dbname, Oid dboid, */ InvalidateCatalogSnapshot(); + /* + * Recheck pg_database to make sure the target database hasn't gone away. + * If there was a concurrent DROP DATABASE, this ensures we will die + * cleanly without creating a mess. + */ + if (!bootstrap) + { + HeapTuple tuple; + Form_pg_database datform; + + tuple = GetDatabaseTuple(dbname); + if (!HeapTupleIsValid(tuple) || + MyDatabaseId != ((Form_pg_database) GETSTRUCT(tuple))->oid || + MyDatabaseTableSpace != ((Form_pg_database) GETSTRUCT(tuple))->dattablespace) + ereport(FATAL, + (errcode(ERRCODE_UNDEFINED_DATABASE), + errmsg("database \"%s\" does not exist", dbname), + errdetail("It seems to have just been dropped or renamed."))); + + datform = (Form_pg_database) GETSTRUCT(tuple); + if (database_is_invalid_form(datform)) + { + ereport(FATAL, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot connect to invalid database \"%s\"", dbname), + errhint("Use DROP DATABASE to drop invalid databases.")); + } + } + /* * Now we should be able to access the database directory safely. Verify * it's there and looks reasonable. diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl index 201b80ea709..d3c2533b02b 100644 --- a/src/bin/pg_dump/t/002_pg_dump.pl +++ b/src/bin/pg_dump/t/002_pg_dump.pl @@ -1969,6 +1969,9 @@ my %tests = ( # invalid databases should never be dumped like => {}, + not_like => { + pg_dumpall_dbprivs => 1, + }, }, 'CREATE ACCESS METHOD gist2' => { diff --git a/src/test/isolation2/expected/dropdb_crash_before_remove_tuple.out b/src/test/isolation2/expected/dropdb_crash_before_remove_tuple.out new file mode 100644 index 00000000000..2b9908729d1 --- /dev/null +++ b/src/test/isolation2/expected/dropdb_crash_before_remove_tuple.out @@ -0,0 +1,75 @@ +-- Test crash recovery when the coordinator panics right before +-- removing the database's tuple from pg_database. +-- +-- The fault injection point 'before_remove_pg_database_tuple' is +-- reached in dropdb() after all the preconditions have been checked +-- (permissions, active backends, etc.) and after the DROP has been +-- dispatched to the segments, but *before* the pg_database tuple is +-- deleted on the coordinator. If the coordinator panics here, the +-- transaction aborts and the tuple must remain, so the database +-- should still be present in pg_database after recovery. + +-- start_matchsubs +-- m/PANIC: fault triggered, fault name:'before_remove_pg_database_tuple' fault type:'panic'\n/ +-- s/PANIC: fault triggered, fault name:'before_remove_pg_database_tuple' fault type:'panic'\n// +-- end_matchsubs + +-- Create the extension that provides the fault injector functions. +1:CREATE EXTENSION IF NOT EXISTS gp_inject_fault; +CREATE EXTENSION + +-- Create a database to be dropped. +1:DROP DATABASE IF EXISTS dropdb_crash_test; +DROP DATABASE +1:CREATE DATABASE dropdb_crash_test; +CREATE DATABASE + +-- Inject a panic fault on the coordinator (content = -1, role = p), +-- at the point right before the pg_database tuple is removed. +1:SELECT gp_inject_fault('before_remove_pg_database_tuple', 'panic', dbid) + FROM gp_segment_configuration WHERE content = -1 AND role = 'p'; + gp_inject_fault +----------------- + Success: +(1 row) + +-- DROP DATABASE will panic right before removing the tuple. +1:DROP DATABASE dropdb_crash_test; +PANIC: fault triggered, fault name:'before_remove_pg_database_tuple' fault type:'panic' +server closed the connection unexpectedly + This probably means the server terminated abnormally + before or while processing the request. + +-- Wait for the coordinator to come back up after crash recovery. +2:SELECT 1; + ?column? +---------- + 1 +(1 row) + +-- The database tuple should still be in pg_database because the +-- transaction that removes it was aborted by the panic. +2:SELECT datname FROM pg_database WHERE datname = 'dropdb_crash_test'; + datname +------------------- + dropdb_crash_test +(1 row) + +-- Reset any leftover faults as a safety net in case the panic did +-- not fire for some reason. +2:SELECT gp_inject_fault('before_remove_pg_database_tuple', 'reset', dbid) + FROM gp_segment_configuration WHERE content = -1 AND role = 'p'; + gp_inject_fault +----------------- + Success: +(1 row) + +-- Now drop the database for real, to clean up. +2:DROP DATABASE dropdb_crash_test; +DROP DATABASE + +-- Verify it's gone. +2:SELECT datname FROM pg_database WHERE datname = 'dropdb_crash_test'; + datname +--------- +(0 rows) diff --git a/src/test/isolation2/expected/dropdb_crash_hazards.out b/src/test/isolation2/expected/dropdb_crash_hazards.out new file mode 100644 index 00000000000..08430f79dc0 --- /dev/null +++ b/src/test/isolation2/expected/dropdb_crash_hazards.out @@ -0,0 +1,73 @@ +-- Test crash recovery when the coordinator panics right before +-- removing the database's tuple from pg_database. +-- +-- The fault injection point 'after_dbdrop_tuple_update_tuple' is +-- reached in dropdb() after all the preconditions have been checked +-- (permissions, active backends, etc.) and after the DROP has been +-- dispatched to the segments, but *before* the pg_database tuple is +-- deleted on the coordinator. If the coordinator panics here, the +-- transaction aborts and the tuple must remain, so the database +-- should still be present in pg_database after recovery. + +-- start_matchsubs +-- m/PANIC: fault triggered, fault name:'after_dbdrop_tuple_update_tuple' fault type:'panic'\n/ +-- s/PANIC: fault triggered, fault name:'after_dbdrop_tuple_update_tuple' fault type:'panic'\n// +-- end_matchsubs + +-- Create the extension that provides the fault injector functions. +1:CREATE EXTENSION IF NOT EXISTS gp_inject_fault; +CREATE + +-- Create a database to be dropped. +1:DROP DATABASE IF EXISTS dropdb_crash_test; +DROP +1:CREATE DATABASE dropdb_crash_test; +CREATE + +-- Inject a panic fault on the coordinator (content = -1, role = p), +-- at the point right before the pg_database tuple is removed. +1:SELECT gp_inject_fault('after_dbdrop_tuple_update_tuple', 'panic', dbid) FROM gp_segment_configuration WHERE content = -1 AND role = 'p'; + gp_inject_fault +----------------- + Success: +(1 row) + +-- DROP DATABASE will panic right before removing the tuple. +1:DROP DATABASE dropdb_crash_test; +PANIC: fault triggered, fault name:'after_dbdrop_tuple_update_tuple' fault type:'panic' +server closed the connection unexpectedly + This probably means the server terminated abnormally + before or while processing the request. + +-- Wait for the coordinator to come back up after crash recovery. +2:SELECT 1; + ?column? +---------- + 1 +(1 row) + +-- The database tuple should still be in pg_database because the +-- transaction that removes it was aborted by the panic. +2:SELECT datconnlimit, datname FROM pg_database WHERE datname = 'dropdb_crash_test'; + datconnlimit | datname +--------------+------------------- + -2 | dropdb_crash_test +(1 row) + +-- Reset any leftover faults as a safety net in case the panic did +-- not fire for some reason. +2:SELECT gp_inject_fault('after_dbdrop_tuple_update_tuple', 'reset', dbid) FROM gp_segment_configuration WHERE content = -1 AND role = 'p'; + gp_inject_fault +----------------- + Success: +(1 row) + +-- Now drop the database for real, to clean up. +2:DROP DATABASE dropdb_crash_test; +DROP + +-- Verify it's gone. +2:SELECT datname FROM pg_database WHERE datname = 'dropdb_crash_test'; + datname +--------- +(0 rows) diff --git a/src/test/isolation2/isolation2_crash_schedule b/src/test/isolation2/isolation2_crash_schedule index b1013619299..c250e3f9329 100644 --- a/src/test/isolation2/isolation2_crash_schedule +++ b/src/test/isolation2/isolation2_crash_schedule @@ -9,6 +9,7 @@ test: unlogged_appendonly_tables test: udf_exception_blocks_panic_scenarios test: ao_same_trans_truncate_crash test: frozen_insert_crash +test: dropdb_crash_hazards test: prevent_ao_wal diff --git a/src/test/isolation2/sql/dropdb_crash_before_remove_tuple.sql b/src/test/isolation2/sql/dropdb_crash_before_remove_tuple.sql new file mode 100644 index 00000000000..d1d02b21d94 --- /dev/null +++ b/src/test/isolation2/sql/dropdb_crash_before_remove_tuple.sql @@ -0,0 +1,48 @@ +-- Test crash recovery when the coordinator panics right before +-- removing the database's tuple from pg_database. +-- +-- The fault injection point 'after_dbdrop_tuple_update_tuple' is +-- reached in dropdb() after all the preconditions have been checked +-- (permissions, active backends, etc.) and after the DROP has been +-- dispatched to the segments, but *before* the pg_database tuple is +-- deleted on the coordinator. If the coordinator panics here, the +-- transaction aborts and the tuple must remain, so the database +-- should still be present in pg_database after recovery. + +-- start_matchsubs +-- m/PANIC: fault triggered, fault name:'after_dbdrop_tuple_update_tuple' fault type:'panic'\n/ +-- s/PANIC: fault triggered, fault name:'after_dbdrop_tuple_update_tuple' fault type:'panic'\n// +-- end_matchsubs + +-- Create the extension that provides the fault injector functions. +1:CREATE EXTENSION IF NOT EXISTS gp_inject_fault; + +-- Create a database to be dropped. +1:DROP DATABASE IF EXISTS dropdb_crash_test; +1:CREATE DATABASE dropdb_crash_test; + +-- Inject a panic fault on the coordinator (content = -1, role = p), +-- at the point right before the pg_database tuple is removed. +1:SELECT gp_inject_fault('after_dbdrop_tuple_update_tuple', 'panic', dbid) + FROM gp_segment_configuration WHERE content = -1 AND role = 'p'; + +-- DROP DATABASE will panic right before removing the tuple. +1:DROP DATABASE dropdb_crash_test; + +-- Wait for the coordinator to come back up after crash recovery. +2:SELECT 1; + +-- The database tuple should still be in pg_database because the +-- transaction that removes it was aborted by the panic. +2:SELECT datname FROM pg_database WHERE datname = 'dropdb_crash_test'; + +-- Reset any leftover faults as a safety net in case the panic did +-- not fire for some reason. +2:SELECT gp_inject_fault('after_dbdrop_tuple_update_tuple', 'reset', dbid) + FROM gp_segment_configuration WHERE content = -1 AND role = 'p'; + +-- Now drop the database for real, to clean up. +2:DROP DATABASE dropdb_crash_test; + +-- Verify it's gone. +2:SELECT datname FROM pg_database WHERE datname = 'dropdb_crash_test'; diff --git a/src/test/isolation2/sql/dropdb_crash_hazards.sql b/src/test/isolation2/sql/dropdb_crash_hazards.sql new file mode 100644 index 00000000000..35e5f9c79df --- /dev/null +++ b/src/test/isolation2/sql/dropdb_crash_hazards.sql @@ -0,0 +1,48 @@ +-- Test crash recovery when the coordinator panics right before +-- removing the database's tuple from pg_database. +-- +-- The fault injection point 'after_dbdrop_tuple_update_tuple' is +-- reached in dropdb() after all the preconditions have been checked +-- (permissions, active backends, etc.) and after the DROP has been +-- dispatched to the segments, but *before* the pg_database tuple is +-- deleted on the coordinator. If the coordinator panics here, the +-- transaction aborts and the tuple must remain, so the database +-- should still be present in pg_database after recovery. + +-- start_matchsubs +-- m/PANIC: fault triggered, fault name:'after_dbdrop_tuple_update_tuple' fault type:'panic'\n/ +-- s/PANIC: fault triggered, fault name:'after_dbdrop_tuple_update_tuple' fault type:'panic'\n// +-- end_matchsubs + +-- Create the extension that provides the fault injector functions. +1:CREATE EXTENSION IF NOT EXISTS gp_inject_fault; + +-- Create a database to be dropped. +1:DROP DATABASE IF EXISTS dropdb_crash_test; +1:CREATE DATABASE dropdb_crash_test; + +-- Inject a panic fault on the coordinator (content = -1, role = p), +-- at the point right before the pg_database tuple is removed. +1:SELECT gp_inject_fault('after_dbdrop_tuple_update_tuple', 'panic', dbid) + FROM gp_segment_configuration WHERE content = -1 AND role = 'p'; + +-- DROP DATABASE will panic right before removing the tuple. +1:DROP DATABASE dropdb_crash_test; + +-- Wait for the coordinator to come back up after crash recovery. +2:SELECT 1; + +-- The database tuple should still be in pg_database because the +-- transaction that removes it was aborted by the panic. +2:SELECT datconnlimit, datname FROM pg_database WHERE datname = 'dropdb_crash_test'; + +-- Reset any leftover faults as a safety net in case the panic did +-- not fire for some reason. +2:SELECT gp_inject_fault('after_dbdrop_tuple_update_tuple', 'reset', dbid) + FROM gp_segment_configuration WHERE content = -1 AND role = 'p'; + +-- Now drop the database for real, to clean up. +2:DROP DATABASE dropdb_crash_test; + +-- Verify it's gone. +2:SELECT datname FROM pg_database WHERE datname = 'dropdb_crash_test'; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
