Horton

Re: Refactoring backend fork+exec code

Tristan Partin <tristan@neon.tech>

View thread
From: "Tristan Partin" <tristan@neon.tech>
To: "Heikki Linnakangas" <hlinnaka@iki.fi>, "Alexander Lakhin" <exclusion@gmail.com>, "Andres Freund" <andres@anarazel.de>
Cc: "pgsql-hackers" <pgsql-hackers@postgresql.org>, "Thomas Munro" <thomas.munro@gmail.com>
Date: 2023-12-01T21:11:49Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Avoid 0-length memcpy to NULL with EXEC_BACKEND

  2. Tighten check for --forkchild argument when spawning child process

  3. Add missing PGDLLIMPORT markings

  4. Remove assertions that some compiler say are tautological

  5. Move code for backend startup to separate file

  6. Refactor postmaster child process launching

  7. Move some functions from postmaster.c to a new source file

  8. Split registration of Win32 deadchild callback to separate function

  9. Improve log messages referring to background worker processes

  10. Move initialization of the Port struct to the child process

  11. Pass CAC as an argument to the backend process

  12. Set socket options in child process after forking

  13. Fix references to renamed function in comments

  14. Use MyBackendType in more places to check what process this is

  15. Remove MyAuxProcType, use MyBackendType instead

  16. Replace BackendIds with 0-based ProcNumbers

  17. Redefine backend ID to be an index into the proc array

  18. Remove superfluous 'pgprocno' field from PGPROC

  19. Fix incorrect comment on how BackendStatusArray is indexed

  20. Refactor how InitProcess is called

  21. Pass BackgroundWorker entry in the parameter file in EXEC_BACKEND mode

  22. Refactor CreateSharedMemoryAndSemaphores

  23. Silence Valgrind complaint with EXEC_BACKEND

  24. Adjust obsolete comment explaining set_stack_base().

  25. Print lwlock stats also for aux processes, when built with LWLOCK_STATS

  26. Move canAcceptConnections check from ProcessStartupPacket to caller.

  27. Fix crash on syslogger startup

  28. Refactor ListenSocket array.

  29. Use FD_CLOEXEC on ListenSockets

  30. Don't leak descriptors into subprograms.

  31. Use WaitEventSet API for postmaster's event loop.

  32. Rearrange postmaster's startup sequence for better syslogger results.

  33. Fix and enhance the assertion of no palloc's in a critical section.

On Fri Dec 1, 2023 at 2:44 PM CST, Heikki Linnakangas wrote:
> On 01/12/2023 16:00, Alexander Lakhin wrote:
> > Maybe you could look at issues with running `make check` under Valgrind
> > when server built with CPPFLAGS="-DUSE_VALGRIND -DEXEC_BACKEND":
> > # +++ regress check in src/test/regress +++
> > # initializing database system by copying initdb template
> > # postmaster failed, examine ".../src/test/regress/log/postmaster.log" for the reason
> > Bail out!make[1]: ***
> > 
> > ...
> > 2023-12-01 16:48:39.136 MSK postmaster[1307988] LOG:  listening on Unix socket "/tmp/pg_regress-pPFNk0/.s.PGSQL.55312"
> > ==00:00:00:01.692 1259396== Syscall param write(buf) points to uninitialised byte(s)
> > ==00:00:00:01.692 1259396==    at 0x5245A37: write (write.c:26)
> > ==00:00:00:01.692 1259396==    by 0x51BBF6C: _IO_file_write@@GLIBC_2.2.5 (fileops.c:1180)
> > ==00:00:00:01.692 1259396==    by 0x51BC84F: new_do_write (fileops.c:448)
> > ==00:00:00:01.692 1259396==    by 0x51BC84F: _IO_new_file_xsputn (fileops.c:1254)
> > ==00:00:00:01.692 1259396==    by 0x51BC84F: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1196)
> > ==00:00:00:01.692 1259396==    by 0x51B1056: fwrite (iofwrite.c:39)
> > ==00:00:00:01.692 1259396==    by 0x552E21: internal_forkexec (postmaster.c:4518)
> > ==00:00:00:01.692 1259396==    by 0x5546A1: postmaster_forkexec (postmaster.c:4444)
> > ==00:00:00:01.692 1259396==    by 0x55471C: StartChildProcess (postmaster.c:5275)
> > ==00:00:00:01.692 1259396==    by 0x557B61: PostmasterMain (postmaster.c:1454)
> > ==00:00:00:01.692 1259396==    by 0x472136: main (main.c:198)
> > ==00:00:00:01.692 1259396==  Address 0x1ffeffdc11 is on thread 1's stack
> > ==00:00:00:01.692 1259396==  in frame #4, created by internal_forkexec (postmaster.c:4482)
> > ==00:00:00:01.692 1259396==
> > 
> > With memset(param, 0, sizeof(*param)); added at the beginning of
> > save_backend_variables(), server starts successfully, but then
> > `make check` fails with other Valgrind error.
>
> That's actually a pre-existing issue, I'm seeing that even on unpatched 
> 'master'.
>
> In a nutshell, the problem is that the uninitialized padding bytes in 
> BackendParameters are written to the file. When we read the file back, 
> we don't access the padding bytes, so that's harmless. But Valgrind 
> doesn't know that.
>
> On Windows, the file is created with 
> CreateFileMapping(INVALID_HANDLE_VALUE, ...) and we write the variables 
> directly to the mapping. If I understand the Windows API docs correctly, 
> it is guaranteed to be initialized to zeros, so we don't have this 
> problem on Windows, only on other platforms with EXEC_BACKEND. I think 
> it makes sense to clear the memory on other platforms too, since that's 
> what we do on Windows.
>
> Committed a fix with memset(). I'm not sure what our policy with 
> backpatching this kind of issues is. This goes back to all supported 
> versions, but given the lack of complaints, I chose to not backpatch.

Seems like a harmless think to backpatch. It is conceivable that someone 
might want to run Valgrind on something other than HEAD.

-- 
Tristan Partin
Neon (https://neon.tech)