public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nadav Shatz <[email protected]>
To: Tatsuo Ishii <[email protected]>
Cc: [email protected]
Subject: Re: Proposal: Recent mutated table tracking in memory
Date: Sun, 28 Jun 2026 07:33:32 +0300
Message-ID: <CACeKOO0xkDuZJ1jQiqbh-qmxsdCWODkR5X7d+5FvNAkY7t964g@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CACeKOO0aG2qyMPmeRpVxSR92YnF+i0iRk3to=KqJ_3xG6PBOmg@mail.gmail.com>
<[email protected]>
<CACeKOO27Qr1CeL1Vqo1SWEdhsGhFSaRca4_S7CXfyErcLpzcpg@mail.gmail.com>
<[email protected]>
So happy to hear it!
Thank you for all the help in getting it ready !
Nadav Shatz
Tailor Brands | CTO
On Sun, Jun 28, 2026 at 4:03 AM Tatsuo Ishii <[email protected]> wrote:
> Hi Nadav,
>
> Thanks for the updated patch.
>
> I made subtle changes to the patch (and along with Japanese docs) and
> pushed it.
>
> - Fix indentation in loadbalance.sgml.
>
> - Add a limitation of the feature to the doc to avoid confusion from
> users who want to use the feature and
> dml_adaptive_object_relationship_list together.
>
> <para>
> Note, however, you cannot
> use <xref linkend="guc-dml-adaptive-object-relationship-list"> to
> track
> dependency among table and other objects
> if <xref linkend="guc-disable-load-balance-on-write"> is set
> to <varname>dml_adaptive_global</varname>.
> </para>
>
> - Modify handle_query_context() to use foreach_oid, rather than plain
> foreach to simplify the code a little bit.
>
> Thank you so much to add the excellent feature to Pgpool-II!
> --
> Tatsuo Ishii
> SRA OSS K.K.
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp
>
> > 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 <[email protected]>
> 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
>
view thread (61+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Proposal: Recent mutated table tracking in memory
In-Reply-To: <CACeKOO0xkDuZJ1jQiqbh-qmxsdCWODkR5X7d+5FvNAkY7t964g@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox