Hi, On 2023-05-22 13:36:42 +0900, Kyotaro Horiguchi wrote: > At Sun, 21 May 2023 15:14:23 -0700, Andres Freund <and...@anarazel.de> wrote > in > > Hi, > > > > I don't really feel confident we're going to get the timeout approach solid > > enough for something going into the tree around beta 1. > > > > How about this, imo a lot simpler, approach: We flush stats whenever > > replaying > > a XLOG_RUNNING_XACTS record. Unless the primary is idle, it will log those > > at > > a regular interval. If the primary is idle, we don't need to flush stats in > > the startup process, because we'll not have done any io. > > > > We only log XLOG_RUNNING_XACTS when wal_level >= replica, so stats wouldn't > > get regularly flushed if wal_level = minimal - but in that case the stats > > are > > also not accessible, so that's not a problem. > > I concur with the three aspects, interval regularity, reliability and > about the case of the minimal wal_level. While I can't confidently > claim it is the best, its simplicilty gives us a solid reason to > proceed with it. If necessary, we can switch to alternative timing > sources in the future without causing major disruptions for users, I > believe. > > > It's not the prettiest solution, but I think the simplicity is worth a lot. > > > > Greetings, > > +1.
Attached a patch implementing this approach. It's imo always a separate bug that we were using GetCurrentTransactionStopTimestamp() when force was passed in - that timestamp can be quite out of date in some cases. But I don't immediately see a noticeable consequence, so ... Greetings, Andres Freund
>From 249c811da9d257c5a71e42016584fc3db0c9a99b Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Fri, 9 Jun 2023 21:19:36 -0700 Subject: [PATCH v1] Report stats when replaying XLOG_RUNNING_XACTS Previously stats in the startup process would only get reported during shutdown of the startup process. It has been that way for a long time, but became a lot more noticeable with the new pg_stat_io view, which separates out IO done by different backend types... While replaying after every XLOG_RUNNING_XACTS isn't the prettiest approach, it has the advantage of being quite easy. Given that we're well past feature freeze... It's not a problem that we don't report stats more frequently with wal_level=minimal, in that case stats can't be read before the stats process has shut down. Reported-by: Fujii Masao <masao.fu...@oss.nttdata.com> Discussion: https://postgr.es/m/5315aedc-fbca-1556-c5de-dc2e00b23...@oss.nttdata.com --- src/backend/storage/ipc/standby.c | 9 +++++++++ src/backend/utils/activity/pgstat.c | 17 ++++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c index ffe5e1563f5..70b0da50fc1 100644 --- a/src/backend/storage/ipc/standby.c +++ b/src/backend/storage/ipc/standby.c @@ -1193,6 +1193,15 @@ standby_redo(XLogReaderState *record) running.xids = xlrec->xids; ProcArrayApplyRecoveryInfo(&running); + + /* + * The startup process currently has no convenient way to schedule + * stats to be reported. XLOG_RUNNING_XACTS records issued at a + * regular cadence, making this a convenient location to report + * stats. While these records aren't generated with wal_level=minimal, + * stats also cannot be accessed during WAL replay. + */ + pgstat_report_stat(true); } else if (info == XLOG_INVALIDATIONS) { diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index 0cdb552631e..d743fc0b289 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -615,10 +615,21 @@ pgstat_report_stat(bool force) */ Assert(!pgStatLocal.shmem->is_shutdown); - now = GetCurrentTransactionStopTimestamp(); - - if (!force) + if (force) { + /* + * Stats reports are forced either when it's been too long since stats + * have been reported or in processes that force stats reporting to + * happen at specific points (including shutdown). In the former case + * the transaction stop time might be quite old, in the latter it + * would never get cleared. + */ + now = GetCurrentTimestamp(); + } + else + { + now = GetCurrentTransactionStopTimestamp(); + if (pending_since > 0 && TimestampDifferenceExceeds(pending_since, now, PGSTAT_MAX_INTERVAL)) { -- 2.38.0
