Re: shared-memory based stats collector

[Tomas Vondra]

On 11/29/18 1:18 PM, Alvaro Herrera wrote:
> On 2018-Nov-28, Tomas Vondra wrote:
> 
>>> v10-0004-Shared-memory-based-stats-collector.patch
>>>   updated not to touch guc.
>>> v10-0005-Remove-the-GUC-stats_temp_directory.patch
>>>   collected all guc-related changes.
>>>   updated not to break other programs.
>>> v10-0006-Split-out-backend-status-monitor-part-from-pgstat.patch
>>>   basebackup.c requires both bestats.h and pgstat.h
>>> v10-0007-Documentation-update.patch
>>>   small change related to 0005.
>>
>> I need to do a more thorough review of part 0006, but these patches
>> seems quite fine to me. I'd however merge 0007 into the other relevant
>> parts (it seems like a mix of docs changes for 0004, 0005 and 0006).
> 
> Looking at 0001 - 0003 it seems OK to keep each as separate commits, but
> I suggest to have 0004+0006 be a single commit, mostly because
> introducing a bunch of "new" code in 0004 and then moving it over to
> bestatus.c in 0006 makes "git blame" doubly painful.  And I think
> committing 0005 and not 0007 makes the documentation temporarily buggy,
> so I see no reason to think of this as two commits, one being 0004+0006
> and the other 0005+0007.  And even those could conceivably be pushed
> together instead of as a single patch.  (But be sure to push very early
> in your work day, to have plenty of time to deal with any resulting
> buildfarm problems.)
> 

Kyotaro-san, do you agree with committing the patch the way Alvaro
proposed? That is, 0001-0003 as separate commits, and 0004+0006 and
0005+0007 together. The plan seems reasonable to me.

FWIW I see cputube reports some build failures on Windows:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.26736#L3135

If I understand it correctly, it complains about this line in postmaster.c:

extern pgsocket pgStatSock;

which seems to only affect EXEC_BACKEND (including Win32). ISTM we
should get rid of all pgStatSock references, per the attached fix.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 0c84d58b0b..ff7313ffe4 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -512,7 +512,6 @@ typedef struct
 	PGPROC	   *AuxiliaryProcs;
 	PGPROC	   *PreparedXactProcs;
 	PMSignalData *PMSignalState;
-	InheritableSocket pgStatSock;
 	pid_t		PostmasterPid;
 	TimestampTz PgStartTime;
 	TimestampTz PgReloadTime;
@@ -5955,7 +5954,6 @@ extern slock_t *ShmemLock;
 extern slock_t *ProcStructLock;
 extern PGPROC *AuxiliaryProcs;
 extern PMSignalData *PMSignalState;
-extern pgsocket pgStatSock;
 extern pg_time_t first_syslogger_file_time;
 
 #ifndef WIN32
@@ -6008,8 +6006,6 @@ save_backend_variables(BackendParameters *param, Port *port,
 	param->AuxiliaryProcs = AuxiliaryProcs;
 	param->PreparedXactProcs = PreparedXactProcs;
 	param->PMSignalState = PMSignalState;
-	if (!write_inheritable_socket(&param->pgStatSock, pgStatSock, childPid))
-		return false;
 
 	param->PostmasterPid = PostmasterPid;
 	param->PgStartTime = PgStartTime;
@@ -6241,7 +6237,6 @@ restore_backend_variables(BackendParameters *param, Port *port)
 	AuxiliaryProcs = param->AuxiliaryProcs;
 	PreparedXactProcs = param->PreparedXactProcs;
 	PMSignalState = param->PMSignalState;
-	read_inheritable_socket(&pgStatSock, &param->pgStatSock);
 
 	PostmasterPid = param->PostmasterPid;
 	PgStartTime = param->PgStartTime;