From: Tom Lane Date: Fri, 18 Jan 2013 23:06:50 +0000 (-0500) Subject: Protect against SnapshotNow race conditions in pg_tablespace scans. X-Git-Tag: REL8_3_23~10 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=6eedec19f0785c107803d7011955956f81e9b45a;p=postgresql 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 f7bf94883a..351b92b5b6 100644 --- 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);