Re: shared-memory based stats collector

Fujii Masao <masao.fujii@oss.nttdata.com>

From: Fujii Masao <masao.fujii@oss.nttdata.com>

To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>

Cc: gkokolatos@protonmail.com, pgsql-hackers@lists.postgresql.org

Date: 2021-03-09T14:24:10Z

Lists: pgsql-hackers


On 2021/03/09 16:51, Kyotaro Horiguchi wrote:
> At Sat, 6 Mar 2021 00:32:07 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>>
>>
>> On 2021/03/05 17:18, Kyotaro Horiguchi wrote:
>>> At Thu, 21 Jan 2021 12:03:48 +0900 (JST), Kyotaro Horiguchi
>>> <horikyota.ntt@gmail.com> wrote in
>>>> Commit 960869da08 (database statistics) conflicted with this. Rebased.
>>>>
>>>> I'm concerned about the behavior that pgstat_update_connstats calls
>>>> GetCurrentTimestamp() every time stats update happens (with intervals
>>>> of 10s-60s in this patch). But I didn't change that design since that
>>>> happens with about 0.5s intervals in master and the rate is largely
>>>> reduced in this patch, to make this patch simpler.
>>> I stepped on my foot, and another commit coflicted. Just rebased.
>>
>> Thanks for rebasing the patches!
>>
>> I think that 0003 patch is self-contained and useful, for example
>> which
>> enables us to monitor archiver process in pg_stat_activity. So IMO
>> it's worth pusing 0003 patch firstly.
> 
> I'm not sure archiver process is worth realtime monitoring, but I
> agree that the patch makes it possible.  Anyway it is requried in this
> patchset and I'm happy to see it committed beforehand.
> 
> Thanks for the review.
> 
>> Here are the review comments for 0003 patch.
>>
>> +	/* Archiver process's latch */
>> +	Latch	   *archiverLatch;
>> +	/* Current shared estimate of appropriate spins_per_delay value */
>>
>> The last line in the above seems not necessary.
> 
> Oops. It seems like a garbage after a past rebasing. Removed.
> 
>> In proc.h, NUM_AUXILIARY_PROCS needs to be incremented.
> 
> Right.  Increased to 5 and rewrote the comment.
> 
>>      /* ----------
>>       * Functions called from postmaster
>>       * ----------
>>       */
>>      extern int	pgarch_start(void);
>>
>> In pgarch.h, the above is not necessary.
> 
> Removed.
> 
>> +extern void XLogArchiveWakeup(void);
>>
>> This seems no longer necessary.
>>
>> +extern void XLogArchiveWakeupStart(void);
>> +extern void XLogArchiveWakeupEnd(void);
>> +extern void XLogArchiveWakeup(void);
>>
>> These seem also no longer necessary.
> 
> Sorry for many garbages. Removed all of them.
> 
>>   			PgArchPID = 0;
>>   			if (!EXIT_STATUS_0(exitstatus))
>> -				LogChildExit(LOG, _("archiver process"),
>> -							 pid, exitstatus);
>> -			if (PgArchStartupAllowed())
>> -				PgArchPID = pgarch_start();
>> +				HandleChildCrash(pid, exitstatus,
>> +								 _("archiver process"));
>>
>> I don't think that we should treat non-zero exit condition as a crash,
>> as before. Otherwise when archive_command fails on a signal,
>> archiver emits FATAL error and which leads the server restart.
> 
> Sounds reasonable. Now archiver is treated the same way to wal
> receiver.  Specifically exit(1) doesn't cause server restart.

Thanks!

-			if (PgArchStartupAllowed())
-				PgArchPID = pgarch_start();

In the latest patch, why did you remove the code to restart new archiver
in reaper()? When archiver dies, I think new archiver should be restarted like
the current reaper() does. Otherwise, the restart of archiver can be
delayed until the next cycle of ServerLoop, which may take time.


> 
>> - * walwriter, autovacuum, or background worker.
>> + * walwriter, autovacuum, archiver or background worker.
>>    *
>>    * The objectives here are to clean up our local state about the child
>>    * process, and to signal all other remaining children to quickdie.
>> @@ -3609,6 +3606,18 @@ HandleChildCrash(int pid, int exitstatus, const
>> char *procname)
>>   		signal_child(AutoVacPID, (SendStop ? SIGSTOP : SIGQUIT));
>>   	}
>>   +	/* Take care of the archiver too */
>> +	if (pid == PgArchPID)
>> +		PgArchPID = 0;
>> +	else if (PgArchPID != 0 && take_action)
>> +	{
>> +		ereport(DEBUG2,
>> +				(errmsg_internal("sending %s to process %d",
>> +								 (SendStop ? "SIGSTOP" : "SIGQUIT"),
>> +								 (int) PgArchPID)));
>> +		signal_child(PgArchPID, (SendStop ? SIGSTOP : SIGQUIT));
>> +	}
>> +
>>
>> Same as above.
> 
> Mmm.  In the first place, I found that I forgot to remove existing
> code to handle archiver... Removed it instead of the above, which is
> added by this patch.  Since the process becomes an auxiliary process,
> no reason to differentiate from other auxiliary processes in handling?

Yes, ok.


> 
>> In xlogarchive.c, "#include "storage/pmsignal.h"" is no longer
>> necessary.
> 
> Removed.
> 
>> pgarch_forkexec() should be removed from pgarch.c because it's no
>> longer used.
> 
> Right. Removed. EXEC_BACKEND still fails for another reason of
> prototype mismatch of PgArchiverMain. Fixed it toghether.
> 
>>      /* ------------------------------------------------------------
>>       * Public functions called from postmaster follow
>>       * ------------------------------------------------------------
>>       */
>>
>> The definition of PgArchiverMain() should be placed just
>> after the above comment.
> 
> The module no longer have a function called from postmaster. Now
> PgArchiverMain() is placed just below "/* Main entry point...".
> 
>> exit(0) in PgArchiverMain() should be proc_exit(0)?
> 
> Yeah, proc_exit() says as it should be the only function to call
> exit() directly.
> 
> By the way, the patch (0003) removes the flag "wakened". The flag was
> originally added to prevent spurious wakeups (66ec2db7284). At the
> time the pg_ysleep in pgarch_MainLoop was replaced with WaitLatch by
> 89fd72cbf26, the flag survived but with losing its effect since
> WaitLatch doesn't get spurious wakeups (AFAICS).  So if the change
> (removal of "wakened") is correct, it might worth another patch.
> 
> I'll send new patchset soon.

I read v50_003 patch.

When archiver dies, ProcGlobal->archiverLatch should be reset to NULL,
like walreceiver does the similar thing in WalRcvDie()?


In pgarch.c, #include "postmaster/fork_process.h" seems no longer necessary.


+	if (strcmp(argv[1], "--forkarch") == 0)
+	{
+		/* Restore basic shared memory pointers */
+		InitShmemAccess(UsedShmemSegAddr);
+
+		/* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
+		InitAuxiliaryProcess();
+
+		/* Attach process to shared data structures */
+		CreateSharedMemoryAndSemaphores();
+
+		PgArchiverMain(); /* does not return */
+	}

Why is this necessary? I was thinking that "--forkboot" handles archiver
in SubPostmasterMain().

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Commits

