Re: New instability in stats regression test

On Mon, Nov 27, 2023 at 8:26 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> I was ready to argue that we'd better keep this test and keep it close
> to the end of stats.sql while documenting why things are kept in this
> order,

It's easy for someone to come and add pg_stat_reset_shared() before
the end without noticing the comment as the test failure is sporadic
in nature.

> but two resets done on the same shared stats type would still
> be prone to race conditions without all the previous activity done in
> the tests (like pg_stat_wal).

Can running stats.sql in non-parallel mode help stabilize the tests as-is?

> With all that in mind and because we have checks for the individual
> targets with pg_stat_reset_shared(), I would agree to just remove it
> entirely.  Say as of the attached?

I tend to agree with this approach, the code is anyways covered. I
think the attached patch also needs to remove setting
archiver_reset_ts (and friends) after pg_stat_reset_shared('archiver')
(and friends), something like [1].

Can we also remove pg_stat_reset_slru() with no argument test to keep
things consistent?

-- Test that multiple SLRUs are reset when no specific SLRU provided
to reset function
SELECT pg_stat_reset_slru();
SELECT stats_reset > :'slru_commit_ts_reset_ts'::timestamptz FROM
pg_stat_slru WHERE name = 'CommitTs';
SELECT stats_reset > :'slru_notify_reset_ts'::timestamptz FROM
pg_stat_slru WHERE name = 'Notify';

[1]
diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index d867fb406f..e3b4ca96e8 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -462,37 +462,31 @@ SELECT stats_reset >
:'slru_notify_reset_ts'::timestamptz FROM pg_stat_slru WHER
 SELECT stats_reset AS archiver_reset_ts FROM pg_stat_archiver \gset
 SELECT pg_stat_reset_shared('archiver');
 SELECT stats_reset > :'archiver_reset_ts'::timestamptz FROM pg_stat_archiver;
-SELECT stats_reset AS archiver_reset_ts FROM pg_stat_archiver \gset

 -- Test that reset_shared with bgwriter specified as the stats type works
 SELECT stats_reset AS bgwriter_reset_ts FROM pg_stat_bgwriter \gset
 SELECT pg_stat_reset_shared('bgwriter');
 SELECT stats_reset > :'bgwriter_reset_ts'::timestamptz FROM pg_stat_bgwriter;
-SELECT stats_reset AS bgwriter_reset_ts FROM pg_stat_bgwriter \gset

 -- Test that reset_shared with checkpointer specified as the stats type works
 SELECT stats_reset AS checkpointer_reset_ts FROM pg_stat_checkpointer \gset
 SELECT pg_stat_reset_shared('checkpointer');
 SELECT stats_reset > :'checkpointer_reset_ts'::timestamptz FROM
pg_stat_checkpointer;
-SELECT stats_reset AS checkpointer_reset_ts FROM pg_stat_checkpointer \gset

 -- Test that reset_shared with recovery_prefetch specified as the
stats type works
 SELECT stats_reset AS recovery_prefetch_reset_ts FROM
pg_stat_recovery_prefetch \gset
 SELECT pg_stat_reset_shared('recovery_prefetch');
 SELECT stats_reset > :'recovery_prefetch_reset_ts'::timestamptz FROM
pg_stat_recovery_prefetch;
-SELECT stats_reset AS recovery_prefetch_reset_ts FROM
pg_stat_recovery_prefetch \gset

 -- Test that reset_shared with slru specified as the stats type works
 SELECT max(stats_reset) AS slru_reset_ts FROM pg_stat_slru \gset
 SELECT pg_stat_reset_shared('slru');
 SELECT max(stats_reset) > :'slru_reset_ts'::timestamptz FROM pg_stat_slru;
-SELECT max(stats_reset) AS slru_reset_ts FROM pg_stat_slru \gset

 -- Test that reset_shared with wal specified as the stats type works
 SELECT stats_reset AS wal_reset_ts FROM pg_stat_wal \gset
 SELECT pg_stat_reset_shared('wal');
 SELECT stats_reset > :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
-SELECT stats_reset AS wal_reset_ts FROM pg_stat_wal \gset

 -- Test error case for reset_shared with unknown stats type
 SELECT pg_stat_reset_shared('unknown');


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