MIME-Version: 1.0
References: 
 <CACeKOO02V7_SBpCUJWG7yS-ABNEXzAAU-vb+JrY=rD8yBiiy0g@mail.gmail.com>
 <20260619.111037.159094338431818955.ishii@postgresql.org>
 <CACeKOO31reCtDYU1twicmqgdEStCJXWJ6P4LMFMYytaYaGew-A@mail.gmail.com>
 <20260626.074028.806271918741516663.ishii@postgresql.org>
In-Reply-To: <20260626.074028.806271918741516663.ishii@postgresql.org>
From: Nadav Shatz <nadav@tailorbrands.com>
Date: Fri, 26 Jun 2026 13:25:05 +0300
Message-ID: 
 <CACeKOO0aG2qyMPmeRpVxSR92YnF+i0iRk3to=KqJ_3xG6PBOmg@mail.gmail.com>
Subject: Re: Proposal: Recent mutated table tracking in memory
To: Tatsuo Ishii <ishii@postgresql.org>
Cc: pgpool-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="0000000000006e087d0655258657"
Archived-At: 
 <https://www.postgresql.org/message-id/CACeKOO0aG2qyMPmeRpVxSR92YnF%2Bi0iRk3to%3DKqJ_3xG6PBOmg%40mail.gmail.com>
Precedence: bulk

--0000000000006e087d0655258657
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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

Sorry again for the confusing earlier phrasing.

On Fri, Jun 26, 2026 at 1:40=E2=80=AFAM Tatsuo Ishii <ishii@postgresql.org>=
 wrote:

> Hi Nadav,
>
> > Hi Tatsuo,
> >
> > Good catch -- that was a bug, and chasing it down turned up a second
> > one in the same area.
> >
> > The original problem you described: the flush ran in dml_adaptive()
> > while pgpool was *routing* the COMMIT, before the COMMIT was sent to
> > the backend.  So a COMMIT that later failed (your deferred-constraint
> > example) had already marked the tables, even though the writes rolled
> > back.
> >
> > My first instinct was to just move the flush to handle_query_context(),
> > where reaching CommandComplete for the COMMIT means it succeeded.  But
> > that path resolves table/database OIDs via pool_table_name_to_oid() /
> > the relcache, which on a cache miss issues a do_query() -- and at that
> > point the COMMIT response is still being read from the backend, so
> > injecting a query desyncs the protocol and hangs the session.  Testing
> > caught this immediately.
> >
> > It turned out the existing autocommit marking branch had the very same
> > hazard: pool_extract_table_oids() in handle_query_context() also does a
> > relcache do_query() on a miss, so e.g. "DROP TABLE IF EXISTS
> <nonexistent>"
> > (an uncacheable name -> guaranteed miss) reliably hung the session too.
>
> Are you saying that the non patched pgpool has a problem?
> I tried following queries with master branch and found no hang.
>
> BEGIN;
> DROP TABLE IF EXISTS non_existent_table;
> COMMIT;
>
> Regards,
> --
> Tatsuo Ishii
> SRA OSS K.K.
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp
>


--=20
Nadav Shatz
Tailor Brands | CTO

--0000000000006e087d0655258657
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Hi Tatsuo,<br><br>Sorry -- my wording was misleading. No, =
unpatched pgpool does NOT have<br>this problem, and you&#39;re right that y=
our test shows no hang on master.<br>The hang is entirely in the new code m=
y patch adds, not in pgpool<br>itself.<br><br>To be precise: the marking co=
de that hung is the dml_adaptive_global<br>branch in handle_query_context()=
 (CommandComplete.c) that my patch<br>introduces. It is gated on disable_lo=
ad_balance_on_write =3D<br>dml_adaptive_global, which only exists with the =
patch applied, so master<br>never runs it.<br><br>It also doesn&#39;t fire =
for your exact sequence, for two reasons:<br><br>=C2=A0 1. You ran it on ma=
ster, which doesn&#39;t contain the branch at all.<br>=C2=A0 2. Even on a p=
atched build, your statements are inside an explicit<br>=C2=A0 =C2=A0 =C2=
=A0transaction (BEGIN ... COMMIT), and that branch only runs for<br>=C2=A0 =
=C2=A0 =C2=A0autocommit statements (the &quot;!is_in_transaction&quot; case=
).<br><br>What actually hung, on a patched build with dml_adaptive_global s=
et in<br>streaming replication, was a *bare autocommit* statement (no surro=
unding<br>transaction) whose table OID cannot be served from the relcache -=
- for<br>example:<br><br>=C2=A0 DROP TABLE IF EXISTS non_existent_table; =
=C2=A0 =C2=A0 -- no BEGIN/COMMIT<br><br>Because the name is unresolvable, p=
ool_table_name_to_oid() does not cache<br>it (no_cache_if_zero), so every l=
ookup is a relcache miss. The marking<br>branch calls pool_extract_table_oi=
ds() -&gt; ... -&gt; do_query() to resolve<br>the OID, but handle_query_con=
text() runs before the statement&#39;s<br>CommandComplete response has been=
 read from the backend, so injecting<br>that query desyncs the protocol and=
 the session blocks. Backtrace:<br><br>=C2=A0 #2 =C2=A0pool_read2 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(blocked in read())<br>=C2=
