Improve and fix connection timeout and total retry timeout.

Previously, when a connection timeout occurred with the default connection
timeout set to pgconnect_timeout (10 seconds), there were no retries. This
was because the total retry timeout (maxT) passed to `pgsql_set_retry_policy`
in `pgsql_set_interactive_retry_policy` was also set to pgconnect_timeout,
which had already elapsed during the initial connection attempt.

To address this issue, the maxT value has been updated to
POSTGRES_PING_RETRY_TIMEOUT. This change also means that maxT cannot be
configured by altering PGCONNECT_TIMEOUT. If one wants to configure maxT
they can change the values in defaults.h or we can make it configurable
through some environment variable in future.

After all this change `pgconnect_timeout` seems redundant so removed it.

src/bin/pgcopydb/cli_root.h

@@ -12,7 +12,6 @@
 
 extern char pgcopydb_argv0[];
 extern char pgcopydb_program[];
-extern int pgconnect_timeout;
 extern int logLevel;
 
 extern Semaphore log_semaphore;

src/bin/pgcopydb/defaults.h

@@ -50,9 +50,8 @@
 
 #define POSTGRES_CONNECT_TIMEOUT "10"
 
-
-/* retry PQping for a maximum of 15 mins, up to 2 secs between attemps */
-#define POSTGRES_PING_RETRY_TIMEOUT 900               /* seconds */
+/* retry PQping for a maximum of 1 min, up to 2 secs between attemps */
+#define POSTGRES_PING_RETRY_TIMEOUT 60               /* seconds */
 #define POSTGRES_PING_RETRY_CAP_SLEEP_TIME (2 * 1000) /* milliseconds */
 #define POSTGRES_PING_RETRY_BASE_SLEEP_TIME 5         /* milliseconds */
 

src/bin/pgcopydb/main.c

@@ -25,7 +25,6 @@
 
 char pgcopydb_argv0[MAXPGPATH];
 char pgcopydb_program[MAXPGPATH];
-int pgconnect_timeout = 10;     /* see also POSTGRES_CONNECT_TIMEOUT */
 
 char *ps_buffer;                /* will point to argv area */
 size_t ps_buffer_size;          /* space determined at run time */
@@ -85,30 +84,6 @@ main(int argc, char **argv)
 		command = root_with_debug;
 	}
 
-	/*
-	 * When PGCONNECT_TIMEOUT is set in the environment, keep a copy of it in
-	 * our own global variable pgconnect_timeout. We implement our own
-	 * connection retry policy and will change change the environment variable
-	 * setting when calling pg_basebackup and other tools anyway.
-	 */
-	if (env_exists("PGCONNECT_TIMEOUT"))
-	{
-		char env_pgtimeout[BUFSIZE] = { 0 };
-
-		if (get_env_copy("PGCONNECT_TIMEOUT", env_pgtimeout, BUFSIZE) > 0)
-		{
-			if (!stringToInt(env_pgtimeout, &pgconnect_timeout))
-			{
-				log_warn("Failed to parse environment variable "
-						 "PGCONNECT_TIMEOUT value \"%s\" as a "
-						 "number of seconds (integer), "
-						 "using our default %d seconds instead",
-						 env_pgtimeout,
-						 pgconnect_timeout);
-			}
-		}
-	}
-
 	/*
 	 * We need to follow POSIX specifications for argument parsing, in
 	 * particular we want getopt() to stop as soon as it reaches a non option

src/bin/pgcopydb/pgcmd.c

@@ -15,6 +15,7 @@
 #include "postgres_fe.h"
 #include "pqexpbuffer.h"
 
+#include "cli_root.h"
 #include "defaults.h"
 #include "env_utils.h"
 #include "file_utils.h"
@@ -354,7 +355,10 @@ pg_dump_db(PostgresPaths *pgPaths,
 	char *PGPASSWORD = NULL;
 	bool pgpassword_found_in_env = env_exists("PGPASSWORD");
 
-	setenv("PGCONNECT_TIMEOUT", POSTGRES_CONNECT_TIMEOUT, 1);
+	if (!env_exists("PGCONNECT_TIMEOUT"))
+	{
+		setenv("PGCONNECT_TIMEOUT", POSTGRES_CONNECT_TIMEOUT, 1);
+	}
 
 	/* override PGPASSWORD environment variable if the pguri contains one */
 	if (connStrings->safeSourcePGURI.password != NULL)
