Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitee3f8d3

Browse files
committed
pgstat: Bring up pgstat in BaseInit() to fix uninitialized use of pgstat by AV.
Previously pgstat_initialize() was called in InitPostgres() andAuxiliaryProcessMain(). As it turns out there was at least one case where wereported stats before pgstat_initialize() was called, seeAutoVacWorkerMain()'s intentionally early call to pgstat_report_autovac().This turns out to not be a problem with the current pgstat implementation aspgstat_initialize() only registers a shutdown callback. But in the sharedmemory based stats implementation we are working towards pgstat_initialize()has to do more work.Afterb406478 BaseInit() is a central place where initialization shared bynormal backends and auxiliary backends can be put. Obviously BaseInit() iscalled before InitPostgres() registers ShutdownPostgres. PreviouslyShutdownPostgres was the first before_shmem_exit callback, now that's commonlypgstats. That should be fine.Previously pgstat_initialize() was not called in bootstrap mode, but theredoes not appear to be a need for that. It's now done unconditionally.To detect future issues like this, assertions are added to a few placesverifying that the pgstat subsystem is initialized and not yet shut down.Author: Andres Freund <andres@anarazel.de>Discussion:https://postgr.es/m/20210405092914.mmxqe7j56lsjfsej@alap3.anarazel.deDiscussion:https://postgr.es/m/20210802164124.ufo5buo4apl6yuvs@alap3.anarazel.de
1 parent5c056b0 commitee3f8d3

File tree

3 files changed

+65
-13
lines changed

3 files changed

+65
-13
lines changed

‎src/backend/postmaster/auxprocess.c

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,6 @@ AuxiliaryProcessMain(AuxProcType auxtype)
125125
*/
126126
CreateAuxProcessResourceOwner();
127127

128-
/* Initialize statistics reporting */
129-
pgstat_initialize();
130128

131129
/* Initialize backend status information */
132130
pgstat_beinit();

‎src/backend/postmaster/pgstat.c

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,15 @@ static List *pending_write_requests = NIL;
295295
*/
296296
staticinstr_timetotal_func_time;
297297

298+
/*
299+
* For assertions that check pgstat is not used before initialization / after
300+
* shutdown.
301+
*/
302+
#ifdefUSE_ASSERT_CHECKING
303+
staticboolpgstat_is_initialized= false;
304+
staticboolpgstat_is_shutdown= false;
305+
#endif
306+
298307

299308
/* ----------
300309
* Local function forward declarations
@@ -330,6 +339,7 @@ static void pgstat_send_connstats(bool disconnect, TimestampTz last_report);
330339
staticPgStat_TableStatus*get_tabstat_entry(Oidrel_id,boolisshared);
331340

332341
staticvoidpgstat_setup_memcxt(void);
342+
staticvoidpgstat_assert_is_up(void);
333343

334344
staticvoidpgstat_setheader(PgStat_MsgHdr*hdr,StatMsgTypemtype);
335345
staticvoidpgstat_send(void*msg,intlen);
@@ -854,6 +864,8 @@ pgstat_report_stat(bool disconnect)
854864
TabStatusArray*tsa;
855865
inti;
856866

867+
pgstat_assert_is_up();
868+
857869
/*
858870
* Don't expend a clock check if nothing to do.
859871
*
@@ -1960,6 +1972,8 @@ pgstat_init_function_usage(FunctionCallInfo fcinfo,
19601972
PgStat_BackendFunctionEntry*
19611973
find_funcstat_entry(Oidfunc_id)
19621974
{
1975+
pgstat_assert_is_up();
1976+
19631977
if (pgStatFunctions==NULL)
19641978
returnNULL;
19651979

@@ -2078,6 +2092,8 @@ get_tabstat_entry(Oid rel_id, bool isshared)
20782092
TabStatusArray*tsa;
20792093
boolfound;
20802094

2095+
pgstat_assert_is_up();
2096+
20812097
/*
20822098
* Create hash table if we don't have it already.
20832099
*/
@@ -2938,6 +2954,8 @@ pgstat_fetch_replslot(NameData slotname)
29382954
staticvoid
29392955
pgstat_shutdown_hook(intcode,Datumarg)
29402956
{
2957+
Assert(!pgstat_is_shutdown);
2958+
29412959
/*
29422960
* If we got as far as discovering our own database ID, we can report what
29432961
* we did to the collector. Otherwise, we'd be sending an invalid
@@ -2946,20 +2964,26 @@ pgstat_shutdown_hook(int code, Datum arg)
29462964
*/
29472965
if (OidIsValid(MyDatabaseId))
29482966
pgstat_report_stat(true);
2967+
2968+
#ifdefUSE_ASSERT_CHECKING
2969+
pgstat_is_shutdown= true;
2970+
#endif
29492971
}
29502972

