Fix bugs not to discard statistics when changing stats_fetch_consistency
Hi, hackers
There is below description in docs for stats_fetch_consistency.
"Changing this parameter in a transaction discards the statistics
snapshot."
However, I wonder if changes stats_fetch_consistency in a transaction,
statistics is not discarded in some cases.
Example:
--
* session 1
=# SET stats_fetch_consistency TO snapshot;
=# BEGIN;
=*# SELECT wal_records, wal_fpi, wal_bytes FROM pg_stat_wal;
wal_records | wal_fpi | wal_bytes
-------------+---------+-----------
23592 | 628 | 5939027
(1 row)
* session 2
=# CREATE TABLE test (i int); -- generate WAL records
=# SELECT wal_records, wal_fpi, wal_bytes FROM pg_stat_wal;
wal_records | wal_fpi | wal_bytes
-------------+---------+-----------
23631 | 644 | 6023411
(1 row)
* session 1
=*# -- snapshot is not discarded, it is right
=*# SELECT wal_records, wal_fpi, wal_bytes FROM pg_stat_wal;
wal_records | wal_fpi | wal_bytes
-------------+---------+-----------
23592 | 628 | 5939027
(1 row)
=*# SET stats_fetch_consistency TO cache;
=*# -- snapshot is not discarded, it is not right
=*# SELECT wal_records, wal_fpi, wal_bytes FROM pg_stat_wal;
wal_records | wal_fpi | wal_bytes
-------------+---------+-----------
23592 | 628 | 5939027
(1 row)
--
I can see similar cases in pg_stat_archiver, pg_stat_bgwriter,
pg_stat_checkpointer, pg_stat_io, and pg_stat_slru.
Is it a bug? I fixed it, and do you think?
--
Regards,
Shinya Kato
NTT DATA GROUP CORPORATION
Attachments:
v1-0001-fix-stats-fetch-consinstency-bugs.patchtext/x-diff; name=v1-0001-fix-stats-fetch-consinstency-bugs.patchDownload+14-0
On Thu, Jan 11, 2024 at 06:18:38PM +0900, Shinya Kato wrote:
Hi, hackers
(Sorry for the delay, this thread was on my TODO list for some time.)
There is below description in docs for stats_fetch_consistency.
"Changing this parameter in a transaction discards the statistics snapshot."However, I wonder if changes stats_fetch_consistency in a transaction,
statistics is not discarded in some cases.
Yep, you're right. This is inconsistent with the documentation where
we need to clean up the cached data when changing this GUC. I was
considering a few options regarding the location of the extra
pgstat_clear_snapshot(), but at the end I see the point in doing it in
a path even if it creates a duplicate with pgstat_build_snapshot()
when pgstat_fetch_consistency is using the snapshot mode. A location
better than your patch is pgstat_snapshot_fixed(), though, so as new
stats kinds will be able to get the call.
I have been banging my head on my desk for a bit when thinking about a
way to test that in a predictible way, until I remembered that these
stats are only flushed at commit, so this requires at least two
sessions, with one of them having a transaction opened while
manipulating stats_fetch_consistency. TAP would be one option, but
I'm not really tempted about spending more cycles with a
background_psql just for this case. If I'm missing something, feel
free.
--
Michael
On 2024-02-01 17:33, Michael Paquier wrote:
On Thu, Jan 11, 2024 at 06:18:38PM +0900, Shinya Kato wrote:
Hi, hackers
(Sorry for the delay, this thread was on my TODO list for some time.)
There is below description in docs for stats_fetch_consistency.
"Changing this parameter in a transaction discards the statistics
snapshot."However, I wonder if changes stats_fetch_consistency in a transaction,
statistics is not discarded in some cases.Yep, you're right. This is inconsistent with the documentation where
we need to clean up the cached data when changing this GUC. I was
considering a few options regarding the location of the extra
pgstat_clear_snapshot(), but at the end I see the point in doing it in
a path even if it creates a duplicate with pgstat_build_snapshot()
when pgstat_fetch_consistency is using the snapshot mode. A location
better than your patch is pgstat_snapshot_fixed(), though, so as new
stats kinds will be able to get the call.I have been banging my head on my desk for a bit when thinking about a
way to test that in a predictible way, until I remembered that these
stats are only flushed at commit, so this requires at least two
sessions, with one of them having a transaction opened while
manipulating stats_fetch_consistency. TAP would be one option, but
I'm not really tempted about spending more cycles with a
background_psql just for this case. If I'm missing something, feel
free.
Thank you for the review and pushing!
I think it is better and more concise than my implementation.
--
Regards,
Shinya Kato
NTT DATA GROUP CORPORATION