@@ -508,7 +512,10 @@ pg_dumpall_roles(PostgresPaths *pgPaths,
 	char *PGPASSWORD = NULL;
 	bool pgpassword_found_in_env = env_exists("PGPASSWORD");
 
-	setenv("PGCONNECT_TIMEOUT", POSTGRES_CONNECT_TIMEOUT, 1);
+	if (!env_exists("PGCONNECT_TIMEOUT"))
+	{
+		setenv("PGCONNECT_TIMEOUT", POSTGRES_CONNECT_TIMEOUT, 1);
+	}
 
 	/* override PGPASSWORD environment variable if the pguri contains one */
 	if (connStrings->safeSourcePGURI.password != NULL)
@@ -592,7 +599,10 @@ pg_restore_roles(PostgresPaths *pgPaths,
 	char *content = NULL;
 	long size = 0L;
 
-	setenv("PGCONNECT_TIMEOUT", POSTGRES_CONNECT_TIMEOUT, 1);
+	if (!env_exists("PGCONNECT_TIMEOUT"))
+	{
+		setenv("PGCONNECT_TIMEOUT", POSTGRES_CONNECT_TIMEOUT, 1);
+	}
 
 	/*
 	 * Rather than using psql --single-transaction --file filename, we read the
@@ -790,7 +800,10 @@ pg_restore_db(PostgresPaths *pgPaths,
 	char *PGPASSWORD = NULL;
 	bool pgpassword_found_in_env = env_exists("PGPASSWORD");
 
-	setenv("PGCONNECT_TIMEOUT", POSTGRES_CONNECT_TIMEOUT, 1);
+	if (!env_exists("PGCONNECT_TIMEOUT"))
+	{
+		setenv("PGCONNECT_TIMEOUT", POSTGRES_CONNECT_TIMEOUT, 1);
+	}
 
 	/* override PGPASSWORD environment variable if the pguri contains one */
 	if (connStrings->safeTargetPGURI.password != NULL)

src/bin/pgcopydb/pgsql.c

@@ -218,33 +218,15 @@ pgsql_set_retry_policy(ConnectionRetryPolicy *retryPolicy,
 
 
 /*
- * pgsql_set_default_retry_policy sets the default retry policy: no retry. We
- * use the other default parameters but with a maxR of zero they don't get
- * used.
- *
- * This is the retry policy that prevails in the main keeper loop.
- */
-void
-pgsql_set_main_loop_retry_policy(ConnectionRetryPolicy *retryPolicy)
-{
-	(void) pgsql_set_retry_policy(retryPolicy,
-								  POSTGRES_PING_RETRY_TIMEOUT,
-								  0, /* do not retry by default */
-								  POSTGRES_PING_RETRY_CAP_SLEEP_TIME,
-								  POSTGRES_PING_RETRY_BASE_SLEEP_TIME);
-}
-
-
-/*
- * pgsql_set_interactive_retry_policy sets the retry policy to 2 seconds of
- * total retrying time (or PGCONNECT_TIMEOUT when that's set), unbounded number
- * of attempts, and up to 2 seconds of sleep time in between attempts.
+ * pgsql_set_interactive_retry_policy sets the retry policy to 1 minute of
+ * total retrying time, unbounded number of attempts, and up to 2 seconds
+ * of sleep time in between attempts.
  */
 void
 pgsql_set_interactive_retry_policy(ConnectionRetryPolicy *retryPolicy)
 {
 	(void) pgsql_set_retry_policy(retryPolicy,
-								  pgconnect_timeout,
+								  POSTGRES_PING_RETRY_TIMEOUT,
 								  -1, /* unbounded number of attempts */
 								  POSTGRES_PING_RETRY_CAP_SLEEP_TIME,
 								  POSTGRES_PING_RETRY_BASE_SLEEP_TIME);
@@ -517,7 +499,10 @@ pgsql_open_connection(PGSQL *pgsql)
 	}
 
 	/* we implement our own retry strategy */
-	setenv("PGCONNECT_TIMEOUT", POSTGRES_CONNECT_TIMEOUT, 1);
+	if (!env_exists("PGCONNECT_TIMEOUT"))
+	{
+		setenv("PGCONNECT_TIMEOUT", POSTGRES_CONNECT_TIMEOUT, 1);
+	}
 
 	/* register our starting time */
 	INSTR_TIME_SET_CURRENT(pgsql->retryPolicy.startTime);

src/bin/pgcopydb/pgsql.h

@@ -242,7 +242,6 @@ void pgsql_set_retry_policy(ConnectionRetryPolicy *retryPolicy,
 							int maxR,
 							int maxSleepTime,
 							int baseSleepTime);
-void pgsql_set_main_loop_retry_policy(ConnectionRetryPolicy *retryPolicy);
 void pgsql_set_interactive_retry_policy(ConnectionRetryPolicy *retryPolicy);
 int pgsql_compute_connection_retry_sleep_time(ConnectionRetryPolicy *retryPolicy);
 bool pgsql_retry_policy_expired(ConnectionRetryPolicy *retryPolicy);