From: Tatsuo Ishii <ishii@sraoss.co.jp>
Date: Tue, 1 Sep 2020 03:40:18 +0000 (+0900)
Subject: Fix connection_life_time not working when serialize_accept is enabled.
X-Git-Tag: V3_6_23~7
X-Git-Url: http://git.postgresql.org/gitweb/static/gitweb.js?a=commitdiff_plain;h=87910d60fc352cb67eaa4d5ff056954226e56466;p=pgpool2.git

Fix connection_life_time not working when serialize_accept is enabled.

If serialize_accept is enabled, pgpool child process tries to acquire
semaphore locking so that there's only one process which can issue
accept(2). Unfortunately if connection_life_time is enabled, an alarm
is set right before the semaphore locking. So when the alarm fires,
nothing happened because the process was in acquiring semaphore lock
loop by using pool_semaphore_lock().

To fix this new pool_semaphore_lock_allow_interrupt() is introduced,
which immediately returns if interrupted by a signal. The caller:
wait_for_new_connections() checks whether connection_life_time alarm
is fired. If so, call backend_timer() to close expired backend
connection cache.

Discussion: https://www.pgpool.net/pipermail/pgpool-general/2020-August/007233.html
---

diff --git a/src/include/pool.h b/src/include/pool.h
index 3a07a1180..0595e5dca 100644
--- a/src/include/pool.h
+++ b/src/include/pool.h
@@ -628,6 +628,7 @@ extern void pool_shmem_exit(int code);
 
 extern void pool_semaphore_create(int numSems);
 extern void pool_semaphore_lock(int semNum);
+extern int	pool_semaphore_lock_allow_interrupt(int semNum);
 extern void pool_semaphore_unlock(int semNum);
 
 extern BackendInfo *pool_get_node_info(int node_number);
diff --git a/src/protocol/child.c b/src/protocol/child.c
index 096ffd407..8c143e24b 100644
--- a/src/protocol/child.c
+++ b/src/protocol/child.c
@@ -1981,7 +1981,50 @@ wait_for_new_connections(int *fds, struct timeval *timeout, SockAddr *saddr)
 	 */
 	if (SERIALIZE_ACCEPT)
 	{
-		pool_semaphore_lock(ACCEPT_FD_SEM);
+		if (pool_config->connection_life_time == 0)
+		{
+			pool_semaphore_lock(ACCEPT_FD_SEM);
+		}
+		else
+		{
+			int sts;
+
+			for (;;)
+			{
+				sts = pool_semaphore_lock_allow_interrupt(ACCEPT_FD_SEM);
+
+				/* Interrupted by alarm */
+				if (sts == -2)
+				{
+					/*
+					 * Check if there are expired connection_life_time.
+					 */
+					if (backend_timer_expired)
+					{
+						/*
+						 * We add 10 seconds to connection_life_time so that there's
+						 * enough margin.
+						 */
+						int	seconds = pool_config->connection_life_time + 10;
+
+						while (seconds-- > 0)
+						{
+							/* check backend timer is expired */
+							if (backend_timer_expired)
+							{
+								pool_backend_timer();
+								backend_timer_expired = 0;
+								break;
+							}
+							sleep(1);
+						}
+					}
+				}
+				else	/* success or other error */
+					break;
+			}
+		}
+
 		set_ps_display("wait for connection request", false);
 		ereport(DEBUG1,
 			   (errmsg("LOCKING select()")));
diff --git a/src/protocol/pool_connection_pool.c b/src/protocol/pool_connection_pool.c
index fa93eb96e..979f2fcd0 100644
--- a/src/protocol/pool_connection_pool.c
+++ b/src/protocol/pool_connection_pool.c
@@ -384,7 +384,7 @@ void pool_backend_timer(void)
 	now = time(NULL);
 
 	ereport(DEBUG1,
-		(errmsg("backend timer handler called at%ld", now)));
+			(errmsg("backend timer handler called at %ld", now)));
 
 	for (i=0;i<pool_config->max_pool;i++, p++)
 	{
diff --git a/src/utils/pool_sema.c b/src/utils/pool_sema.c
index abe4b55a4..befc63857 100644
--- a/src/utils/pool_sema.c
+++ b/src/utils/pool_sema.c
@@ -129,6 +129,48 @@ pool_semaphore_lock(int semNum)
 			(errmsg("failed to lock semaphore error:\"%s\"",strerror(errno))));
 }
 
+/*
+ * Lock a semaphore (decrement count), blocking if count would be < 0.
+ * Unlike pool_semaphore_lock, this returns if interrupted.
+ * Return values:
+ * 0: succeeded in acquiring lock.
+ * -1: error.
+ * -2: interrupted.
+ */
+int
+pool_semaphore_lock_allow_interrupt(int semNum)
+{
+	int			errStatus;
+	struct sembuf sops;
+
+	sops.sem_op = -1;			/* decrement */
+	sops.sem_flg = SEM_UNDO;
+	sops.sem_num = semNum;
+
+	/*
+	 * Note: if errStatus is -1 and errno == EINTR then it means we returned
+	 * from the operation prematurely because we were sent a signal.
+	 */
+	errStatus = semop(semId, &sops, 1);
+
+	if (errStatus < 0)
+	{
+		if (errno == EINTR)
+		{
+			ereport(DEBUG1,
+					(errmsg("interrupted while trying to lock semaphore")));
+			return -2;
+		}
+		else
+		{
+			ereport(WARNING,
+					(errmsg("failed to lock semaphore error:\"%s\"", strerror(errno))));
+			return -1;
+		}
+	}
+	return 0;
+}
+
 /*
  * Unlock a semaphore (increment count)
  */