| From: | Pavlo Golub <pavlo(dot)golub(at)cybertec(dot)at> |
|---|---|
| To: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
| Cc: | Sami Imseih <samimseih(at)gmail(dot)com>, Christoph Berg <myon(at)debian(dot)org>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: Re[2]: [PATCH] Add last_executed timestamp to pg_stat_statements |
| Date: | 2026-02-05 17:35:27 |
| Message-ID: | CAK7ymc+E00zKC9Da+BK2se=O2RiOUkdW5dz4iAynD1Vj=Pwexg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi all,
Thank you to Sami, Christoph, and Bertrand for the thorough review and valuable
feedback on v1. I've prepared a v2 patch that addresses all the concerns raised.
You're absolutely right that calling `GetCurrentTimestamp()` while holding a
spinlock is unacceptable.
I've adopted Sami's suggestion to use `GetCurrentStatementStartTimestamp()`
instead. While Christoph correctly points out that this means we're capturing
statement start time rather than end time, I believe this is an acceptable
trade-off for several reasons:
- the vast majority of statements complete quickly (well under typical polling
intervals of 1-5 minutes);
- long-running queries will appear in the next poll anyway;
- deferring timestamp updates adds significant complexity;
- and most importantly zero performance overhead vs a kernel call per execution.
For the monitoring use case, what matters is knowing has this statement been
active since my last poll? The start time serves this purpose effectively.
I've renamed the column to `stats_last_updated` as Christoph suggested. This
matches the existing "stats_since" column for consistency. Following Christoph's
suggestion, I've also moved it to the end of the view.
Per Sami's feedback, I've moved the timestamp from the `Counters`
struct directly
to `pgssEntry`. This is more semantically correct since it's metadata about the
entry rather than a statistical counter.
I've updated `PGSS_FILE_HEADER` to 0x20260205.
Among other changes:
- fixed all whitespace errors
- moved tests to `entry_timestamp.sql` as suggested
- added documentation notice about nested statement behavior
- the timestamp is now set outside the spinlock-protected section
Sami mentioned this was discussed in 2017 [0]. Looking at that thread, the main
concerns were overhead and use cases. I believe we've addressed both:
- zero overhead by using existing statement start timestamp
- clear, documented use case from production monitoring tools (pgwatch, etc.)
I've attached the v2 patch. All existing tests pass.
I'd appreciate further review and feedback.
Thanks,
Pavlo Golub
| Attachment | Content-Type | Size |
|---|---|---|
| v2-0001-pg_stat_statements-Add-stats_last_updated-column.patch | application/octet-stream | 17.0 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Nathan Bossart | 2026-02-05 17:36:00 | Re: pg_upgrade: transfer pg_largeobject_metadata's files when possible |
| Previous Message | Jacob Champion | 2026-02-05 17:26:34 | Re: client_connection_check_interval default value |