| From: | Nadav Shatz <nadav(at)tailorbrands(dot)com> |
|---|---|
| To: | Tatsuo Ishii <ishii(at)postgresql(dot)org> |
| Cc: | pgpool-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: Proposal: Recent mutated table tracking in memory |
| Date: | 2026-06-26 10:25:05 |
| Message-ID: | CACeKOO0aG2qyMPmeRpVxSR92YnF+i0iRk3to=KqJ_3xG6PBOmg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgpool-hackers |
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.
Sorry again for the confusing earlier phrasing.
On Fri, Jun 26, 2026 at 1:40 AM Tatsuo Ishii <ishii(at)postgresql(dot)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
>
--
Nadav Shatz
Tailor Brands | CTO
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Tatsuo Ishii | 2026-06-27 01:51:08 | Re: Proposal: Recent mutated table tracking in memory |
| Previous Message | Tatsuo Ishii | 2026-06-26 06:32:27 | Reject sub-minimum ErrorResponse length in read_kind_from_backend. |