Date: Sat, 27 Jun 2026 10:51:08 +0900 (JST)
Message-Id: <20260627.105108.665094997299401916.ishii@postgresql.org>
To: nadav@tailorbrands.com
Cc: pgpool-hackers@lists.postgresql.org
Subject: Re: Proposal: Recent mutated table tracking in memory
From: Tatsuo Ishii <ishii@postgresql.org>
In-Reply-To: 
 <CACeKOO0aG2qyMPmeRpVxSR92YnF+i0iRk3to=KqJ_3xG6PBOmg@mail.gmail.com>
References: 
 <CACeKOO31reCtDYU1twicmqgdEStCJXWJ6P4LMFMYytaYaGew-A@mail.gmail.com>
	<20260626.074028.806271918741516663.ishii@postgresql.org>
	<CACeKOO0aG2qyMPmeRpVxSR92YnF+i0iRk3to=KqJ_3xG6PBOmg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20260627.105108.665094997299401916.ishii%40postgresql.org>
Precedence: bulk

Hi Nadav,

> Hi Tatsuo,
> 
> Sorry -- my wording was misleading. No, unpatched pgpool does NOT have
> this problem, and you're right that your test shows no hang on master.
> The hang is entirely in the new code my patch adds, not in pgpool
> itself.
> 
> To be precise: the marking code that hung is the dml_adaptive_global
> branch in handle_query_context() (CommandComplete.c) that my patch
> introduces. It is gated on disable_load_balance_on_write =
> dml_adaptive_global, which only exists with the patch applied, so master
> never runs it.
> 
> It also doesn't fire for your exact sequence, for two reasons:
> 
>   1. You ran it on master, which doesn't contain the branch at all.
>   2. Even on a patched build, your statements are inside an explicit
>      transaction (BEGIN ... COMMIT), and that branch only runs for
>      autocommit statements (the "!is_in_transaction" case).
> 
> What actually hung, on a patched build with dml_adaptive_global set in
> streaming replication, was a *bare autocommit* statement (no surrounding
> transaction) whose table OID cannot be served from the relcache -- for
> example:
> 
>   DROP TABLE IF EXISTS non_existent_table;     -- no BEGIN/COMMIT
> 
> Because the name is unresolvable, pool_table_name_to_oid() does not cache
> it (no_cache_if_zero), so every lookup is a relcache miss. The marking
> branch calls pool_extract_table_oids() -> ... -> do_query() to resolve
> the OID, but handle_query_context() runs before the statement's
> CommandComplete response has been read from the backend, so injecting
> that query desyncs the protocol and the session blocks. Backtrace:
> 
>   #2  pool_read2                (blocked in read())
>   #3  do_query                  "SELECT
> COALESCE(pg_catalog.to_regclass('...')::oid, 0)"
>   #4  pool_search_relcache      (relcache miss)
>   #5  pool_table_name_to_oid
>   #6  pool_extract_table_oids   query_cache/pool_memqcache.c
>   #7  handle_query_context      protocol/CommandComplete.c
>  (!is_in_transaction branch)
>   #8  CommandComplete
> 
> This is the same class of issue as the one behind your original review
> comment: resolving OIDs at CommandComplete time means doing backend I/O
> while a response is in flight.
> 
> v6-0001 fixes it for good by resolving the written tables' OIDs (and the
> database OID) at DML *routing* time -- when the connection is idle and
> do_query is safe, the same point the feature already resolves OIDs for
> the SELECT staleness check. handle_query_context() then only marks the
> already-resolved OIDs and does no backend I/O at all, for both the
> autocommit and the COMMIT paths.
> 
> Verified on a PG16 streaming-replication cluster with dml_adaptive_global:
> the bare "DROP TABLE IF EXISTS non_existent_table;" and your
> "BEGIN; DROP TABLE IF EXISTS ...; COMMIT;" both return promptly, the
> deferred-constraint COMMIT failure leaves the table unmarked (reads keep
> load-balancing to the standby), and a successful autocommit or COMMIT
> write marks the table and forces reads to the primary.

Understood.

> Sorry again for the confusing earlier phrasing.

No problem. Thank you for detailed explanation!

So, my review comments are below. Mostly cosmetic coding style
suggestions.

