Improve management of SLRU statistics collection.

tglsfdc · tglsfdc · commit 81ca8686305c · 2020-05-13T13:08:23.000-04:00
Instead of re-identifying which statistics bucket to use for a given SLRU on every counter increment, do it once during shmem initialization. This saves a fair number of cycles, and there's no real cost because we could not have a bucket assignment that varies over time or across backends anyway. Also, get rid of the ill-considered decision to let pgstat.c pry directly into SLRU's shared state; it's cleaner just to have slru.c pass the stats bucket number. In consequence of these changes, there's no longer any need to store an SLRU's LWLock tranche info in shared memory, so get rid of that, making this a net reduction in shmem consumption. (That partly reverts fe702a7.) This is basically code review for 28cac71, so I also cleaned up some comments, removed a dangling extern declaration, fixed some things that should be static and/or const, etc. Discussion: https://postgr.es/m/3618.1589313035@sss.pgh.pa.us
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
@@ -191,6 +191,8 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
 
 		/* shared->latest_page_number will be set later */
 
+		shared->slru_stats_idx = pgstat_slru_index(name);
+
 		ptr = (char *) shared;
 		offset = MAXALIGN(sizeof(SlruSharedData));
 		shared->page_buffer = (char **) (ptr + offset);
@@ -214,15 +216,11 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
 			offset += MAXALIGN(nslots * nlsns * sizeof(XLogRecPtr));
 		}
 
-		Assert(strlen(name) + 1 < SLRU_MAX_NAME_LENGTH);
-		strlcpy(shared->lwlock_tranche_name, name, SLRU_MAX_NAME_LENGTH);
-		shared->lwlock_tranche_id = tranche_id;
-
 		ptr += BUFFERALIGN(offset);
 		for (slotno = 0; slotno < nslots; slotno++)
 		{
 			LWLockInitialize(&shared->buffer_locks[slotno].lock,
-							 shared->lwlock_tranche_id);
+							 tranche_id);
 
 			shared->page_buffer[slotno] = ptr;
 			shared->page_status[slotno] = SLRU_PAGE_EMPTY;
@@ -238,8 +236,7 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
 		Assert(found);
 
 	/* Register SLRU tranche in the main tranches array */
-	LWLockRegisterTranche(shared->lwlock_tranche_id,
-						  shared->lwlock_tranche_name);
+	LWLockRegisterTranche(tranche_id, name);
 
 	/*
 	 * Initialize the unshared control struct, including directory path. We
@@ -287,7 +284,7 @@ SimpleLruZeroPage(SlruCtl ctl, int pageno)
 	shared->latest_page_number = pageno;
 
 	/* update the stats counter of zeroed pages */
-	pgstat_count_slru_page_zeroed(ctl);
+	pgstat_count_slru_page_zeroed(shared->slru_stats_idx);
 
 	return slotno;
 }
@@ -408,7 +405,7 @@ SimpleLruReadPage(SlruCtl ctl, int pageno, bool write_ok,
 			SlruRecentlyUsed(shared, slotno);
 
 			/* update the stats counter of pages found in the SLRU */
-			pgstat_count_slru_page_hit(ctl);
+			pgstat_count_slru_page_hit(shared->slru_stats_idx);
 
 			return slotno;
 		}
@@ -453,7 +450,7 @@ SimpleLruReadPage(SlruCtl ctl, int pageno, bool write_ok,
 		SlruRecentlyUsed(shared, slotno);
 
 		/* update the stats counter of pages not found in SLRU */
-		pgstat_count_slru_page_read(ctl);
+		pgstat_count_slru_page_read(shared->slru_stats_idx);
 
 		return slotno;
 	}
@@ -493,7 +490,7 @@ SimpleLruReadPage_ReadOnly(SlruCtl ctl, int pageno, TransactionId xid)
 			SlruRecentlyUsed(shared, slotno);
 
 			/* update the stats counter of pages found in the SLRU */
