Fix unsafe order of operations in foreign-table DDL commands.

When updating or deleting a system catalog tuple, it's necessary to acquire
RowExclusiveLock on the catalog before looking up the tuple; otherwise a
concurrent VACUUM FULL on the catalog might move the tuple to a different
TID before we can apply the update.  Coding patterns that find the tuple
via a table scan aren't at risk here, but when obtaining the tuple from a
catalog cache, correct ordering is important; and several routines in
foreigncmds.c got it wrong.  Noted while running the regression tests in
parallel with VACUUM FULL of assorted system catalogs.

For consistency I moved all the heap_open calls to the starts of their
functions, including a couple for which there was no actual bug.

Back-patch to 8.4 where foreigncmds.c was added.

diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index d16932ba6a025049f78235b13d528295f69671c5..3a3c131366829018913e8a0b7afa12661718c371 100644 (file)
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -215,6 +215,8 @@ AlterForeignDataWrapperOwner(const char *name, Oid newOwnerId)
        Oid                     fdwId;
        Form_pg_foreign_data_wrapper form;
 
+       rel = heap_open(ForeignDataWrapperRelationId, RowExclusiveLock);
+
        /* Must be a superuser to change a FDW owner */
        if (!superuser())
                ereport(ERROR,
@@ -231,8 +233,6 @@ AlterForeignDataWrapperOwner(const char *name, Oid newOwnerId)
                                                name),
                errhint("The owner of a foreign-data wrapper must be a superuser.")));
 
-       rel = heap_open(ForeignDataWrapperRelationId, RowExclusiveLock);
-
        tup = SearchSysCacheCopy1(FOREIGNDATAWRAPPERNAME, CStringGetDatum(name));
 
        if (!HeapTupleIsValid(tup))
@@ -432,6 +432,8 @@ CreateForeignDataWrapper(CreateFdwStmt *stmt)
        ObjectAddress myself;
        ObjectAddress referenced;
 
+       rel = heap_open(ForeignDataWrapperRelationId, RowExclusiveLock);
+
        /* Must be super user */
        if (!superuser())
                ereport(ERROR,
@@ -455,8 +457,6 @@ CreateForeignDataWrapper(CreateFdwStmt *stmt)
        /*
         * Insert tuple into pg_foreign_data_wrapper.
         */
-       rel = heap_open(ForeignDataWrapperRelationId, RowExclusiveLock);
-
        memset(values, 0, sizeof(values));
        memset(nulls, false, sizeof(nulls));
 
@@ -545,6 +545,8 @@ AlterForeignDataWrapper(AlterFdwStmt *stmt)
        Oid                     fdwhandler;
        Oid                     fdwvalidator;
 
+       rel = heap_open(ForeignDataWrapperRelationId, RowExclusiveLock);
+
        /* Must be super user */
        if (!superuser())
                ereport(ERROR,
@@ -635,9 +637,6 @@ AlterForeignDataWrapper(AlterFdwStmt *stmt)
        }
 
        /* Everything looks good - update the tuple */
-
-       rel = heap_open(ForeignDataWrapperRelationId, RowExclusiveLock);
-
        tp = heap_modify_tuple(tp, RelationGetDescr(rel),
                                                   repl_val, repl_null, repl_repl);
 
@@ -773,6 +772,8 @@ CreateForeignServer(CreateForeignServerStmt *stmt)
        ObjectAddress referenced;
        ForeignDataWrapper *fdw;
 
+       rel = heap_open(ForeignServerRelationId, RowExclusiveLock);
+
        /* For now the owner cannot be specified on create. Use effective user ID. */
        ownerId = GetUserId();
 
@@ -798,8 +799,6 @@ CreateForeignServer(CreateForeignServerStmt *stmt)
        /*
         * Insert tuple into pg_foreign_server.
         */
-       rel = heap_open(ForeignServerRelationId, RowExclusiveLock);
-
        memset(values, 0, sizeof(values));
        memset(nulls, false, sizeof(nulls));
 
@@ -880,6 +879,8 @@ AlterForeignServer(AlterForeignServerStmt *stmt)
        Oid                     srvId;
        Form_pg_foreign_server srvForm;
 
+       rel = heap_open(ForeignServerRelationId, RowExclusiveLock);
+
        tp = SearchSysCacheCopy1(FOREIGNSERVERNAME,
                                                         CStringGetDatum(stmt->servername));
 
@@ -945,9 +946,6 @@ AlterForeignServer(AlterForeignServerStmt *stmt)
        }
 
        /* Everything looks good - update the tuple */
-
-       rel = heap_open(ForeignServerRelationId, RowExclusiveLock);
-
        tp = heap_modify_tuple(tp, RelationGetDescr(rel),
                                                   repl_val, repl_null, repl_repl);
 
@@ -1068,6 +1066,8 @@ CreateUserMapping(CreateUserMappingStmt *stmt)
        ForeignServer *srv;
        ForeignDataWrapper *fdw;
 
+       rel = heap_open(UserMappingRelationId, RowExclusiveLock);
+
        useId = GetUserOidFromMapping(stmt->username, false);
 
        /* Check that the server exists. */
@@ -1093,8 +1093,6 @@ CreateUserMapping(CreateUserMappingStmt *stmt)
        /*
         * Insert tuple into pg_user_mapping.
         */
-       rel = heap_open(UserMappingRelationId, RowExclusiveLock);
-
        memset(values, 0, sizeof(values));
        memset(nulls, false, sizeof(nulls));
 
@@ -1161,6 +1159,8 @@ AlterUserMapping(AlterUserMappingStmt *stmt)
        Oid                     umId;
        ForeignServer *srv;
 
+       rel = heap_open(UserMappingRelationId, RowExclusiveLock);
+
        useId = GetUserOidFromMapping(stmt->username, false);
        srv = GetForeignServerByName(stmt->servername, false);
 
@@ -1218,9 +1218,6 @@ AlterUserMapping(AlterUserMappingStmt *stmt)
        }
 
        /* Everything looks good - update the tuple */
-
-       rel = heap_open(UserMappingRelationId, RowExclusiveLock);
-
        tp = heap_modify_tuple(tp, RelationGetDescr(rel),
                                                   repl_val, repl_null, repl_repl);
 
@@ -1344,11 +1341,13 @@ CreateForeignTable(CreateForeignTableStmt *stmt, Oid relid)
        ForeignServer *server;
 
        /*
-        * Advance command counter to ensure the pg_attribute tuple visible; the
-        * tuple might be updated to add constraints in previous step.
+        * Advance command counter to ensure the pg_attribute tuple is visible;
+        * the tuple might be updated to add constraints in previous step.
         */
        CommandCounterIncrement();
 
+       ftrel = heap_open(ForeignTableRelationId, RowExclusiveLock);
+
        /*
         * For now the owner cannot be specified on create. Use effective user ID.
         */
@@ -1368,8 +1367,6 @@ CreateForeignTable(CreateForeignTableStmt *stmt, Oid relid)
        /*
         * Insert tuple into pg_foreign_table.
         */
-       ftrel = heap_open(ForeignTableRelationId, RowExclusiveLock);
-
        memset(values, 0, sizeof(values));
        memset(nulls, false, sizeof(nulls));