MIME-Version: 1.0
References: <aWTVEycKj7Qh/SXH@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA5RZ0vgN8nf7yPrk_uNt-ABFtRnRuz7-9CM=qSzUSwj=EEUKw@mail.gmail.com>
 <aWjbBd5z+LJNWX1l@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA5RZ0t6j0VYuUpxZ8JLq-ERoUriZ0rK=+8PCUtjRirmSmCx7A@mail.gmail.com> <aWojE18aF9qj0EPI@ip-10-97-1-34.eu-west-3.compute.internal>
In-Reply-To: <aWojE18aF9qj0EPI@ip-10-97-1-34.eu-west-3.compute.internal>
From: Sami Imseih <samimseih@gmail.com>
Date: Fri, 16 Jan 2026 10:44:48 -0600
Message-ID: <CAA5RZ0s9j3x5UPpgaQdDyGA=MNjVEkT73SL7LMoVEKUwiZrVqA@mail.gmail.com>
Subject: Re: Flush some statistics within running transactions
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAA5RZ0s9j3x5UPpgaQdDyGA%3DMNjVEkT73SL7LMoVEKUwiZrVqA%40mail.gmail.com>
Precedence: bulk

I took a look at 0001 in depth.

> I don't think this feature could add a noticeable performance impact, so the tests
> have been that simple. Do you think we should worry more?

One observation is there's no coordination between ANYTIME and
TXN_BOUNDARY flushes. While PGSTAT_MIN_INTERVAL
prevents a backend from flushing more than once per second, a backend can
still perform both an ANYTIME flush and a TXN_BOUNDARY flush within
the same 1-second window. Not saying this will be a real problem in
the real-world,
but we definitely took measures in the current implementation to avoid
this scenario.

A few other comments on 0001

+               /* Skip if completely idle */
+               if (!DoingCommandRead || IsTransactionOrTransactionBlock())
+                       pgstat_report_anytime_stat(false);

Does this need to be conditional? worst case, we return right away with an empty
list. Best case, is we are consistently flushing.

+       /*
+        * When in anytime_only mode, the list may not be empty because
+        * FLUSH_AT_TXN_BOUNDARY entries were skipped.
+        */
+       Assert(!anytime_only || dlist_is_empty(&pgStatPending) ==
!have_pending);

Checking for !anytime_only is unnecessary here.
"list_is_empty(&pgStatPending) == !have_pending"
should be true regardless of ANYTIME or TXN_BOUNDARY, right?

Below are a couple of edits for comments I felt would improve
readability of the code.

1/
 /*
- * Flush non-transactional stats
- *
- * This is safe to call even inside a transaction. It only flushes stats
- * kinds marked as FLUSH_ANYTIME.
- *
- * This allows long running transactions to report activity without waiting
- * for transaction to finish.
+ * Flushes only FLUSH_ANYTIME stats using non-blocking locks. Transactional
+ * stats (FLUSH_AT_TXN_BOUNDARY) remain pending until transaction boundary.
+ * Safe to call inside transactions.
  */

2/
 typedef enum PgStat_FlushBehavior
 {
-       FLUSH_ANYTIME,                          /* All fields can
flush anytime */
-       FLUSH_AT_TXN_BOUNDARY,          /* All fields need transaction
boundary */
+       FLUSH_ANYTIME,                          /* All fields can be
flushed anytime,
+                                                                *
including within transactions */
+       FLUSH_AT_TXN_BOUNDARY,          /* All fields can only be flushed at
+                                                                *
transaction boundary */
 } PgStat_FlushBehavior;

I will start looking at the remaining patches next.

--
Sami Imseih
Amazon Web Services (AWS)