-			pgstat_count_slru_page_hit(ctl);
+			pgstat_count_slru_page_hit(shared->slru_stats_idx);
 
 			return slotno;
 		}
@@ -612,7 +609,7 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int pageno)
 	off_t		endpos;
 
 	/* update the stats counter of checked pages */
-	pgstat_count_slru_page_exists(ctl);
+	pgstat_count_slru_page_exists(ctl->shared->slru_stats_idx);
 
 	SlruFileName(ctl, path, segno);
 
@@ -749,7 +746,7 @@ SlruPhysicalWritePage(SlruCtl ctl, int pageno, int slotno, SlruFlush fdata)
 	int			fd = -1;
 
 	/* update the stats counter of written pages */
-	pgstat_count_slru_page_written(ctl);
+	pgstat_count_slru_page_written(shared->slru_stats_idx);
 
 	/*
 	 * Honor the write-WAL-before-data rule, if appropriate, so that we do not
@@ -1147,7 +1144,7 @@ SimpleLruFlush(SlruCtl ctl, bool allow_redirtied)
 	bool		ok;
 
 	/* update the stats counter of flushes */
-	pgstat_count_slru_flush(ctl);
+	pgstat_count_slru_flush(shared->slru_stats_idx);
 
 	/*
 	 * Find and write dirty pages
@@ -1211,7 +1208,7 @@ SimpleLruTruncate(SlruCtl ctl, int cutoffPage)
 	int			slotno;
 
 	/* update the stats counter of truncates */
-	pgstat_count_slru_truncate(ctl);
+	pgstat_count_slru_truncate(shared->slru_stats_idx);
 
 	/*
 	 * The cutoff point is the start of the segment containing cutoffPage.
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
@@ -142,24 +142,30 @@ char	   *pgstat_stat_tmpname = NULL;
 PgStat_MsgBgWriter BgWriterStats;
 
 /*
- * SLRU statistics counters (unused in other processes) stored directly in
- * stats structure so it can be sent without needing to copy things around.
- * We assume this inits to zeroes. There is no central registry of SLRUs,
- * so we use this fixed list instead.
- *
- * There's a separte entry for each SLRU we have. The "other" entry is used
- * for all SLRUs without an explicit entry (e.g. SLRUs in extensions).
- */
-static char *slru_names[] = {"async", "clog", "commit_timestamp",
-							  "multixact_offset", "multixact_member",
-							  "oldserxid", "subtrans",
-							  "other" /* has to be last */};
+ * List of SLRU names that we keep stats for.  There is no central registry of
+ * SLRUs, so we use this fixed list instead.  The "other" entry is used for
+ * all SLRUs without an explicit entry (e.g. SLRUs in extensions).
+ */
+static const char *const slru_names[] = {
+	"async",
+	"clog",
+	"commit_timestamp",
+	"multixact_offset",
+	"multixact_member",
+	"oldserxid",
+	"subtrans",
+	"other"						/* has to be last */
+};
+
+#define SLRU_NUM_ELEMENTS	lengthof(slru_names)
 
-/* number of elemenents of slru_name array */
-#define SLRU_NUM_ELEMENTS	(sizeof(slru_names) / sizeof(char *))
-
-/* entries in the same order as slru_names */
-PgStat_MsgSLRU SLRUStats[SLRU_NUM_ELEMENTS];
+/*
+ * SLRU statistics counts waiting to be sent to the collector.  These are
+ * stored directly in stats message format so they can be sent without needing
+ * to copy things around.  We assume this variable inits to zeroes.  Entries
+ * are one-to-one with slru_names[].
+ */
+static PgStat_MsgSLRU SLRUStats[SLRU_NUM_ELEMENTS];
 
 /* ----------
  * Local data
@@ -4411,12 +4417,10 @@ pgstat_send_bgwriter(void)
 static void
 pgstat_send_slru(void)
 {
-	int		i;
-
 	/* We assume this initializes to zeroes */
 	static const PgStat_MsgSLRU all_zeroes;
 
