Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wdhDQ-004Hgh-1L for pgpool-hackers@arkaria.postgresql.org; Sun, 28 Jun 2026 04:33:56 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wdhDN-00G1Gl-0v for pgpool-hackers@arkaria.postgresql.org; Sun, 28 Jun 2026 04:33:53 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wdhDM-00G1Gd-2E for pgpool-hackers@lists.postgresql.org; Sun, 28 Jun 2026 04:33:52 +0000 Received: from mail-yx1-xb12c.google.com ([2607:f8b0:4864:20::b12c]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wdhDG-00000000YvU-35wB for pgpool-hackers@lists.postgresql.org; Sun, 28 Jun 2026 04:33:51 +0000 Received: by mail-yx1-xb12c.google.com with SMTP id 956f58d0204a3-664a09bc459so2025219d50.1 for ; Sat, 27 Jun 2026 21:33:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1782621225; cv=none; d=google.com; s=arc-20260327; b=BKuF5jp/Zea01bEqYdUzSwHo+FBf0AlCd63FfYpocJ2L9mOIowpGzVisP1DnQhDEtj OWegEcGzMDlTM15Nib4CsZNTY4jjXADJ+E7yMAv2IuV0A9e/o67AP3GIA/yaIAH6SW0T CnBcC6HoQfJg++GVJuGzVJlw6OVVdU6ap57chjZ59/bFhs1aIDPbDEtJ5gf2uTwaowUQ oD7l2EdtZXJkBP4ymPlUc/uFaMgaKOjwzhTKDtT+Oui8akyRGCV+m08ceI3mvaXA0mwa wo28my+Fp1QGapAe2At8CwVMxDOsnWox6iuMADXI56u8NMbeFEviLtdLt0Dbi48xFim+ vJSA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20260327; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:dkim-signature; bh=6Lnl8CoH4kjywPSbuwKlIOPhlBc/WjpnHrL4ZqcH4PY=; fh=m3oiE62fRN5J1XaE+5vpHZRIgCyFefxVI0mFw3d8yxA=; b=Ge8ezeQ2I7OV9y7eehNE1s41AM1mm3feHSq3hnjpBkGwhAVlaI2sZ55svuUKBRdRdJ CTeUwYlIrJ1j0S2BhTKcAf+qkvZ2ZO1++i61qk0IvozkZF0OXAiWYO1l3YpwDFKdfYHH t+LPYSrEegFdATR6WIAaiZtz+fRzZiSbxK8OT9PgJC5eydrqt2M1L4C/wKAi8nxSIAs/ sgPMOJLyV0IoGc5c4/sIDTVSeE7hwlPmoYP8lnldbVeEwxwxpXagClEMer+DPgq63PMx h3odMCvjrn4+cmTLpdtKe4Ifzxp37jkQ4hvbJYsH9KAtQpqTeyTsajbMWBJ9U8bzZ6N6 OIkg==; darn=lists.postgresql.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tailorbrands.com; s=google; t=1782621225; x=1783226025; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=6Lnl8CoH4kjywPSbuwKlIOPhlBc/WjpnHrL4ZqcH4PY=; b=WbjYTkMplweaSv8Hwg5ETnglUr3glHZWjBdRpxBzUdjukdq5jLcBuiNETTowe2Yy2A 7PYOfEV0iHIzWvLLSQEs8Lo5u7PXHHJ7zx1CZKwxVI4HN+YkFyIonfAa903P9lkn1B1M QXvgdLH61Q0oHjvpEWC/lHyE2c138FtqBVOTNDGU+5Gu9REWo/RJCDEtwnrBiAKVTetj bXr+AzyO/OpWeoXFHd5vvYAReZbliNVi+BFY8eICsBpOGRZ6Uvn8uyJ2a4Ofors3WVg4 8424cLmjOchvD2row5GFzdmIbbXQ5EwVkxuqQWTj0/FtiV78brisviUXbkR1lxHVKb+S 3R8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782621225; x=1783226025; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=6Lnl8CoH4kjywPSbuwKlIOPhlBc/WjpnHrL4ZqcH4PY=; b=a0Py2ojdyvwmAcMmfzIUg+GxZyzc8j7BRzdHdfb+qV4iCRsNamAL3Oioi650tBlyTW 5p8QYR4AA2KutqjOv4oZOy+Chudg1aLv+CT3Mb0hoxeNWLU3saBZdwVit8I47dOs+nmD MvMmokx1DJMYcwt9Vh0oNgpLne6nfAJMhwDk3fGFIFwvf0NIQ/VqoVcIXGHnhjS1Qvng jg3D+pzgGrpyhDfa07RQd/fQotVq7wbmuPsq27SvBlFjApfc1PnFBkb7OSW/WUacGicA +bzwwg/2XaBawgEitaJ6wHHOPrm6xbfRgNoWAlaUV4W6Ojtpuj2uocV1LEbH9gnIbdJy oqug== X-Gm-Message-State: AOJu0YynxlC+002BHr9cmTVstlEkfUCuWT2cM5irKf22vSLAOzvZKNh3 mxxmQ3lGI82jXrLmfuHkFuNZI+RN9iTwxVUYGmu8j5pxMHrRQ0gl92muUnUttevbpp575sd6/rw eSjGd9YApkEaeo1RbyfdvVu4PyEQ4H4npfPfB8yUKIA== X-Gm-Gg: AfdE7cnDND75ZCbljSz7xDevY5sV3/ah+b2WqAcdxG9YRur4s6lGYVcO9pU/Nfu8ydU fbRDqG0Yf2WK5mUMbflDFoQILLNgbOz8cT4kki1TNtElyu2QUctDMi1fnxebSDn8+EhXzI5OePa x9lE85wAMxQ0jQr2PDElBWk155V/yWi+N6ashQgVJkhfpZtUD2fvj35rp4eXZXsdi+aSAyWqEEy /6ibuCTfGexRRsTy5g9R45DsEBf+BNvGhJ5J1CdOc89egvvFBUzvK9OQHJav37OEMEJKgKV6FqQ Q12Yu8SY7UpbhaYaREnKlOUtnhEShXUEuDAZkCbRevPRUwnDQ52CuhHKIg8H1g3DvdIIaxVCNnk rndg/EV5GSJl2PysVxdxcR6oxRbbvnCsxLKP95zZkQxsJkClyRPew/Qfys/s= X-Received: by 2002:a05:690e:4390:b0:663:2329:bdfc with SMTP id 956f58d0204a3-66487cb761emr9272503d50.23.1782621224600; Sat, 27 Jun 2026 21:33:44 -0700 (PDT) MIME-Version: 1.0 References: <20260627.105108.665094997299401916.ishii@postgresql.org> <20260628.100341.1340554543853410923.ishii@postgresql.org> In-Reply-To: <20260628.100341.1340554543853410923.ishii@postgresql.org> From: Nadav Shatz Date: Sun, 28 Jun 2026 07:33:32 +0300 X-Gm-Features: AVVi8CdACXf3mpPrg0DDObUq4tIt2H1dqcazOLcPLfK-F7h-39OmSzFDVJR20Ls Message-ID: Subject: Re: Proposal: Recent mutated table tracking in memory To: Tatsuo Ishii Cc: pgpool-hackers@lists.postgresql.org Content-Type: multipart/alternative; boundary="000000000000dcb44a065548d830" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000dcb44a065548d830 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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=E2=80=AFAM Tatsuo Ishii = 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. > > > Note, however, you cannot > use t= o > track > dependency among table and other objects > if is set > to dml_adaptive_global. > > > - 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 =3D=3D > > 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 ca= ll > > 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=E2=80=AFAM Tatsuo Ishii > wrote: > > > >> Hi Nadav, > >> > >> > Hi Tatsuo, > >> > > >> > Sorry -- my wording was misleading. No, unpatched pgpool does NOT ha= ve > >> > this problem, and you're right that your test shows no hang on maste= r. > >> > 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 =3D > >> > 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 -- f= or > >> > 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 marki= ng > >> > branch calls pool_extract_table_oids() -> ... -> do_query() to resol= ve > >> > the OID, but handle_query_context() runs before the statement's > >> > CommandComplete response has been read from the backend, so injectin= g > >> > 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 revi= ew > >> > 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 a= nd > >> > do_query is safe, the same point the feature already resolves OIDs f= or > >> > the SELECT staleness check. handle_query_context() then only marks t= he > >> > 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 COMMI= T > >> > 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 =3D=3D NULL || pool_config->disable_load_balance_on_w= rite > !=3D > >> DLBOW_DML_ADAPTIVE) > >> + if (node =3D=3D NULL || > >> + > >> !DLBOW_IS_DML_ADAPTIVE(pool_config->disable_load_balance_on_write)) > >> return false; > >> > >> if (IsA(node, RangeVar)) > >> { > >> RangeVar *rgv =3D (RangeVar *) node; > >> - POOL_SESSION_CONTEXT *session_context =3D > >> pool_get_session_context(false); > >> + POOL_SESSION_CONTEXT *session_context; > >> > >> - if (pool_config->disable_load_balance_on_write =3D=3D > >> DLBOW_DML_ADAPTIVE && session_context->is_in_transaction) > >> + session_context =3D 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\", foun= d > >> 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 =3D=3D > >> + > 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 PostgreS= QL > >> + * 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 PostgreS= QL > >> + * 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 > --000000000000dcb44a065548d830 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
So happy to hear it!
Thank you for= all the help in getting it ready !


Nadav Shatz
Tailor Brands=C2=A0| CTO

On = Sun, Jun 28, 2026 at 4:03=E2=80=AFAM Tatsuo Ishii <ishii@postgresql.org> 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
=C2=A0 users who want to use the feature and
=C2=A0 dml_adaptive_object_relationship_list together.

=C2=A0 =C2=A0 =C2=A0 <para>
=C2=A0 =C2=A0 =C2=A0 =C2=A0Note, however, you cannot
=C2=A0 =C2=A0 =C2=A0 =C2=A0use <xref linkend=3D"guc-dml-adaptive-ob= ject-relationship-list"> to track
=C2=A0 =C2=A0 =C2=A0 =C2=A0dependency among table and other objects
=C2=A0 =C2=A0 =C2=A0 =C2=A0if <xref linkend=3D"guc-disable-load-bal= ance-on-write"> is set
=C2=A0 =C2=A0 =C2=A0 =C2=A0to <varname>dml_adaptive_global</varnam= e>.
=C2=A0 =C2=A0 =C2=A0</para>

- Modify handle_query_context() to use foreach_oid, rather than plain
=C2=A0 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
>=C2=A0 =C2=A0 Restored to single-line strings in is_select_object_in_te= mp_write_list()
>=C2=A0 =C2=A0 and every other place in the patch (pool_query_context.c = and
>=C2=A0 =C2=A0 pool_track_table_mutation.c). Long lines are left long; o= nly the
>=C2=A0 =C2=A0 message strings were rejoined.
>
> 2. The large "else if" branch in where_to_send_main_replica(= )
>=C2=A0 =C2=A0 Moved into a new static subroutine,
>=C2=A0 =C2=A0 where_to_send_dml_adaptive_global(), so the routing funct= ion stays
>=C2=A0 =C2=A0 readable. The call site is now just:
>
>=C2=A0 =C2=A0 =C2=A0 else if (pool_config->disable_load_balance_on_w= rite =3D=3D
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DLBOW_DML_ADAPTI= VE_GLOBAL)
>=C2=A0 =C2=A0 =C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 where_to_send_dml_adaptive_global(qu= ery_context, query, node,
> dest);
>=C2=A0 =C2=A0 =C2=A0 }
>
> 3. Copyright year on the two new files
>=C2=A0 =C2=A0 Changed to "Copyright (c) 2026" in pool_track_t= able_mutation.c and
>=C2=A0 =C2=A0 pool_track_table_mutation.h.
>
> 4. FNV-1a comment
>=C2=A0 =C2=A0 Expanded with a short explanation of what FNV-1a is (offs= et basis +
>=C2=A0 =C2=A0 per-byte XOR/multiply by the FNV prime, non-cryptographic= , good
>=C2=A0 =C2=A0 dispersion for short keys) and a reference link.
>
> 5. get_current_time() wrapper
>=C2=A0 =C2=A0 Good point -- there was no reason for it. Removed the wra= pper and call
>=C2=A0 =C2=A0 gettimeofday() directly at the call sites.
>
> Attaching the updated single patch:
>
>=C2=A0 =C2=A0v7-0001-Feature-load-balancing-control-by-table-tracking.p= atch
>
> 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=E2=80=AFAM Tatsuo Ishii <ishii@postgresql.org> w= rote:
>
>> 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 ha= ng on master.
>> > The hang is entirely in the new code my patch adds, not in pg= pool
>> > 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 =3D<= br> >> > 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 rea= sons:
>> >
>> >=C2=A0 =C2=A01. You ran it on master, which doesn't contai= n the branch at all.
>> >=C2=A0 =C2=A02. Even on a patched build, your statements are i= nside an explicit
>> >=C2=A0 =C2=A0 =C2=A0 transaction (BEGIN ... COMMIT), and that = branch only runs for
>> >=C2=A0 =C2=A0 =C2=A0 autocommit statements (the "!is_in_t= ransaction" case).
>> >
>> > What actually hung, on a patched build with dml_adaptive_glob= al set in
>> > streaming replication, was a *bare autocommit* statement (no = surrounding
>> > transaction) whose table OID cannot be served from the relcac= he -- for
>> > example:
>> >
>> >=C2=A0 =C2=A0DROP TABLE IF EXISTS non_existent_table;=C2=A0 = =C2=A0 =C2=A0-- no BEGIN/COMMIT
>> >
>> > Because the name is unresolvable, pool_table_name_to_oid() do= es not cache
>> > it (no_cache_if_zero), so every lookup is a relcache miss. Th= e marking
>> > branch calls pool_extract_table_oids() -> ... -> do_que= ry() to resolve
>> > the OID, but handle_query_context() runs before the statement= 's
>> > CommandComplete response has been read from the backend, so i= njecting
>> > that query desyncs the protocol and the session blocks. Backt= race:
>> >
>> >=C2=A0 =C2=A0#2=C2=A0 pool_read2=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 (blocked in read())
>> >=C2=A0 =C2=A0#3=C2=A0 do_query=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "SELECT
>> > COALESCE(pg_catalog.to_regclass('...')::oid, 0)"=
>> >=C2=A0 =C2=A0#4=C2=A0 pool_search_relcache=C2=A0 =C2=A0 =C2=A0= (relcache miss)
>> >=C2=A0 =C2=A0#5=C2=A0 pool_table_name_to_oid
>> >=C2=A0 =C2=A0#6=C2=A0 pool_extract_table_oids=C2=A0 =C2=A0quer= y_cache/pool_memqcache.c
>> >=C2=A0 =C2=A0#7=C2=A0 handle_query_context=C2=A0 =C2=A0 =C2=A0= protocol/CommandComplete.c
>> >=C2=A0 (!is_in_transaction branch)
>> >=C2=A0 =C2=A0#8=C2=A0 CommandComplete
>> >
>> > This is the same class of issue as the one behind your origin= al review
>> > comment: resolving OIDs at CommandComplete time means doing b= ackend 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 bot= h 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 ret= urn promptly, the
>> > deferred-constraint COMMIT failure leaves the table unmarked = (reads keep
>> > load-balancing to the standby), and a successful autocommit o= r 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)
>>=C2=A0 static bool
>>=C2=A0 is_select_object_in_temp_write_list(Node *node, void *contex= t)
>>=C2=A0 {
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0if (node =3D=3D NULL || pool_config-&g= t;disable_load_balance_on_write !=3D
>> DLBOW_DML_ADAPTIVE)
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0if (node =3D=3D NULL ||
>> +
>>=C2=A0 !DLBOW_IS_DML_ADAPTIVE(pool_config->disable_load_balance_= on_write))
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0retur= n false;
>>
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (IsA(node, RangeVar))
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Range= Var=C2=A0 =C2=A0*rgv =3D (RangeVar *) node;
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0POOL_SESSI= ON_CONTEXT *session_context =3D
>> pool_get_session_context(false);
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0POOL_SESSI= ON_CONTEXT *session_context;
>>
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (pool_c= onfig->disable_load_balance_on_write =3D=3D
>> DLBOW_DML_ADAPTIVE && session_context->is_in_transactio= n)
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0session_co= ntext =3D pool_get_session_context(false);
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (sessio= n_context->is_in_transaction)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0ereport(DEBUG1,
>> -
>>=C2=A0 (errmsg("is_select_object_in_temp_write_list: \"%s= \", found relation
>> \"%s\"", (char *) context, rgv->relname)));
>> +
>>=C2=A0 (errmsg("is_select_object_in_temp_write_list:"
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0" \"%s= \", found
>> relation \"%s\"",
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(char *) context= ,
>> rgv->relname)));
>>
>> I guess you aimed to shorten the message so that it does not excee= d
>> 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 form= er style.
>>
>> Same suggestion is applied to all other similar places.
>>
>> @@ -2169,6 +2275,153 @@ where_to_send_main_replica(POOL_QUERY_CONT= EXT
>> *query_context, char *query, Node
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{
>>
>> pool_set_node_to_be_sent(query_context, PRIMARY_NODE_ID);
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/*
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * Check track table mutati= on for recently
>> written tables.=C2=A0 If
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * in cold start or any tab= le was recently
>> written, route to
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * primary to avoid stale r= eads.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else if
>> (pool_config->disable_load_balance_on_write =3D=3D
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 DLBOW_DML_ADAPTIVE_GLOBAL)
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{
>>
>> 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 Post= greSQL
>> + * written by Tatsuo Ishii
>> + *
>> + * Copyright (c) 2003-2026=C2=A0 =C2=A0 =C2=A0PgPool Global Devel= opment Group
>>
>> Since this file is new, the Copyright year should be just 2026. >> + * Copyright (c) 2026=C2=A0 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 Post= greSQL
>> + * written by Tatsuo Ishii
>> + *
>> + * Copyright (c) 2003-2026=C2=A0 =C2=A0 =C2=A0PgPool Global Devel= opment Group
>>
>> Since this file is new, the Copyright year should be just 2026. >> + * Copyright (c) 2026=C2=A0 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 sur= e
>> everyone is famimilar with FNV-1a (at lease I am not).
>>
>> +/*
>> + * Get current time
>> + */
>> +static void
>> +get_current_time(struct timeval *tv)
>> +{
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0gettimeofday(tv, NULL);
>> +}
>>
>> Just out of curiosity, why you don't call gettimeofday directl= y?
>>
>> 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
--000000000000dcb44a065548d830--