Re: Proposal: Recent mutated table tracking in memory

Hi Tatsuo,

Thanks for the review. All addressed in v7:

1. Message format strings split across lines
Restored to single-line strings in is_select_object_in_temp_write_list()
and every other place in the patch (pool_query_context.c and
pool_track_table_mutation.c). Long lines are left long; only the
message strings were rejoined.

2. The large "else if" branch in where_to_send_main_replica()
Moved into a new static subroutine,
where_to_send_dml_adaptive_global(), so the routing function stays
readable. The call site is now just:

else if (pool_config->disable_load_balance_on_write ==
DLBOW_DML_ADAPTIVE_GLOBAL)
{
where_to_send_dml_adaptive_global(query_context, query, node,
dest);
}

3. Copyright year on the two new files
Changed to "Copyright (c) 2026" in pool_track_table_mutation.c and
pool_track_table_mutation.h.

4. FNV-1a comment
Expanded with a short explanation of what FNV-1a is (offset basis +
per-byte XOR/multiply by the FNV prime, non-cryptographic, good
dispersion for short keys) and a reference link.

5. get_current_time() wrapper
Good point -- there was no reason for it. Removed the wrapper and call
gettimeofday() directly at the call sites.

Attaching the updated single patch:

v7-0001-Feature-load-balancing-control-by-table-tracking.patch

Builds cleanly and passes the dml_adaptive_global routing tests on a
PG16 streaming-replication cluster.

Thanks!

On Sat, Jun 27, 2026 at 4:51 AM Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:

> 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
>

--
Nadav Shatz
Tailor Brands | CTO