Horton

Use FD_CLOEXEC on ListenSockets (was Re: Refactoring backend fork+exec code)

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Andres Freund <andres@anarazel.de>, Thomas Munro <thomas.munro@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>, Tristan Partin <tristan@neon.tech>
Date: 2023-08-24T11:41:44Z
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.

Attachments

Focusing on this one patch in this series:

On 11/07/2023 01:50, Andres Freund wrote:
>> From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
>> Date: Mon, 12 Jun 2023 16:33:20 +0300
>> Subject: [PATCH 4/9] Use FD_CLOEXEC on ListenSockets
>>
>> We went through some effort to close them in the child process. Better to
>> not hand them down to the child process in the first place.
> 
> I think Thomas has a larger version of this patch:
> https://postgr.es/m/CA%2BhUKGKPNFcfBQduqof4-7C%3DavjcSfdkKBGvQoRuAvfocnvY0A%40mail.gmail.com

Hmm, no, that's a little different. Thomas added the FD_CLOEXEC option 
to the *accepted* socket in commit 1da569ca1f. That was part of that 
thread. This patch adds the option to the *listen* sockets. That was not 
discussed in that thread, but it's certainly in the same vein.

Thomas: What do you think of the attached?

On 11/07/2023 00:07, Tristan Partin wrote:
>> @@ -831,7 +834,8 @@ StreamConnection(pgsocket server_fd, Port *port)
>>  void
>>  StreamClose(pgsocket sock)
>>  {
>> -       closesocket(sock);
>> +       if (closesocket(sock) != 0)
>> +               elog(LOG, "closesocket failed: %m");
>>  }
>>
>>  /*
> 
> Do you think WARNING would be a more appropriate log level?

No, WARNING is for messages that you expect the client to receive. This 
failure is unexpected at the system level, the message is for the 
administrator. The distinction isn't always very clear, but LOG seems 
more appropriate in this case.

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