-	for (i = 0; i < SLRU_NUM_ELEMENTS; i++)
+	for (int i = 0; i < SLRU_NUM_ELEMENTS; i++)
 	{
 		/*
 		 * This function can be called even if nothing at all has happened. In
@@ -6705,15 +6709,13 @@ pgstat_slru_index(const char *name)
  * in which case this returns NULL. This allows writing code that does not
  * know the number of entries in advance.
  */
-char *
-pgstat_slru_name(int idx)
+const char *
+pgstat_slru_name(int slru_idx)
 {
-	Assert(idx >= 0);
-
-	if (idx >= SLRU_NUM_ELEMENTS)
+	if (slru_idx < 0 || slru_idx >= SLRU_NUM_ELEMENTS)
 		return NULL;
 
-	return slru_names[idx];
+	return slru_names[slru_idx];
 }
 
 /*
@@ -6722,54 +6724,56 @@ pgstat_slru_name(int idx)
  * Returns pointer to entry with counters for given SLRU (based on the name
  * stored in SlruCtl as lwlock tranche name).
  */
-static PgStat_MsgSLRU *
-slru_entry(SlruCtl ctl)
+static inline PgStat_MsgSLRU *
+slru_entry(int slru_idx)
 {
-	int		idx = pgstat_slru_index(ctl->shared->lwlock_tranche_name);
-
-	Assert((idx >= 0) && (idx < SLRU_NUM_ELEMENTS));
+	Assert((slru_idx >= 0) && (slru_idx < SLRU_NUM_ELEMENTS));
 
-	return &SLRUStats[idx];
+	return &SLRUStats[slru_idx];
 }
 
+/*
+ * SLRU statistics count accumulation functions --- called from slru.c
+ */
+
 void
-pgstat_count_slru_page_zeroed(SlruCtl ctl)
+pgstat_count_slru_page_zeroed(int slru_idx)
 {
-	slru_entry(ctl)->m_blocks_zeroed += 1;
+	slru_entry(slru_idx)->m_blocks_zeroed += 1;
 }
 
 void
-pgstat_count_slru_page_hit(SlruCtl ctl)
+pgstat_count_slru_page_hit(int slru_idx)
 {
-	slru_entry(ctl)->m_blocks_hit += 1;
+	slru_entry(slru_idx)->m_blocks_hit += 1;
 }
 
 void
-pgstat_count_slru_page_exists(SlruCtl ctl)
+pgstat_count_slru_page_exists(int slru_idx)
 {
-	slru_entry(ctl)->m_blocks_exists += 1;
+	slru_entry(slru_idx)->m_blocks_exists += 1;
 }
 
 void
-pgstat_count_slru_page_read(SlruCtl ctl)
+pgstat_count_slru_page_read(int slru_idx)
 {
-	slru_entry(ctl)->m_blocks_read += 1;
+	slru_entry(slru_idx)->m_blocks_read += 1;
 }
 
 void
-pgstat_count_slru_page_written(SlruCtl ctl)
+pgstat_count_slru_page_written(int slru_idx)
 {
-	slru_entry(ctl)->m_blocks_written += 1;
+	slru_entry(slru_idx)->m_blocks_written += 1;
 }
 
 void
-pgstat_count_slru_flush(SlruCtl ctl)
+pgstat_count_slru_flush(int slru_idx)
 {
-	slru_entry(ctl)->m_flush += 1;
+	slru_entry(slru_idx)->m_flush += 1;
 }
 
 void
-pgstat_count_slru_truncate(SlruCtl ctl)
+pgstat_count_slru_truncate(int slru_idx)
 {
-	slru_entry(ctl)->m_truncate += 1;
+	slru_entry(slru_idx)->m_truncate += 1;
 }
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
@@ -1739,7 +1739,7 @@ pg_stat_get_slru(PG_FUNCTION_ARGS)
 		Datum		values[PG_STAT_GET_SLRU_COLS];
 		bool		nulls[PG_STAT_GET_SLRU_COLS];
 		PgStat_SLRUStats	stat = stats[i];
-		char	   *name;
+		const char *name;
 
 		name = pgstat_slru_name(i);
 
diff --git a/src/include/access/slru.h b/src/include/access/slru.h
@@ -32,9 +32,6 @@
  */
 #define SLRU_PAGES_PER_SEGMENT	32
 
-/* Maximum length of an SLRU name */
-#define SLRU_MAX_NAME_LENGTH	32
-
 /*
  * Page status codes.  Note that these do not include the "dirty" bit.
  * page_dirty can be true only in the VALID or WRITE_IN_PROGRESS states;
@@ -68,6 +65,7 @@ typedef struct SlruSharedData
 	bool	   *page_dirty;
 	int		   *page_number;
 	int		   *page_lru_count;
+	LWLockPadded *buffer_locks;
 
 	/*
 	 * Optional array of WAL flush LSNs associated with entries in the SLRU
@@ -98,10 +96,8 @@ typedef struct SlruSharedData
 	 */
 	int			latest_page_number;
 
-	/* LWLocks */
-	int			lwlock_tranche_id;
-	char		lwlock_tranche_name[SLRU_MAX_NAME_LENGTH];
-	LWLockPadded *buffer_locks;
+	/* SLRU's index for statistics purposes (might not be unique) */
+	int			slru_stats_idx;
 } SlruSharedData;
 
 typedef SlruSharedData *SlruShared;
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
@@ -11,7 +11,6 @@
 #ifndef PGSTAT_H
 #define PGSTAT_H
 
