diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index 48cc17ea4f2..9929edc65b7 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -1687,7 +1687,11 @@ 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; + ScanKeyData scankey; + void *inplace_state; + Form_pg_database datform; + int notherbackends; int npreparedxacts; int nslots, nslots_active; @@ -1817,39 +1821,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 +1849,61 @@ dropdb(const char *dbname, bool missing_ok, bool force) */ pgstat_drop_database(db_id); + /* + * 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. + */ + ScanKeyInit(&scankey, + Anum_pg_database_datname, + BTEqualStrategyNumber, F_NAMEEQ, + CStringGetDatum(dbname)); + systable_inplace_update_begin(pgdbrel, DatabaseNameIndexId, true, + NULL, 1, &scankey, &tup, &inplace_state); + if (!HeapTupleIsValid(tup)) + elog(ERROR, "cache lookup failed for database %u", db_id); + datform = (Form_pg_database) GETSTRUCT(tup); + datform->datconnlimit = DATCONNLIMIT_INVALID_DB; + systable_inplace_update_finish(inplace_state, 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); + heap_freetuple(tup); + + 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 @@ # 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';