| From: | Sami Imseih <samimseih(at)gmail(dot)com> |
|---|---|
| To: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: Flush some statistics within running transactions |
| Date: | 2026-01-16 16:44:48 |
| Message-ID: | CAA5RZ0s9j3x5UPpgaQdDyGA=MNjVEkT73SL7LMoVEKUwiZrVqA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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)
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Corey Huinker | 2026-01-16 16:49:30 | Re: Import Statistics in postgres_fdw before resorting to sampling. |
| Previous Message | Jacob Champion | 2026-01-16 16:39:04 | Re: Custom oauth validator options |