public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tatsuo Ishii <[email protected]>
To: [email protected]
Cc: [email protected]
Subject: Re: Proposal: Recent mutated table tracking in memory
Date: Sun, 28 Jun 2026 10:03:41 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACeKOO27Qr1CeL1Vqo1SWEdhsGhFSaRca4_S7CXfyErcLpzcpg@mail.gmail.com>
References: <CACeKOO0aG2qyMPmeRpVxSR92YnF+i0iRk3to=KqJ_3xG6PBOmg@mail.gmail.com>
<[email protected]>
<CACeKOO27Qr1CeL1Vqo1SWEdhsGhFSaRca4_S7CXfyErcLpzcpg@mail.gmail.com>
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) latest in thread
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: <[email protected]>
* 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