Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 7fd89f4

Browse files
committed
Fix async.c to not register any SLRU stats counts in the postmaster.
Previously, AsyncShmemInit forcibly initialized the first page of the async SLRU, to save dealing with that case in asyncQueueAddEntries. But this is a poor tradeoff, since many installations do not ever use NOTIFY; for them, expending those cycles in AsyncShmemInit is a complete waste. Besides, this only saves a couple of instructions in asyncQueueAddEntries, which hardly seems likely to be measurable. The real reason to change this now, though, is that now that we track SLRU access stats, the existing code is causing the postmaster to accumulate some access counts, which then get inherited into child processes by fork(), messing up the statistics. Delaying the initialization into the first child that does a NOTIFY fixes that. Hence, we can revert f3d23d8, which was an incorrect attempt at fixing that issue. Also, add an Assert to pgstat.c that should catch any future errors of the same sort. Discussion: https://postgr.es/m/8367.1589391884@sss.pgh.pa.us
1 parent d82a505 commit 7fd89f4

File tree

2 files changed

+24
-14
lines changed

2 files changed

+24
-14
lines changed

src/backend/commands/async.c

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,10 @@ typedef struct QueuePosition
200200
} while (0)
201201

202202
#define QUEUE_POS_EQUAL(x,y) \
203-
((x).page == (y).page && (x).offset == (y).offset)
203+
((x).page == (y).page && (x).offset == (y).offset)
204+
205+
#define QUEUE_POS_IS_ZERO(x) \
206+
((x).page == 0 && (x).offset == 0)
204207

205208
/* choose logically smaller QueuePosition */
206209
#define QUEUE_POS_MIN(x,y) \
@@ -515,7 +518,6 @@ void
515518
AsyncShmemInit(void)
516519
{
517520
bool found;
518-
int slotno;
519521
Size size;
520522

521523
/*
@@ -562,13 +564,6 @@ AsyncShmemInit(void)
562564
* During start or reboot, clean out the pg_notify directory.
563565
*/
564566
(void) SlruScanDirectory(AsyncCtl, SlruScanDirCbDeleteAll, NULL);
565-
566-
/* Now initialize page zero to empty */
567-
LWLockAcquire(AsyncCtlLock, LW_EXCLUSIVE);
568-
slotno = SimpleLruZeroPage(AsyncCtl, QUEUE_POS_PAGE(QUEUE_HEAD));
569-
/* This write is just to verify that pg_notify/ is writable */
570-
SimpleLruWritePage(AsyncCtl, slotno);
571-
LWLockRelease(AsyncCtlLock);
572567
}
573568
}
574569

@@ -1470,9 +1465,21 @@ asyncQueueAddEntries(ListCell *nextNotify)
14701465
*/
14711466
queue_head = QUEUE_HEAD;
14721467

1473-
/* Fetch the current page */
1468+
/*
1469+
* If this is the first write since the postmaster started, we need to
1470+
* initialize the first page of the async SLRU. Otherwise, the current
1471+
* page should be initialized already, so just fetch it.
1472+
*
1473+
* (We could also take the first path when the SLRU position has just
1474+
* wrapped around, but re-zeroing the page is harmless in that case.)
1475+
*/
14741476
pageno = QUEUE_POS_PAGE(queue_head);
1475-
slotno = SimpleLruReadPage(AsyncCtl, pageno, true, InvalidTransactionId);
1477+
if (QUEUE_POS_IS_ZERO(queue_head))
1478+
slotno = SimpleLruZeroPage(AsyncCtl, pageno);
1479+
else
1480+
slotno = SimpleLruReadPage(AsyncCtl, pageno, true,
1481+
InvalidTransactionId);
1482+
14761483
/* Note we mark the page dirty before writing in it */
14771484
AsyncCtl->shared->page_dirty[slotno] = true;
14781485

src/backend/postmaster/pgstat.c

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2906,9 +2906,6 @@ pgstat_initialize(void)
29062906
MyBEEntry = &BackendStatusArray[MaxBackends + MyAuxProcType];
29072907
}
29082908

2909-
/* Initialize SLRU statistics to zero */
2910-
memset(&SLRUStats, 0, sizeof(SLRUStats));
2911-
29122909
/* Set up a process-exit hook to clean up */
29132910
on_shmem_exit(pgstat_beshutdown_hook, 0);
29142911
}
@@ -6727,6 +6724,12 @@ pgstat_slru_name(int slru_idx)
67276724
static inline PgStat_MsgSLRU *
67286725
slru_entry(int slru_idx)
67296726
{
6727+
/*
6728+
* The postmaster should never register any SLRU statistics counts; if it
6729+
* did, the counts would be duplicated into child processes via fork().
6730+
*/
6731+
Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
6732+
67306733
Assert((slru_idx >= 0) && (slru_idx < SLRU_NUM_ELEMENTS));
67316734

67326735
return &SLRUStats[slru_idx];

0 commit comments

Comments
 (0)