Re: Flush some statistics within running transactions
| От | Bertrand Drouvot |
|---|---|
| Тема | Re: Flush some statistics within running transactions |
| Дата | |
| Msg-id | aW4gwqVybB4INCvu@ip-10-97-1-34.eu-west-3.compute.internal обсуждение исходный текст |
| Ответ на | Re: Flush some statistics within running transactions (Sami Imseih <samimseih@gmail.com>) |
| Ответы |
Re: Flush some statistics within running transactions
Re: Flush some statistics within running transactions |
| Список | pgsql-hackers |
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
Вложения
В списке pgsql-hackers по дате отправления: