| 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-21 07:18:35 |
| Message-ID: | CACeKOO31reCtDYU1twicmqgdEStCJXWJ6P4LMFMYytaYaGew-A@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgpool-hackers |
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.
So I fixed both uniformly: resolve the written tables' OIDs (and the
database OID) at DML *routing* time, when the connection is idle and
do_query is safe -- the same context where the feature already resolves
OIDs for the SELECT staleness check. The OIDs are stashed in the
session context and only *marked* in shared memory at CommandComplete,
which now does no backend I/O at all. Autocommit statements mark on
their own CommandComplete; explicit transactions accumulate and mark on
COMMIT; ROLLBACK and failed/rolled-back COMMITs never reach the mark
step, so their writes are correctly not recorded.
A side benefit: using pool_extract_table_oids() for both paths means we
now mark only the actual write *targets*, instead of every relation in
the statement (the in-transaction path previously walked all RangeVars,
so an "INSERT ... SELECT FROM t" would also mark the read source t).
Since your really_writing_transaction fix is now in master (1333d2ae8),
I dropped the copy I was carrying and rebased the feature directly onto
current master. One patch this time:
v6-0001-Feature-load-balancing-control-by-table-tracking.patch
Verified on a PG16 streaming-replication cluster (dml_adaptive_global,
standby weighted for load balancing):
- your deferred-constraint COMMIT fails -> table not marked -> reads
keep load-balancing to the standby;
- a successful COMMIT, and a successful autocommit write -> table
marked -> reads forced to the primary;
- COMMIT returns promptly (no hang), and "DROP TABLE IF EXISTS
<nonexistent>" no longer hangs.
Thanks!
On Fri, Jun 19, 2026 at 5:10 AM Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
> > understood. we'll wait patiently or try master.
> >
> > thanks!
>
> While reviewing
> v5-0002-Feature-load-balancing-control-by-table-tracking.patch, I
> noticed following code:
>
> static void
> dml_adaptive(Node *node, char *query)
> {
> :
> :
> + /*
> + * For dml_adaptive_global: on COMMIT,
> flush the accumulated
> + * table writes to shared memory. On
> ROLLBACK, skip -- the
> + * writes never committed so no stale-read
> risk exists. This
> + * prevents polluting the table map with
> rolled-back
> + * transactions.
> + */
> + int dlbow =
> + pool_config->disable_load_balance_on_write;
> + List *wlist =
> +
> session_context->transaction_temp_write_list;
> +
> + if (dlbow == DLBOW_DML_ADAPTIVE_GLOBAL &&
> + is_commit_query(node) &&
> + wlist != NIL)
> + {
> + ListCell *cell;
> + int dboid;
> +
> + dboid =
> +
> pool_track_table_mutation_get_database_oid();
> + if (dboid > 0)
> + {
> + foreach(cell, wlist)
> + {
> + char *tname;
> + int
> toid;
> +
> + tname = (char *)
> lfirst(cell);
> + toid =
> +
> pool_table_name_to_oid(tname);
> +
> + if (toid > 0)
> +
> pool_track_table_mutation_mark_table_written(
> +
>
> toid, dboid);
> + }
>
> The code path is executed before the commit command is sent to primary
> PostgreSQL. I wonder what will happen if the commit command failed?
> Example:
>
> CREATE TEMP TABLE t1(i int PRIMARY KEY DEFERRABLE INITIALLY DEFERRED);
> CREATE TABLE
> INSERT INTO t1 VALUES(1);
> INSERT 0 1
> BEGIN;
> BEGIN
> INSERT INTO t1 VALUES(1);
> INSERT 0 1
> INSERT INTO t1 VALUES(2);
> INSERT 0 1
> COMMIT;
> psql:deferable.sql:6: ERROR: duplicate key value violates unique
> constraint "t1_pkey"
> DETAIL: Key (i)=(1) already exists.
>
> As you can see, the COMMIT command failed and the effect of INSERT
> commands vanished too. However the code invalidates the data on the
> shared memory by pool_track_table_mutation_mark_table_written() when
> it should not be called?
>
> 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
| Attachment | Content-Type | Size |
|---|---|---|
| v6-0001-Feature-load-balancing-control-by-table-tracking.patch | application/octet-stream | 96.9 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Tatsuo Ishii | 2026-06-23 11:38:48 | Re: Proposal: Recent mutated table tracking in memory |
| Previous Message | Tatsuo Ishii | 2026-06-19 02:10:37 | Re: Proposal: Recent mutated table tracking in memory |