Make drop database safer

From: Alexandra Wang <lewang(at)pivotal(dot)io>
To: pgsql-hackers(at)postgresql(dot)org
Cc: Ashwin Agrawal <aagrawal(at)pivotal(dot)io>
Subject: Make drop database safer
Date: 2019-02-09 00:36:13
Message-ID: CACiyaSqPp3Vuyok38QBG7HioYXF50BUHK8S6HNDT6PcRt791WA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Current sequence of operations for drop database (dropdb())
1. Start Transaction
2. Make catalog changes
3. Drop database buffers
4. Forget database fsync requests
5. Checkpoint
6. Delete database directory
7. Commit Transaction

Problem
This sequence is unsafe from couple of fronts. Like if drop database,
aborts (means system can crash/shutdown can happen) right after buffers are
dropped step 3 or step 4. The database will still exist and fully
accessible but will loose the data from the dirty buffers. This seems very
bad.

Operation can abort after step 5 as well in which can the entries remain in
catalog but the database is not accessible. Which is bad as well but not as
severe as above case mentioned, where it exists but some stuff goes
magically missing.

Repo:
```
CREATE DATABASE test;
\c test
CREATE TABLE t1(a int); CREATE TABLE t2(a int); CREATE TABLE t3(a int);
\c postgres
DROP DATABASE test; <<====== kill the session after DropDatabaseBuffers()
(make sure to issue checkpoint before killing the session)
```

Proposed ways to fix
1. CommitTransactionCommand() right after step 2. This makes it fully safe
as the catalog will have the database dropped. Files may still exist on
disk in some cases which is okay. This also makes it consistent with the
approach used in movedb().

2. Alternative way to make it safer is perform Checkpoint (step 5) just
before dropping database buffers, to avoid the unsafe nature. Caveats of
this solution is:
- Performs IO for data which in success case anyways will get deleted
- Still doesn't cover the case where catalog has the database entry but
files are removed from disk

3. One more fancier approach is to use pending delete mechanism used by
relation drops, to perform these non-catalog related activities at commit.
Easily, the pending delete structure can be added boolean to convey
database directory dropping instead of file. Given drop database can't be
performed inside transaction, not needed to be done this way, but this
makes it one consistent approach used to deal with on-disk removal.

We passing along patch with fix 1, as seems most promising to us. But we
would love to hear thoughts on other solutions mentioned.
===================================
diff --git a/src/backend/commands/dbcommands.c
b/src/backend/commands/dbcommands.c
index d207cd899f..af1b1e0896 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -917,6 +917,9 @@ dropdb(const char *dbname, bool missing_ok)
*/
dropDatabaseDependencies(db_id);

+ CommitTransactionCommand();
+ StartTransactionCommand();
+
/*
* Drop db-specific replication slots.
*/
===================================

Thanks,
Ashwin and Alex

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2019-02-09 00:41:40 Re: Fixing findDependentObjects()'s dependency on scan order (regressions in DROP diagnostic messages)
Previous Message Tomas Vondra 2019-02-08 22:38:12 Re: libpq compression