29512973
/* ----------
29522974
* pgstat_initialize() -
29532975
*
2954-
*Initialize pgstats state, and set up our on-proc-exit hook.
2955-
*Called from InitPostgres and AuxiliaryProcessMain.
2976+
*Initialize pgstats state, and set up our on-proc-exit hook. Called from
2977+
*BaseInit().
29562978
*
29572979
*NOTE: MyDatabaseId isn't set yet; so the shutdown hook has to be careful.
29582980
* ----------
29592981
*/
29602982
void
29612983
pgstat_initialize(void)
29622984
{
2985+
Assert(!pgstat_is_initialized);
2986+
29632987
/*
29642988
* Initialize prevWalUsage with pgWalUsage so that pgstat_send_wal() can
29652989
* calculate how much pgWalUsage counters are increased by subtracting
@@ -2969,6 +2993,10 @@ pgstat_initialize(void)
29692993

29702994
/* Set up a process-exit hook to clean up */
29712995
on_shmem_exit(pgstat_shutdown_hook,0);
2996+
2997+
#ifdefUSE_ASSERT_CHECKING
2998+
pgstat_is_initialized= true;
2999+
#endif
29723000
}
29733001

29743002
/* ------------------------------------------------------------
@@ -3001,6 +3029,8 @@ pgstat_send(void *msg, int len)
30013029
{
30023030
intrc;
30033031

3032+
pgstat_assert_is_up();
3033+
30043034
if (pgStatSock==PGINVALID_SOCKET)
30053035
return;
30063036

@@ -3053,6 +3083,8 @@ pgstat_send_bgwriter(void)
30533083
/* We assume this initializes to zeroes */
30543084
staticconstPgStat_MsgBgWriterall_zeroes;
30553085

3086+
pgstat_assert_is_up();
3087+
30563088
/*
30573089
* This function can be called even if nothing at all has happened. In
30583090
* this case, avoid sending a completely empty message to the stats
@@ -4629,6 +4661,8 @@ backend_read_statsfile(void)
46294661
Oidinquiry_db;
46304662
intcount;
46314663

4664+
pgstat_assert_is_up();
4665+
46324666
/* already read it? */
46334667
if (pgStatDBHash)
46344668
return;
@@ -4765,6 +4799,17 @@ pgstat_setup_memcxt(void)
47654799
ALLOCSET_SMALL_SIZES);
47664800
}
47674801

4802+
/*
4803+
* Stats should only be reported after pgstat_initialize() and before
4804+
* pgstat_shutdown(). This check is put in a few central places to catch
4805+
* violations of this rule more easily.
4806+
*/
4807+
staticvoid
4808+
pgstat_assert_is_up(void)
4809+
{
4810+
Assert(pgstat_is_initialized&& !pgstat_is_shutdown);
4811+
}
4812+
47684813

47694814
/* ----------
47704815
* pgstat_clear_snapshot() -
@@ -4779,6 +4824,8 @@ pgstat_setup_memcxt(void)
47794824
void
47804825
pgstat_clear_snapshot(void)
47814826
{
4827+
pgstat_assert_is_up();
4828+
47824829
/* Release memory, if any was allocated */
47834830
if (pgStatLocalContext)
47844831
MemoryContextDelete(pgStatLocalContext);
@@ -5897,6 +5944,8 @@ pgstat_slru_name(int slru_idx)
58975944
staticinlinePgStat_MsgSLRU*
58985945
slru_entry(intslru_idx)
58995946
{
5947+
pgstat_assert_is_up();
5948+
59005949
/*
59015950
* The postmaster should never register any SLRU statistics counts; if it
59025951
* did, the counts would be duplicated into child processes via fork().

‎src/backend/utils/init/postinit.c

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -517,6 +517,14 @@ BaseInit(void)
517517
*/
518518
DebugFileOpen();
519519

520+
/*
521+
* Initialize statistics reporting. This needs to happen early to ensure
522+
* that pgstat's shutdown callback runs after the shutdown callbacks of
523+
* all subsystems that can produce stats (like e.g. transaction commits
524+
* can).
525+
*/
526+
pgstat_initialize();
527+
520528
/* Do local initialization of file, storage and buffer managers */
521529
InitFileAccess();
522530
InitSync();
@@ -646,10 +654,6 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
646654
/* Initialize portal manager */
647655
EnablePortalManager();
648656

649-
/* Initialize stats collection --- must happen before first xact */
650-
if (!bootstrap)
651-
pgstat_initialize();
652-
653657
/* Initialize status reporting */
654658
if (!bootstrap)
655659
pgstat_beinit();
@@ -662,11 +666,12 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
662666

663667
/*
664668
* Set up process-exit callback to do pre-shutdown cleanup. This is the
665-
* first before_shmem_exit callback we register; thus, this will be the
666-
* last thing we do before low-level modules like the buffer manager begin
667-
* to close down. We need to have this in place before we begin our first
668-
* transaction --- if we fail during the initialization transaction, as is
669-
* entirely possible, we need the AbortTransaction call to clean up.
669+
* one of the first before_shmem_exit callbacks we register; thus, this
670+
* will be one the last things we do before low-level modules like the
671+
* buffer manager begin to close down. We need to have this in place
672+
* before we begin our first transaction --- if we fail during the
673+
* initialization transaction, as is entirely possible, we need the
674+
* AbortTransaction call to clean up.
670675
*/
671676
before_shmem_exit(ShutdownPostgres,0);
672677

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp