public inbox for [email protected]  

help / color / mirror / Atom feed

From: Sami Imseih <[email protected]>
To: Bertrand Drouvot <[email protected]>
Cc: [email protected]
Subject: Re: Flush some statistics within running transactions
Date: Fri, 16 Jan 2026 10:44:48 -0600
Message-ID: <CAA5RZ0s9j3x5UPpgaQdDyGA=MNjVEkT73SL7LMoVEKUwiZrVqA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <aWTVEycKj7Qh/[email protected]>
	<CAA5RZ0vgN8nf7yPrk_uNt-ABFtRnRuz7-9CM=qSzUSwj=EEUKw@mail.gmail.com>
	<[email protected]>
	<CAA5RZ0t6j0VYuUpxZ8JLq-ERoUriZ0rK=+8PCUtjRirmSmCx7A@mail.gmail.com>
	<[email protected]>

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)

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Flush some statistics within running transactions
  In-Reply-To: <CAA5RZ0s9j3x5UPpgaQdDyGA=MNjVEkT73SL7LMoVEKUwiZrVqA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox