Subject: Re: shared-memory based stats collector
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, gkokolatos@protonmail.com
Cc: pgsql-hackers@lists.postgresql.org
References: <20210108.102434.253483097897189339.horikyota.ntt@gmail.com>
 <20210114.151425.2043098307171137735.horikyota.ntt@gmail.com>
 <20210121.120348.195305077048770640.horikyota.ntt@gmail.com>
 <20210305.171856.303779618961522117.horikyota.ntt@gmail.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <a36c4a1a-239e-e050-e7f5-f52c743438c6@oss.nttdata.com>
Date: Sat, 6 Mar 2021 00:32:07 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0)
 Gecko/20100101 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <20210305.171856.303779618961522117.horikyota.ntt@gmail.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/a36c4a1a-239e-e050-e7f5-f52c743438c6%40oss.nttdata.com>
Precedence: bulk


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.

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.

In proc.h, NUM_AUXILIARY_PROCS needs to be incremented.

     /* ----------
      * Functions called from postmaster
      * ----------
      */
     extern int	pgarch_start(void);

In pgarch.h, the above is not necessary.

+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.

  			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.

- * 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.


In xlogarchive.c, "#include "storage/pmsignal.h"" is no longer necessary.


pgarch_forkexec() should be removed from pgarch.c because it's no longer used.


     /* ------------------------------------------------------------
      * Public functions called from postmaster follow
      * ------------------------------------------------------------
      */

The definition of PgArchiverMain() should be placed just
after the above comment.


exit(0) in PgArchiverMain() should be proc_exit(0)?

Regards,

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