Date: Thu, 02 Jul 2026 22:45:17 +0900 (JST)
Message-Id: <20260702.224517.1668373716426906512.ishii@postgresql.org>
To: emond.papegaaij@gmail.com
Cc: pgpool-hackers@lists.postgresql.org
Subject: Re: [PATCH] Segfault in pool_do_auth() when failover races with a
 new connection
From: Tatsuo Ishii <ishii@postgresql.org>
In-Reply-To: 
 <CAGXsc+aizSg6Z22=pV8NsFb-FukzT2XP1YriOkOJjOBLVdN-vg@mail.gmail.com>
References: 
 <CAGXsc+Y7OKp93zSqt1WUCh7ABC7kyZqrxJPFA8AC1woB30S8EQ@mail.gmail.com>
	<20260702.163922.30147080599128209.ishii@postgresql.org>
	<CAGXsc+aizSg6Z22=pV8NsFb-FukzT2XP1YriOkOJjOBLVdN-vg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Thu_Jul__2_22_45_17_2026_713)--"
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20260702.224517.1668373716426906512.ishii%40postgresql.org>
Precedence: bulk

----Next_Part(Thu_Jul__2_22_45_17_2026_713)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Hi Emond,

> The new patch looks fine to me. There's however one thing Claude
> pointed out. According to it, my_main_node_id,
> private_backend_status[], and my_backend_status[] are per-process
> globals (copied on fork), not shared memory. So, making any of those
> volatile should not make any difference.

You are right. Attached is the v3 patch.

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

----Next_Part(Thu_Jul__2_22_45_17_2026_713)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="connect_crash_fix_v3.patch"

diff --git a/src/protocol/child.c b/src/protocol/child.c
index 4a527c84c..a4ec13dfd 100644
--- a/src/protocol/child.c
+++ b/src/protocol/child.c
@@ -1478,12 +1478,26 @@ pool_initialize_private_backend_status(void)
 
 	for (i = 0; i < MAX_NUM_BACKENDS; i++)
 	{
-		private_backend_status[i] = BACKEND_INFO(i).backend_status;
+		private_backend_status[i] =
+			*(volatile BACKEND_STATUS *) &BACKEND_INFO(i).backend_status;
 		/* my_backend_status is referred to by VALID_BACKEND macro. */
 		my_backend_status[i] = &private_backend_status[i];
 	}
 
-	my_main_node_id = REAL_MAIN_NODE_ID;
+ 	my_main_node_id = *(volatile int *) &REAL_MAIN_NODE_ID;
+
+	/*
+	 * REAL_MAIN_NODE_ID and the per-node status are read from shared memory
+	 * non-atomically, so a concurrent failover can leave my_main_node_id
+	 * pointing at a node we just captured as down. Re-point it at the first
+	 * node that is up in our private snapshot; otherwise MAIN_CONNECTION()
+	 * would dereference a connection slot that is never created for a down
+	 * node.
+	 */
+	if (my_main_node_id < 0 || my_main_node_id >= NUM_BACKENDS ||
+		(private_backend_status[my_main_node_id] != CON_UP &&
+		 private_backend_status[my_main_node_id] != CON_CONNECT_WAIT))
+		my_main_node_id = get_next_main_node();
 }
 
 static void
diff --git a/src/protocol/pool_connection_pool.c b/src/protocol/pool_connection_pool.c
index b16ccc39e..a939b08a7 100644
--- a/src/protocol/pool_connection_pool.c
+++ b/src/protocol/pool_connection_pool.c
@@ -1035,6 +1035,26 @@ new_connection(POOL_CONNECTION_POOL *p)
 
 	if (active_backend_count > 0)
 	{
+		/*
+		 * A concurrent failover may have changed backend status while we were
+		 * creating connections, leaving my_main_node_id pointing at a node we
+		 * skipped (its slot is NULL). Re-point it at a node we did connect to
+		 * so that MAIN_CONNECTION() never dereferences a NULL slot (e.g. in
+		 * pool_do_auth()).
+		 */
+		if (my_main_node_id < 0 || my_main_node_id >= NUM_BACKENDS ||
+			p->slots[my_main_node_id] == NULL)
+		{
+			for (i = 0; i < NUM_BACKENDS; i++)
+			{
+				if (p->slots[i] != NULL)
+				{
+					my_main_node_id = i;
+					break;
+				}
+			}
+		}
+
 		return p;
 	}
 

----Next_Part(Thu_Jul__2_22_45_17_2026_713)----