public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
To: Melanie Plageman <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: Magnus Hagander <[email protected]>
Cc: Pg Hackers <[email protected]>
Cc: Lukas Fittl <[email protected]>
Cc: Thomas Munro <[email protected]>
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)
Date: Wed, 20 Jul 2022 09:50:50 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAAKRu_bmox3REN9xTJCcCji3MQ7XGiuQT8iY+JaYUX_oUmtZug@mail.gmail.com>
References: <CAAKRu_ZKgdacVqLjjfpp6zAnd_Ks9vTqU3UOh-JkPWvWMJ6KEw@mail.gmail.com>
<CAAKRu_Y2t8uOhMBU461foxJd_C0gTyND-wZzvE8d-FLFtJOksw@mail.gmail.com>
<CAAKRu_YbiJxFiEUi_d_UW_DWc=rnP4vJmBNgUHmXPr5awfQPfA@mail.gmail.com>
<CAAKRu_ZJkk1KUZS-KAaa3EZ2e1o2YD7w8-NF-po5dTHM7cF=Rg@mail.gmail.com>
<[email protected]>
<CAAKRu_Y_B9pLm1-b6A5CfRhuWEMMy2iMy1zNG0_3GhejD-ZsMg@mail.gmail.com>
<[email protected]>
<CAAKRu_YKc9e_=CZ4RB97Krx9TQb-zfNMoqdP45AYs5uZFqXeSA@mail.gmail.com>
<[email protected]>
<CAAKRu_bmox3REN9xTJCcCji3MQ7XGiuQT8iY+JaYUX_oUmtZug@mail.gmail.com>
Hi,
On 2022-07-14 18:44:48 -0400, Melanie Plageman wrote:
> Subject: [PATCH v26 1/4] Add BackendType for standalone backends
> Subject: [PATCH v26 2/4] Remove unneeded call to pgstat_report_wal()
LGTM.
> Subject: [PATCH v26 3/4] Track IO operation statistics
> @@ -978,8 +979,17 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
>
> bufBlock = isLocalBuf ? LocalBufHdrGetBlock(bufHdr) : BufHdrGetBlock(bufHdr);
>
> + if (isLocalBuf)
> + io_path = IOPATH_LOCAL;
> + else if (strategy != NULL)
> + io_path = IOPATH_STRATEGY;
> + else
> + io_path = IOPATH_SHARED;
Seems a bit ugly to have an if (isLocalBuf) just after an isLocalBuf ?.
> + /*
> + * When a strategy is in use, reused buffers from the strategy ring will
> + * be counted as allocations for the purposes of IO Operation statistics
> + * tracking.
> + *
> + * However, even when a strategy is in use, if a new buffer must be
> + * allocated from shared buffers and added to the ring, this is counted
> + * as a IOPATH_SHARED allocation.
> + */
There's a bit too much duplication between the paragraphs...
> @@ -628,6 +637,9 @@ pgstat_report_stat(bool force)
> /* flush database / relation / function / ... stats */
> partial_flush |= pgstat_flush_pending_entries(nowait);
>
> + /* flush IO Operations stats */
> + partial_flush |= pgstat_flush_io_ops(nowait);
Could you either add a note to the commit message that the stats file
version needs to be increased, or just iclude that in the patch.
> @@ -1427,8 +1445,10 @@ pgstat_read_statsfile(void)
> FILE *fpin;
> int32 format_id;
> bool found;
> + PgStat_BackendIOPathOps io_stats;
> const char *statfile = PGSTAT_STAT_PERMANENT_FILENAME;
> PgStat_ShmemControl *shmem = pgStatLocal.shmem;
> + PgStatShared_BackendIOPathOps *io_stats_shmem = &shmem->io_ops;
>
> /* shouldn't be called from postmaster */
> Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
> @@ -1486,6 +1506,22 @@ pgstat_read_statsfile(void)
> if (!read_chunk_s(fpin, &shmem->checkpointer.stats))
> goto error;
>
> + /*
> + * Read IO Operations stats struct
> + */
> + if (!read_chunk_s(fpin, &io_stats))
> + goto error;
> +
> + io_stats_shmem->stat_reset_timestamp = io_stats.stat_reset_timestamp;
> +
> + for (int i = 0; i < BACKEND_NUM_TYPES; i++)
> + {
> + PgStat_IOPathOps *stats = &io_stats.stats[i];
> + PgStatShared_IOPathOps *stats_shmem = &io_stats_shmem->stats[i];
> +
> + memcpy(stats_shmem->data, stats->data, sizeof(stats->data));
> + }
Why can't the data be read directly into shared memory?
> /*
> +void
> +pgstat_io_ops_snapshot_cb(void)
> +{
> + PgStatShared_BackendIOPathOps *all_backend_stats_shmem = &pgStatLocal.shmem->io_ops;
> + PgStat_BackendIOPathOps *all_backend_stats_snap = &pgStatLocal.snapshot.io_ops;
> +
> + for (int i = 0; i < BACKEND_NUM_TYPES; i++)
> + {
> + PgStatShared_IOPathOps *stats_shmem = &all_backend_stats_shmem->stats[i];
> + PgStat_IOPathOps *stats_snap = &all_backend_stats_snap->stats[i];
> +
> + LWLockAcquire(&stats_shmem->lock, LW_EXCLUSIVE);
Why acquire the same lock repeatedly for each type, rather than once for
the whole?
> + /*
> + * Use the lock in the first BackendType's PgStat_IOPathOps to protect the
> + * reset timestamp as well.
> + */
> + if (i == 0)
> + all_backend_stats_snap->stat_reset_timestamp = all_backend_stats_shmem->stat_reset_timestamp;
Which also would make this look a bit less awkward.
Starting to look pretty good...
- Andres
view thread (112+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox