store: Make sure we always map the right set of foreign tables

Author: lutter

store/postgres/src/connection_pool.rs

@@ -1037,16 +1037,14 @@ impl PoolInner {
         let result = pool
             .configure_fdw(coord.servers.as_ref())
             .and_then(|()| pool.drop_cross_shard_views())
-            .and_then(|()| migrate_schema(&pool.logger, &mut conn))
-            .and_then(|count| {
-                pool.create_cross_shard_views(coord.servers.as_ref())
-                    .map(|()| count)
-            });
+            .and_then(|()| migrate_schema(&pool.logger, &mut conn));
         debug!(&pool.logger, "Release migration lock");
         advisory_lock::unlock_migration(&mut conn).unwrap_or_else(|err| {
             die(&pool.logger, "failed to release migration lock", &err);
         });
-        let result = result.and_then(|count| coord.propagate(&pool, count));
+        let result = result
+            .and_then(|count| coord.propagate(&pool, count))
+            .and_then(|()| pool.create_cross_shard_views(coord.servers.as_ref()));
         result.unwrap_or_else(|err| die(&pool.logger, "migrations failed", &err));
 
         // Locale check
@@ -1178,9 +1176,9 @@ impl PoolInner {
         .await
     }
 
-    // The foreign server `server` had schema changes, and we therefore need
-    // to remap anything that we are importing via fdw to make sure we are
-    // using this updated schema
+    /// The foreign server `server` had schema changes, and we therefore
+    /// need to remap anything that we are importing via fdw to make sure we
+    /// are using this updated schema
     pub fn remap(&self, server: &ForeignServer) -> Result<(), StoreError> {
         if &server.shard == &*PRIMARY_SHARD {
             info!(&self.logger, "Mapping primary");
@@ -1211,10 +1209,6 @@ impl MigrationCount {
     fn had_migrations(&self) -> bool {
         self.old != self.new
     }
-
-    fn is_new(&self) -> bool {
-        self.old == 0
-    }
 }
 
 /// Run all schema migrations.
@@ -1334,13 +1328,22 @@ impl PoolCoordinator {
     /// code that does _not_ hold the migration lock as it will otherwise
     /// deadlock
     fn propagate(&self, pool: &PoolInner, count: MigrationCount) -> Result<(), StoreError> {
-        // pool is a new shard, map all other shards into it
-        if count.is_new() {
-            for server in self.servers.iter() {
-                pool.remap(server)?;
-            }
+        // Strictly speaking, we only need to remap all these servers into
+        // `pool` if `pool` is a new shard (`count.is_new())`) or if the
+        // list of tables that are mapped have changed from the previous
+        // verison. Since creating foreign tables is pretty fast, we don't
+        // try to be too clever and just recreate these mappings every time.
+        // If that slows startup down unacceptably, we need to do something
+        // better here
+        for server in self.servers.iter() {
+            pool.remap(server)?;
         }
-        // pool had schema changes, refresh the import from pool into all other shards
+
+        // pool had schema changes, refresh the import from pool into all
+        // other shards. This makes sure that schema changes to
+        // already-mapped tables are propagated to all other shards. Since
+        // we run `propagate` after migrations have been applied to `pool`,
+        // we can be sure that these mappings use the correct schema
         if count.had_migrations() {
             let server = self.server(&pool.shard)?;
             for pool in self.pools.lock().unwrap().values() {