Re: Use FD_CLOEXEC on ListenSockets (was Re: Refactoring backend fork+exec code) - Mailing list pgsql-hackers

From Heikki Linnakangas
Subject Re: Use FD_CLOEXEC on ListenSockets (was Re: Refactoring backend fork+exec code)
Date
Msg-id 9caed67f-f93e-5701-8c25-265a2b139ed0@iki.fi
Whole thread Raw
In response to Re: Use FD_CLOEXEC on ListenSockets (was Re: Refactoring backend fork+exec code)  (Michael Paquier <michael@paquier.xyz>)
Responses Re: Use FD_CLOEXEC on ListenSockets (was Re: Refactoring backend fork+exec code)
List pgsql-hackers
On 29/08/2023 01:28, Michael Paquier wrote:
> 
> In case you've not noticed:
> https://www.postgresql.org/message-id/ZOvvuQe0rdj2slA9@paquier.xyz
> But it does not really matter now ;)

Ah sorry, missed that thread.

>> Yes, so it seems. Syslogger is started before the ListenSockets array is
>> initialized, so its still all zeros. When syslogger is forked, the child
>> process tries to close all the listen sockets, which are all zeros. So
>> syslogger calls close(0) MAXLISTEN (64) times. Attached patch moves the
>> array initialization earlier.
> 
> Yep, I've reached the same conclusion.  Wouldn't it be cleaner to move
> the callback registration of CloseServerPorts() closer to the array
> initialization, though?

Ok, pushed that way.

I checked the history of this: it goes back to commit 9a86f03b4e in 
version 13. The SysLogger_Start() call used to be later, after setting p 
ListenSockets, but that commit moved it. So I backpatched this to v13.

>> The first close(0) actually does have an effect: it closes stdin, which is
>> fd 0. That is surely accidental, but I wonder if we should indeed close
>> stdin in child processes? The postmaster process doesn't do anything with
>> stdin either, although I guess a library might try to read a passphrase from
>> stdin before starting up, for example.
> 
> We would have heard about that, wouldn't we?  I may be missing
> something of course, but on HEAD, the array initialization is done
> before starting any child processes, and the syslogger is the first
> one forked.

Yes, syslogger is the only process that closes stdin. After moving the 
initialization, it doesn't close it either.

Thinking about this some more, the ListenSockets array is a bit silly 
anyway. We fill the array starting from index 0, always append to the 
end, and never remove entries from it. It would seem more 
straightforward to keep track of the used size of the array. Currently 
we always loop through the unused parts too, and e.g. 
ConfigurePostmasterWaitSet() needs to walk the array to count how many 
elements are in use.

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




pgsql-hackers by date:

Previous
From: "Drouvot, Bertrand"
Date:
Subject: Re: Autogenerate some wait events code and documentation
Next
From: Amit Kapila
Date:
Subject: Re: [PoC] pg_upgrade: allow to upgrade publisher node