=A0 #3 =C2=A0do_query =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0&quot;SELECT COALESCE(pg_catalog.to_regclass(&#39;...&#39;)::oid,=
 0)&quot;<br>=C2=A0 #4 =C2=A0pool_search_relcache =C2=A0 =C2=A0 =C2=A0(relc=
ache miss)<br>=C2=A0 #5 =C2=A0pool_table_name_to_oid<br>=C2=A0 #6 =C2=A0poo=
l_extract_table_oids =C2=A0 query_cache/pool_memqcache.c<br>=C2=A0 #7 =C2=
=A0handle_query_context =C2=A0 =C2=A0 =C2=A0protocol/CommandComplete.c =C2=
=A0(!is_in_transaction branch)<br>=C2=A0 #8 =C2=A0CommandComplete<br><br>Th=
is is the same class of issue as the one behind your original review<br>com=
ment: resolving OIDs at CommandComplete time means doing backend I/O<br>whi=
le a response is in flight.<br><br>v6-0001 fixes it for good by resolving t=
he written tables&#39; OIDs (and the<br>database OID) at DML *routing* time=
 -- when the connection is idle and<br>do_query is safe, the same point the=
 feature already resolves OIDs for<br>the SELECT staleness check. handle_qu=
ery_context() then only marks the<br>already-resolved OIDs and does no back=
end I/O at all, for both the<br>autocommit and the COMMIT paths.<br><br>Ver=
ified on a PG16 streaming-replication cluster with dml_adaptive_global:<br>=
the bare &quot;DROP TABLE IF EXISTS non_existent_table;&quot; and your<br>&=
quot;BEGIN; DROP TABLE IF EXISTS ...; COMMIT;&quot; both return promptly, t=
he<br>deferred-constraint COMMIT failure leaves the table unmarked (reads k=
eep<br>load-balancing to the standby), and a successful autocommit or COMMI=
T<br>write marks the table and forces reads to the primary.<br><br>Sorry ag=
ain for the confusing earlier phrasing.<br></div><br><div class=3D"gmail_qu=
ote gmail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Fri, Ju=
n 26, 2026 at 1:40=E2=80=AFAM Tatsuo Ishii &lt;<a href=3D"mailto:ishii@post=
gresql.org">ishii@postgresql.org</a>&gt; wrote:<br></div><blockquote class=
=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rg=
b(204,204,204);padding-left:1ex">Hi Nadav,<br>
<br>
&gt; Hi Tatsuo,<br>
&gt; <br>
&gt; Good catch -- that was a bug, and chasing it down turned up a second<b=
r>
&gt; one in the same area.<br>
&gt; <br>
&gt; The original problem you described: the flush ran in dml_adaptive()<br=
>
&gt; while pgpool was *routing* the COMMIT, before the COMMIT was sent to<b=
r>
&gt; the backend.=C2=A0 So a COMMIT that later failed (your deferred-constr=
aint<br>
&gt; example) had already marked the tables, even though the writes rolled<=
br>
&gt; back.<br>
&gt; <br>
&gt; My first instinct was to just move the flush to handle_query_context()=
,<br>
&gt; where reaching CommandComplete for the COMMIT means it succeeded.=C2=
=A0 But<br>
&gt; that path resolves table/database OIDs via pool_table_name_to_oid() /<=
br>
&gt; the relcache, which on a cache miss issues a do_query() -- and at that=
<br>
&gt; point the COMMIT response is still being read from the backend, so<br>
&gt; injecting a query desyncs the protocol and hangs the session.=C2=A0 Te=
sting<br>
&gt; caught this immediately.<br>
&gt; <br>
&gt; It turned out the existing autocommit marking branch had the very same=
<br>
&gt; hazard: pool_extract_table_oids() in handle_query_context() also does =
a<br>
&gt; relcache do_query() on a miss, so e.g. &quot;DROP TABLE IF EXISTS &lt;=
nonexistent&gt;&quot;<br>
&gt; (an uncacheable name -&gt; guaranteed miss) reliably hung the session =
too.<br>
<br>
Are you saying that the non patched pgpool has a problem?<br>
I tried following queries with master branch and found no hang.<br>
<br>
BEGIN;<br>
DROP TABLE IF EXISTS non_existent_table;<br>
COMMIT;<br>
<br>
Regards,<br>
--<br>
Tatsuo Ishii<br>
SRA OSS K.K.<br>
English: <a href=3D"http://www.sraoss.co.jp/index_en/" rel=3D"noreferrer" t=
arget=3D"_blank">http://www.sraoss.co.jp/index_en/</a><br>
Japanese:<a href=3D"http://www.sraoss.co.jp" rel=3D"noreferrer" target=3D"_=
blank">http://www.sraoss.co.jp</a><br>
</blockquote></div><div><br clear=3D"all"></div><div><br></div><span class=
=3D"gmail_signature_prefix">-- </span><br><div dir=3D"rtl" class=3D"gmail_s=
ignature"><div dir=3D"ltr"><div><div dir=3D"ltr"><div dir=3D"rtl"><div dir=
=3D"ltr"><font color=3D"#000000">Nadav Shatz</font></div><div dir=3D"ltr"><=
font color=3D"#000000">Tailor Brands=C2=A0| CTO</font></div></div></div></d=
iv></div></div>

--0000000000006e087d0655258657--