On Wed, Jul 20, 2022 at 12:50 PM Andres Freund <and...@anarazel.de> wrote:
> > On 2022-07-14 18:44:48 -0400, Melanie Plageman wrote: > > > @@ -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? > > It is not the same lock. Each PgStatShared_IOPathOps has a lock so that they can be accessed individually (per BackendType in PgStatShared_BackendIOPathOps). It is optimized for the more common operation of flushing at the expense of the snapshot operation (which should be less common) and reset operation. > > +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? > > This is also because of having a LWLock in each PgStatShared_IOPathOps. Because I don't want a lock in the backend local stats, I have two data structures PgStatShared_IOPathOps and PgStat_IOPathOps. I thought it was odd to write out the lock to the file, so when persisting the stats, I write out the relevant data only and when reading it back in to shared memory, I read in the data member of PgStatShared_IOPathOps.