-#include "access/slru.h"
 #include "datatype/timestamp.h"
 #include "libpq/pqcomm.h"
 #include "miscadmin.h"
@@ -438,7 +437,7 @@ typedef struct PgStat_MsgBgWriter
 } PgStat_MsgBgWriter;
 
 /* ----------
- * PgStat_MsgSLRU			Sent by the SLRU to update statistics.
+ * PgStat_MsgSLRU			Sent by a backend to update SLRU statistics.
  * ----------
  */
 typedef struct PgStat_MsgSLRU
@@ -1260,11 +1259,6 @@ extern char *pgstat_stat_filename;
  */
 extern PgStat_MsgBgWriter BgWriterStats;
 
-/*
- * SLRU statistics counters are updated directly by slru.
- */
-extern PgStat_MsgSLRU SlruStats[];
-
 /*
  * Updated by pgstat_count_buffer_*_time macros
  */
@@ -1480,14 +1474,14 @@ extern PgStat_ArchiverStats *pgstat_fetch_stat_archiver(void);
 extern PgStat_GlobalStats *pgstat_fetch_global(void);
 extern PgStat_SLRUStats *pgstat_fetch_slru(void);
 
-extern void pgstat_count_slru_page_zeroed(SlruCtl ctl);
-extern void pgstat_count_slru_page_hit(SlruCtl ctl);
-extern void pgstat_count_slru_page_read(SlruCtl ctl);
-extern void pgstat_count_slru_page_written(SlruCtl ctl);
-extern void pgstat_count_slru_page_exists(SlruCtl ctl);
-extern void pgstat_count_slru_flush(SlruCtl ctl);
-extern void pgstat_count_slru_truncate(SlruCtl ctl);
-extern char *pgstat_slru_name(int idx);
-extern int pgstat_slru_index(const char *name);
+extern void pgstat_count_slru_page_zeroed(int slru_idx);
+extern void pgstat_count_slru_page_hit(int slru_idx);
+extern void pgstat_count_slru_page_read(int slru_idx);
+extern void pgstat_count_slru_page_written(int slru_idx);
+extern void pgstat_count_slru_page_exists(int slru_idx);
+extern void pgstat_count_slru_flush(int slru_idx);
+extern void pgstat_count_slru_truncate(int slru_idx);
+extern const char *pgstat_slru_name(int slru_idx);
+extern int	pgstat_slru_index(const char *name);
 
 #endif							/* PGSTAT_H */
