Horton

Re: Refactoring backend fork+exec code

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Alexander Lakhin <exclusion@gmail.com>, Tristan Partin <tristan@neon.tech>, Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>, Thomas Munro <thomas.munro@gmail.com>
Date: 2023-12-03T14:41:47Z
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 02/12/2023 05:00, Alexander Lakhin wrote:
> What bothered me additionally, is an error detected after server start. I
> couldn't see it without the patches applied. I mean, on HEAD I now see
> `make check` passing, but with the patches it fails:
> ...
> # parallel group (20 tests):  interval date numerology polygon box macaddr8 macaddr multirangetypes line timestamp
> timetz timestamptz time circle strings lseg inet md5 path point
> not ok 22    + strings                                  1048 ms
> # (test process exited with exit code 2)
> not ok 23    + md5                                      1052 ms
> # (test process exited with exit code 2)
> ...
> src/test/regress/log/postmaster.log contains:
> ==00:00:00:30.730 1713480== Syscall param write(buf) points to uninitialised byte(s)
> ==00:00:00:30.730 1713480==    at 0x5245A37: write (write.c:26)
> ==00:00:00:30.730 1713480==    by 0x51BBF6C: _IO_file_write@@GLIBC_2.2.5 (fileops.c:1180)
> ==00:00:00:30.730 1713480==    by 0x51BC84F: new_do_write (fileops.c:448)
> ==00:00:00:30.730 1713480==    by 0x51BC84F: _IO_new_file_xsputn (fileops.c:1254)
> ==00:00:00:30.730 1713480==    by 0x51BC84F: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1196)
> ==00:00:00:30.730 1713480==    by 0x51B1056: fwrite (iofwrite.c:39)
> ==00:00:00:30.730 1713480==    by 0x5540CF: internal_forkexec (postmaster.c:4526)
> ==00:00:00:30.730 1713480==    by 0x5543C0: bgworker_forkexec (postmaster.c:5624)
> ==00:00:00:30.730 1713480==    by 0x555477: do_start_bgworker (postmaster.c:5665)
> ==00:00:00:30.730 1713480==    by 0x555738: maybe_start_bgworkers (postmaster.c:5928)
> ==00:00:00:30.730 1713480==    by 0x556072: process_pm_pmsignal (postmaster.c:5080)
> ==00:00:00:30.730 1713480==    by 0x556610: ServerLoop (postmaster.c:1761)
> ==00:00:00:30.730 1713480==    by 0x557BE2: PostmasterMain (postmaster.c:1469)
> ==00:00:00:30.730 1713480==    by 0x47216B: main (main.c:198)
> ==00:00:00:30.730 1713480==  Address 0x1ffeffd8c0 is on thread 1's stack
> ==00:00:00:30.730 1713480==  in frame #4, created by internal_forkexec (postmaster.c:4482)
> ==00:00:00:30.730 1713480==
> ...

Ack, I see this too. I fixed it by adding MCXT_ALLOC_ZERO to the 
allocation of the BackendWorker struct. That's a little heavy-handed, 
like with the previous failures the uninitialized padding bytes are 
written to the file and read back, but not accessed after that. But it 
seems like the simplest fix. This isn't performance critical after all.

I also renamed the 'has_worker' field to 'has_bgworker', per Tristan's 
suggestion. Pushed with those changes.

Thanks for the reviews!

> 2023-12-02 05:14:30.751 MSK client backend[1713740] pg_regress/rangetypes FATAL:  terminating connection due to
> unexpected postmaster exit
> 2023-12-02 05:14:31.033 MSK client backend[1713734] pg_regress/numeric FATAL:  postmaster exited during a parallel
> transaction
> TRAP: failed Assert("!IsTransactionOrTransactionBlock()"), File: "pgstat.c", Line: 591, PID: 1713734
> 
> I haven't looked deeper yet, but it seems that we see two issues here (and
> Assert is not directly caused by the patches set.)

I have not been able to reproduce this one.

-- 
Heikki Linnakangas
Neon (https://neon.tech)