Improve comment about dropped entries in pgstat.c
- a0ab20f16541 15.11 landed
- ec194b448cbc 16.7 landed
- bb93b33d7e39 17.3 landed
- 001a537b83ec 18.0 landed
Fix temporary memory leak in system table index scans
- 1acf10549e64 18.0 cited
pgstat: set timestamps of fixed-numbered stats after a crash.
- 5cd1c40b3ce9 15.0 landed
pgstat: Update docs to match the shared memory stats reality.
- b3abca68106d 15.0 landed
pgstat: Hide instability in stats.spec with -DCATCACHE_FORCE_RELEASE.
- d6c0db14836c 15.0 cited
pgstat: add/extend tests for resetting various kinds of stats.
- 5264add78478 15.0 landed
Add minimal tests for recovery conflict handling.
- 9f8a050f68dc 15.0 landed
pgstat: test stats interactions with physical replication.
- 53b9cd20d414 15.0 landed
pgstat: add tests for handling of restarts, including crashes.
- 16acf7f1aaea 15.0 landed
pgstat: add tests for transaction behaviour, 2PC, function stats.
- e349c95d3e91 15.0 landed
pgstat: add pg_stat_have_stats() test helper.
- ad401664b801 15.0 landed
pgstat: add pg_stat_force_next_flush(), use it to simplify tests.
- 0f96965c6581 15.0 landed
pgstat: move pgstat.c to utils/activity.
- fbfe6910eca0 15.0 landed
pgstat: store statistics in shared memory.
- 5891c7a8ed8f 15.0 landed
pgstat: remove stats_temp_directory.
- 6f0cf87872ab 15.0 landed
pgstat: rename STATS_COLLECTOR GUC group to STATS_CUMULATIVE.
- 1db4e5a4eeec 15.0 landed
pgstat: revise replication slot API in preparation for shared memory stats.
- e41aed674f35 15.0 landed
pgstat: scaffolding for transactional stats creation / drop.
- 8b1dccd37c71 15.0 landed
pgstat: introduce PgStat_Kind enum.
- 997afad89d12 15.0 landed
pgstat: prepare APIs used by pgstatfuncs for shared memory stats.
- 8fb580a35ce3 15.0 landed
pgstat: add pgstat_copy_relation_stats().
- 8ea7963fc741 15.0 landed
pgstat: rename some pgstat_send_* functions to pgstat_report_*.
- cc96373cf39b 15.0 landed
pgstat: stats collector references in comments.
- bdbd3d9064f9 15.0 landed
pgstat: move transactional code into pgstat_xact.c.
- ab62a642d52c 15.0 landed
dsm: allow use in single user mode.
- 46a2d2499a64 15.0 landed
dshash: revise sequential scan support.
- 909eebf27b9e 15.0 landed
pgstat: remove some superflous comments from pgstat.h.
- 55e566fc4bc8 15.0 landed
pgstat: reorder pgstat.[ch] contents.
- 315ae75e9b6d 15.0 landed
pgstat: split different types of stats into separate files.
- 13619598f108 15.0 landed
pgstat: introduce pgstat_relation_should_count().
- 8363102009d8 15.0 landed
pgstat: separate "xact level" handling out of relation specific functions.
- d4ba8b51c763 15.0 landed
pgstat: rename pgstat_initstats() to pgstat_relation_init().
- bff258a2732e 15.0 landed
pgstat: split out WAL handling from pgstat_{initialize,report_stat}.
- 78f9506b380f 15.0 landed
pgstat: run pgindent on pgstat.c/h.
- a3a75b982b5b 15.0 landed
pgstat: split relation, database handling out of pgstat_report_stat().
- 89c546c29489 15.0 landed
Move code around in StartupXLOG().
- be1c00ab13a7 15.0 cited
pgstat: Prepare to use mechanism for truncated rels also for droppped rels.
- 6b9501660c93 15.0 landed
pgstat: Split out relation stats handling from AtEO[Sub]Xact_PgStat() etc.
- e1f958d759ff 15.0 landed
pgstat: Schedule per-backend pgstat shutdown via before_shmem_exit().
- fb2c5028e635 15.0 landed
Schedule ShutdownXLOG() in single user mode using before_shmem_exit().
- a1bb3d5dbe6a 15.0 landed
Make parallel worker shutdown complete entirely via before_shmem_exit().
- fa91d4c91f28 15.0 landed
pgstat: Bring up pgstat in BaseInit() to fix uninitialized use of pgstat by AV.
- ee3f8d3d3aec 15.0 landed
pgstat: split reporting/fetching of bgwriter and checkpointer stats.
- 1bc8e7b0991c 15.0 landed
Split backend status and progress related functionality out of pgstat.c.
- e1025044cd4e 14.0 landed
Split wait event related code from pgstat.[ch] into wait_event.[ch].
- a333476b9251 14.0 landed
Make archiver process an auxiliary process.
- d75288fb27b8 14.0 landed
Force to send remaining WAL stats to the stats collector at walwriter exit.
- 33394ee6f243 14.0 cited
Add pg_stat_database counters for sessions and session time
- 960869da0803 14.0 cited
Collect statistics about SLRU caches
- 28cac71bd368 13.0 cited
Don't run atexit callbacks in quickdie signal handlers.
- 8e19a82640d3 12.0 cited
Create a "fast path" for acquiring weak relation locks.
- 3cba8999b343 9.2.0 cited