diff --git a/src/context/pool_query_context.c b/src/context/pool_query_context.c
index 73bc64338..a7756ec8b 100644
--- a/src/context/pool_query_context.c
+++ b/src/context/pool_query_context.c
@@ -1848,20 +1849,26 @@ is_in_list(char *name, List *list)
 static bool
 is_select_object_in_temp_write_list(Node *node, void *context)
 {
-	if (node == NULL || pool_config->disable_load_balance_on_write != DLBOW_DML_ADAPTIVE)
+	if (node == NULL ||
+		!DLBOW_IS_DML_ADAPTIVE(pool_config->disable_load_balance_on_write))
 		return false;
 
 	if (IsA(node, RangeVar))
 	{
 		RangeVar   *rgv = (RangeVar *) node;
-		POOL_SESSION_CONTEXT *session_context = pool_get_session_context(false);
+		POOL_SESSION_CONTEXT *session_context;
 
-		if (pool_config->disable_load_balance_on_write == DLBOW_DML_ADAPTIVE && session_context->is_in_transaction)
+		session_context = pool_get_session_context(false);
+
+		if (session_context->is_in_transaction)
 		{
 			ereport(DEBUG1,
-					(errmsg("is_select_object_in_temp_write_list: \"%s\", found relation \"%s\"", (char *) context, rgv->relname)));
+					(errmsg("is_select_object_in_temp_write_list:"
+							" \"%s\", found relation \"%s\"",
+							(char *) context, rgv->relname)));

I guess you aimed to shorten the message so that it does not exceed
line length limit (78). But IMO the rule is not applied to message
format strings because this kind of style (divide the message line
into multiple lines) is hard to read. I suggest to restore to former style.

Same suggestion is applied to all other similar places.

@@ -2169,6 +2275,153 @@ where_to_send_main_replica(POOL_QUERY_CONTEXT *query_context, char *query, Node
 				{
 					pool_set_node_to_be_sent(query_context, PRIMARY_NODE_ID);
 				}
+
+				/*
+				 * Check track table mutation for recently written tables.  If
+				 * in cold start or any table was recently written, route to
+				 * primary to avoid stale reads.
+				 */
+				else if (pool_config->disable_load_balance_on_write ==
+						 DLBOW_DML_ADAPTIVE_GLOBAL)
+				{

This "else if" branch is too large and hard to read. You should move
the code of the branch to a subroutine and call it from here.

diff --git a/src/include/utils/pool_track_table_mutation.h b/src/include/utils/pool_track_table_mutation.h
new file mode 100644
index 000000000..dfbac666d
--- /dev/null
+++ b/src/include/utils/pool_track_table_mutation.h
@@ -0,0 +1,167 @@
+/* -*-pgsql-c-*- */
+/*
+ * pgpool: a language independent connection pool server for PostgreSQL
+ * written by Tatsuo Ishii
+ *
+ * Copyright (c) 2003-2026	PgPool Global Development Group

Since this file is new, the Copyright year should be just 2026.
+ * Copyright (c) 2026	PgPool Global Development Group

diff --git a/src/protocol/CommandComplete.c b/src/protocol/CommandComplete.c
index 1f63a0e8d..76e77fea0 100644
--- a/src/protocol/CommandComplete.c
+++ b/src/protocol/CommandComplete.c
diff --git a/src/utils/pool_track_table_mutation.c b/src/utils/pool_track_table_mutation.c
new file mode 100644
index 000000000..e7771e7bf
--- /dev/null
+++ b/src/utils/pool_track_table_mutation.c
@@ -0,0 +1,902 @@
+/* -*-pgsql-c-*- */
+/*
+ * pgpool: a language independent connection pool server for PostgreSQL
+ * written by Tatsuo Ishii
+ *
+ * Copyright (c) 2003-2026	PgPool Global Development Group

Since this file is new, the Copyright year should be just 2026.
+ * Copyright (c) 2026	PgPool Global Development Group

+/* ----------------
+ * Hash functions
+ * ----------------
+ */
+
+/*
+ * FNV-1a hash for table/database oid pair
+ */

Can you please add more comments regarding FNV-1a? I'm not sure
everyone is famimilar with FNV-1a (at lease I am not).

+/*
+ * Get current time
+ */
+static void
+get_current_time(struct timeval *tv)
+{
+	gettimeofday(tv, NULL);
+}

Just out of curiosity, why you don't call gettimeofday directly?

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