Fix Back-and-forth RELOAD Bug (#330)

We identified a bug where RELOAD fails to update the pools.

To reproduce you need to start at some config state, modify that state a bit, reload, revert the configs back to the original state, and reload. The last reload will fail to update the pool because PgCat "thinks" the pool state didn't change.

This is because we use a HashSet to keep track of config hashes but we never remove values from it.
Say we start with State A, we modify pool configs to State B and reload. Now the POOL_HASHES struct has State A and State B. Attempting to go back to State A will encounter a hashset hit which is interpreted by PgCat as "Configs are the same, no need to reload pools"

We fix this by attaching a config_hash value to ConnectionPool object and we calculate that value when we create the pool. This eliminates the need for a global variable. One shortcoming here is that changing any config under one user in the pool will trigger a reload for the entire pool (which is fine I think)

Author: drdrsh

src/config.rs

@@ -4,8 +4,9 @@ use log::{error, info};
 use once_cell::sync::Lazy;
 use regex::Regex;
 use serde_derive::{Deserialize, Serialize};
+use std::collections::hash_map::DefaultHasher;
 use std::collections::{BTreeMap, HashMap, HashSet};
-use std::hash::Hash;
+use std::hash::{Hash, Hasher};
 use std::path::Path;
 use std::sync::Arc;
 use tokio::fs::File;
@@ -355,6 +356,12 @@ pub struct Pool {
 }
 
 impl Pool {
+    pub fn hash_value(&self) -> u64 {
+        let mut s = DefaultHasher::new();
+        self.hash(&mut s);
+        s.finish()
+    }
+
     pub fn default_pool_mode() -> PoolMode {
         PoolMode::Transaction
     }

src/pool.rs

@@ -9,7 +9,7 @@ use parking_lot::{Mutex, RwLock};
 use rand::seq::SliceRandom;
 use rand::thread_rng;
 use regex::Regex;
-use std::collections::{HashMap, HashSet};
+use std::collections::HashMap;
 use std::sync::{
     atomic::{AtomicBool, Ordering},
     Arc,
@@ -37,8 +37,6 @@ pub type PoolMap = HashMap<PoolIdentifier, ConnectionPool>;
 /// This is atomic and safe and read-optimized.
 /// The pool is recreated dynamically when the config is reloaded.
 pub static POOLS: Lazy<ArcSwap<PoolMap>> = Lazy::new(|| ArcSwap::from_pointee(HashMap::default()));
-static POOLS_HASH: Lazy<ArcSwap<HashSet<crate::config::Pool>>> =
-    Lazy::new(|| ArcSwap::from_pointee(HashSet::default()));
 
 /// An identifier for a PgCat pool,
 /// a database visible to clients.
@@ -168,6 +166,11 @@ pub struct ConnectionPool {
     /// to use it.
     validated: Arc<AtomicBool>,
 
+    /// Hash value for the pool configs. It is used to compare new configs
+    /// against current config to decide whether or not we need to recreate
+    /// the pool after a RELOAD command
+    pub config_hash: u64,
+
     /// If the pool has been paused or not.
     paused: Arc<AtomicBool>,
     paused_waiter: Arc<Notify>,
@@ -181,18 +184,18 @@ impl ConnectionPool {
         let mut new_pools = HashMap::new();
         let mut address_id = 0;
 
-        let mut pools_hash = (*(*POOLS_HASH.load())).clone();
-
         for (pool_name, pool_config) in &config.pools {
-            let changed = pools_hash.insert(pool_config.clone());
+            let new_pool_hash_value = pool_config.hash_value();
 
             // There is one pool per database/user pair.
             for user in pool_config.users.values() {
-                // If the pool hasn't changed, get existing reference and insert it into the new_pools.
-                // We replace all pools at the end, but if the reference is kept, the pool won't get re-created (bb8).
-                if !changed {
-                    match get_pool(pool_name, &user.username) {
-                        Some(pool) => {
+                let old_pool_ref = get_pool(pool_name, &user.username);
+
+                match old_pool_ref {
+                    Some(pool) => {
+                        // If the pool hasn't changed, get existing reference and insert it into the new_pools.
+                        // We replace all pools at the end, but if the reference is kept, the pool won't get re-created (bb8).
+                        if pool.config_hash == new_pool_hash_value {
                             info!(
                                 "[pool: {}][user: {}] has not changed",
                                 pool_name, user.username
@@ -203,8 +206,8 @@ impl ConnectionPool {
                             );
                             continue;
                         }
-                        None => (),
                     }
+                    None => (),
                 }
 
                 info!(
@@ -293,6 +296,7 @@ impl ConnectionPool {
                     addresses,
                     banlist: Arc::new(RwLock::new(banlist)),
                     stats: get_reporter(),
+                    config_hash: new_pool_hash_value,
                     server_info: Arc::new(RwLock::new(BytesMut::new())),
                     settings: PoolSettings {
                         pool_mode: pool_config.pool_mode,
@@ -342,8 +346,6 @@ impl ConnectionPool {
         }
 
         POOLS.store(Arc::new(new_pools.clone()));
-        POOLS_HASH.store(Arc::new(pools_hash.clone()));
-
         Ok(())
     }
 

tests/ruby/helpers/pgcat_process.rb

@@ -53,7 +53,7 @@ def current_config
 
   def reload_config
     `kill -s HUP #{@pid}`
-    sleep 0.1
+    sleep 0.5
   end
 
   def start

tests/ruby/load_balancing_spec.rb

@@ -150,7 +150,7 @@
         end
       end
 
-      expect(failed_count).to eq(2)
+      expect(failed_count).to be <= 2
       processes.all_databases.each do |instance|
         queries_routed = instance.count_select_1_plus_2
         if processes.replicas[0..1].include?(instance)

tests/ruby/misc_spec.rb

@@ -8,6 +8,55 @@
     processes.pgcat.shutdown
   end
 
+  context "when adding then removing instance using RELOAD" do
+    it "works correctly" do
+      admin_conn = PG::connect(processes.pgcat.admin_connection_string)
+
+      current_configs = processes.pgcat.current_config
+      correct_count = current_configs["pools"]["sharded_db"]["shards"]["0"]["servers"].count
+      expect(admin_conn.async_exec("SHOW DATABASES").count).to eq(correct_count)
+
+      extra_replica = current_configs["pools"]["sharded_db"]["shards"]["0"]["servers"].last.clone
+      extra_replica[0] = "127.0.0.1"
+      current_configs["pools"]["sharded_db"]["shards"]["0"]["servers"] << extra_replica
+
+      processes.pgcat.update_config(current_configs) # with replica added
+      processes.pgcat.reload_config
+      correct_count = current_configs["pools"]["sharded_db"]["shards"]["0"]["servers"].count
+      expect(admin_conn.async_exec("SHOW DATABASES").count).to eq(correct_count)
+
+      current_configs["pools"]["sharded_db"]["shards"]["0"]["servers"].pop
+
+      processes.pgcat.update_config(current_configs) # with replica removed again
+      processes.pgcat.reload_config
+      correct_count = current_configs["pools"]["sharded_db"]["shards"]["0"]["servers"].count
+      expect(admin_conn.async_exec("SHOW DATABASES").count).to eq(correct_count)
+    end
+  end
+
+  context "when removing then adding instance back using RELOAD" do
+    it "works correctly" do
+      admin_conn = PG::connect(processes.pgcat.admin_connection_string)
+
+      current_configs = processes.pgcat.current_config
+      correct_count = current_configs["pools"]["sharded_db"]["shards"]["0"]["servers"].count
+      expect(admin_conn.async_exec("SHOW DATABASES").count).to eq(correct_count)
+
+      removed_replica = current_configs["pools"]["sharded_db"]["shards"]["0"]["servers"].pop
+      processes.pgcat.update_config(current_configs) # with replica removed
+      processes.pgcat.reload_config
+      correct_count = current_configs["pools"]["sharded_db"]["shards"]["0"]["servers"].count
+      expect(admin_conn.async_exec("SHOW DATABASES").count).to eq(correct_count)
+
+      current_configs["pools"]["sharded_db"]["shards"]["0"]["servers"] << removed_replica
+
+      processes.pgcat.update_config(current_configs) # with replica added again
+      processes.pgcat.reload_config
+      correct_count = current_configs["pools"]["sharded_db"]["shards"]["0"]["servers"].count
+      expect(admin_conn.async_exec("SHOW DATABASES").count).to eq(correct_count)
+    end
+  end
+
   describe "TCP Keepalives" do
     # Ideally, we should block TCP traffic to the database using
     # iptables to mimic passive (connection is dropped without a RST packet)