Date: Mon, 19 Jan 2026 12:17:06 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Sami Imseih <samimseih@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Flush some statistics within running transactions
Message-ID: <aW4gwqVybB4INCvu@ip-10-97-1-34.eu-west-3.compute.internal>
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>
 <CAA5RZ0s9j3x5UPpgaQdDyGA=MNjVEkT73SL7LMoVEKUwiZrVqA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="OFsrIl+bjhifp5hk"
Content-Disposition: inline
In-Reply-To: <CAA5RZ0s9j3x5UPpgaQdDyGA=MNjVEkT73SL7LMoVEKUwiZrVqA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/aW4gwqVybB4INCvu%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk


--OFsrIl+bjhifp5hk
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi,

On Fri, Jan 16, 2026 at 10:44:48AM -0600, Sami Imseih wrote:
> I took a look at 0001 in depth.

Thanks!

> > 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.

Right. I think that the PGSTAT_MIN_INTERVAL throttling was put in place to prevent
flushing too frequently when the backend has a high commit rate. But here, while
it's true that we don't follow that rule (means a backend could flush more than one
time per second), that would be a maximum of 2 times (given that ANYTIME is
flushing every second). So, I'm not sure that this single extra flush is worth
worrying about. Plus we'd certainly need an extra GetCurrentTimestamp() call, so
I'm not sure it's worth it.

> 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.

Yeah, I think we could remove this check and just rely on the ones in
pgstat_report_anytime_stat(). Done in the attached.

> +       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?

Right, thanks for catching it, it was remaining garbage from my dev iterations.

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

Done as suggested.

> I will start looking at the remaining patches next.

Thanks!

Note that I also updated the doc in 0003 for the stats that have mixed fields.

BTW, I think that we could also make the Function stat kind as flush any time,
thoughts?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

--OFsrIl+bjhifp5hk
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Add-pgstat_report_anytime_stat-for-periodic-stats.patch"