Protect against SnapshotNow race conditions in pg_tablespace scans.

Use of SnapshotNow is known to expose us to race conditions if the tuple(s)
being sought could be updated by concurrently-committing transactions.
CREATE DATABASE and DROP DATABASE are particularly exposed because they do
heavyweight filesystem operations during their scans of pg_tablespace,
so that the scans run for a very long time compared to most.  Furthermore,
the potential consequences of a missed or twice-visited row are nastier
than average:

* createdb() could fail with a bogus "file already exists" error, or
  silently fail to copy one or more tablespace's worth of files into the
  new database.

* remove_dbtablespaces() could miss one or more tablespaces, thus failing
  to free filesystem space for the dropped database.

* check_db_file_conflict() could likewise miss a tablespace, leading to an
  OID conflict that could result in data loss either immediately or in
  future operations.  (This seems of very low probability, though, since a
  duplicate database OID would be unlikely to start with.)

Hence, it seems worth fixing these three places to use MVCC snapshots, even
though this will someday be superseded by a generic solution to SnapshotNow
race conditions.

Back-patch to all active branches.

Stephen Frost and Tom Lane

diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index f7bf94883afbaf1da58ed1a76eff4b0cfe355dac..351b92b5b61f579022b3064ac0f6dbbb3206b1e0 100644 (file)
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -107,6 +107,7 @@ createdb(const CreatedbStmt *stmt)
        int                     dbconnlimit = -1;
        int                     ctype_encoding;
        createdb_failure_params fparms;
+       Snapshot        snapshot;
 
        /* Extract options from the statement node tree */
        foreach(option, stmt->options)
@@ -460,6 +461,29 @@ createdb(const CreatedbStmt *stmt)
         */
        RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_FORCE | CHECKPOINT_WAIT);
 
+       /*
+        * Take an MVCC snapshot to use while scanning through pg_tablespace.  For
+        * safety, copy the snapshot (this prevents it from changing if
+        * something else were to request a snapshot during the loop).
+        *
+        * Traversing pg_tablespace with an MVCC snapshot is necessary to provide
+        * us with a consistent view of the tablespaces that exist.  Using
+        * SnapshotNow here would risk seeing the same tablespace multiple times,
+        * or worse not seeing a tablespace at all, if its tuple is moved around
+        * by a concurrent update (eg an ACL change).
+        *
+        * Inconsistency of this sort is inherent to all SnapshotNow scans, unless
+        * some lock is held to prevent concurrent updates of the rows being
+        * sought.      There should be a generic fix for that, but in the meantime
+        * it's worth fixing this case in particular because we are doing very
+        * heavyweight operations within the scan, so that the elapsed time for
+        * the scan is vastly longer than for most other catalog scans.  That
+        * means there's a much wider window for concurrent updates to cause
+        * trouble here than anywhere else.  XXX this code should be changed
+        * whenever a generic fix is implemented.
+        */
+       snapshot = CopySnapshot(GetLatestSnapshot());
+
        /*
         * Once we start copying subdirectories, we need to be able to clean 'em
         * up if we fail.  Use an ENSURE block to make sure this happens.  (This
@@ -477,7 +501,7 @@ createdb(const CreatedbStmt *stmt)
                 * each one to the new database.
                 */
                rel = heap_open(TableSpaceRelationId, AccessShareLock);
-               scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
+               scan = heap_beginscan(rel, snapshot, 0, NULL);
                while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
                {
                        Oid                     srctablespace = HeapTupleGetOid(tuple);
@@ -1322,9 +1346,20 @@ remove_dbtablespaces(Oid db_id)
        Relation        rel;
        HeapScanDesc scan;
        HeapTuple       tuple;
+       Snapshot        snapshot;
+
+       /*
+        * As in createdb(), we'd better use an MVCC snapshot here, since this
+        * scan can run for a long time.  Duplicate visits to tablespaces would be
+        * harmless, but missing a tablespace could result in permanently leaked
+        * files.
+        *
+        * XXX change this when a generic fix for SnapshotNow races is implemented
+        */
+       snapshot = CopySnapshot(GetLatestSnapshot());
 
        rel = heap_open(TableSpaceRelationId, AccessShareLock);
-       scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
+       scan = heap_beginscan(rel, snapshot, 0, NULL);
        while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
        {
                Oid                     dsttablespace = HeapTupleGetOid(tuple);
@@ -1391,9 +1426,19 @@ check_db_file_conflict(Oid db_id)
        Relation        rel;
        HeapScanDesc scan;
        HeapTuple       tuple;
+       Snapshot        snapshot;
+
+       /*
+        * As in createdb(), we'd better use an MVCC snapshot here; missing a
+        * tablespace could result in falsely reporting the OID is unique, with
+        * disastrous future consequences per the comment above.
+        *
+        * XXX change this when a generic fix for SnapshotNow races is implemented
+        */
+       snapshot = CopySnapshot(GetLatestSnapshot());
 
        rel = heap_open(TableSpaceRelationId, AccessShareLock);
-       scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
+       scan = heap_beginscan(rel, snapshot, 0, NULL);
        while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
        {
                Oid                     dsttablespace = HeapTupleGetOid(tuple);