[PATCH] Allow Postgres to pick an unused port to listen

Yurii Rashkovskii <yrashk@gmail.com> writes:

I would like to suggest a patch against master (although it may be worth
backporting it) that makes it possible to listen on any unused port.

I think this is a bad idea, mainly because this:

Instead, with this patch, one can specify `port` as `0` (the "wildcard"
port) and retrieve the assigned port from postmaster.pid

is a horrid way to find out what was picked, and yet there could
be no other.

Our existing design for this sort of thing is to let the testing
framework choose the port, and I don't really see what's wrong
with that approach. Yes, I know it's theoretically subject to
race conditions, but that hasn't seemed to be a problem in
practice. It's especially not a problem given that modern
testing practice tends to not open any TCP port at all, just
a Unix socket in a test-private directory, so that port
conflicts are a non-issue.

regards, tom lane

Hi Tom,

Thank you for your feedback. Below are my comments.

On Wed, Mar 29, 2023 at 6:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yurii Rashkovskii <yrashk@gmail.com> writes:

I would like to suggest a patch against master (although it may be worth
backporting it) that makes it possible to listen on any unused port.

I think this is a bad idea, mainly because this:

Instead, with this patch, one can specify `port` as `0` (the "wildcard"
port) and retrieve the assigned port from postmaster.pid

is a horrid way to find out what was picked, and yet there could
be no other.

Can you elaborate on why reading postmaster.pid is a horrid way to discover
the port, given that it is a pretty simple format with a fixed line number
for the port?

Our existing design for this sort of thing is to let the testing
framework choose the port, and I don't really see what's wrong
with that approach. Yes, I know it's theoretically subject to
race conditions, but that hasn't seemed to be a problem in
practice. It's especially not a problem given that modern
testing practice tends to not open any TCP port at all, just
a Unix socket in a test-private directory, so that port
conflicts are a non-issue.

I keep running into this race condition nearly daily, which is why I
proposed to address it with this patch. Yes, I know that one can get around
this with UNIX sockets,
but they have limited capabilities (not accessible outside of the local
machine, to begin with). Here's a real-world example of why I need to be
able to use TCP ports:

I have an extension that allows managing the lifecycle of [Docker/OCI]
containers, and it passes Postgres connection details to these containers
as environment variables.
These containers can now connect to Postgres using any program that can
communicate using the wire protocol. I test this functionality in an
automated test that is executed
concurrently with others. Testing that the extension can supply the correct
connection information to the containers is important.

If we forget the importance of testing this specific part of the
functionality for a bit, the rest of my issue can be _theoretically_
resolved by passing the UNIX socket in `PGHOST` instead.

However, it won't work in a typical Docker Desktop for macOS setup as it
utilizes a virtual machine, and therefore, I can't simply use a UNIX socket
between them.

Here's an example:

1. Create a UNIX socket listener:

```
socat unix-l:test.sock,fork system:'echo hello'
```

2. Verify that it works locally:

```
$ socat test.sock -
hello
```

3. Now, while being on macOS and using Docker Desktop, let Docker mount the
directory with the socket and try to connect it from there:

```
$ docker run -v /path/to/sock/dir:/sock -ti ubuntu bash
# apt update && apt install -y socat
# socat /sock/test.sock -
2023/03/29 23:34:48 socat[451] E connect(5, AF=1 "/sock/test.sock", 17):
Connection refused
```

I get that the UNIX socket around works for many cases, but it does not
work for mine. Hence the proposal. Allowing a (fairly common) practice of a
wildcard port with the discovery of it via
postmaster.pid resolves all the above concerns without having to resort to
a rather race-condition-prone way to pick a port (or a complicated way to
do so with proper locking).

--
http://omnigres.org
Yurii

Hi Tom,

On Wed, Mar 29, 2023 at 6:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yurii Rashkovskii <yrashk@gmail.com> writes:

I would like to suggest a patch against master (although it may be worth
backporting it) that makes it possible to listen on any unused port.

I think this is a bad idea, mainly because this:

Instead, with this patch, one can specify `port` as `0` (the "wildcard"
port) and retrieve the assigned port from postmaster.pid

is a horrid way to find out what was picked, and yet there could
be no other.

I answered you before (
/messages/by-id/CA+RLCQwYw-Er-E_RGNCDfA514w+1YL8HGhNstxX=y1gLAABFdA@mail.gmail.com),
but I am wondering whether you missed that response. I would really be
interested to learn why you think reading port from the pid file is a
"horrid way" to find out what was picked.

I've outlined my reasoning for this feature in detail in the referenced
message. Hope you can consider it.

--
http://omnigres.org
Yurii

On 2023-03-29 We 07:55, Tom Lane wrote:

Yurii Rashkovskii<yrashk@gmail.com> writes:

I would like to suggest a patch against master (although it may be worth
backporting it) that makes it possible to listen on any unused port.

I think this is a bad idea, mainly because this:

Instead, with this patch, one can specify `port` as `0` (the "wildcard"
port) and retrieve the assigned port from postmaster.pid

is a horrid way to find out what was picked, and yet there could
be no other.

Our existing design for this sort of thing is to let the testing
framework choose the port, and I don't really see what's wrong
with that approach. Yes, I know it's theoretically subject to
race conditions, but that hasn't seemed to be a problem in
practice. It's especially not a problem given that modern
testing practice tends to not open any TCP port at all, just
a Unix socket in a test-private directory, so that port
conflicts are a non-issue.

For TAP tests we have pretty much resolved the port collisions issue for
TCP ports too. See commit 9b4eafcaf4

Perhaps the OP could adapt that logic to his use case.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Hi Andrew,

On Fri, Apr 7, 2023, 7:07 p.m. Andrew Dunstan <andrew@dunslane.net> wrote:

On 2023-03-29 We 07:55, Tom Lane wrote:

Yurii Rashkovskii <yrashk@gmail.com> <yrashk@gmail.com> writes:

I would like to suggest a patch against master (although it may be worth
backporting it) that makes it possible to listen on any unused port.

I think this is a bad idea, mainly because this:

Instead, with this patch, one can specify `port` as `0` (the "wildcard"
port) and retrieve the assigned port from postmaster.pid

is a horrid way to find out what was picked, and yet there could
be no other.

Our existing design for this sort of thing is to let the testing
framework choose the port, and I don't really see what's wrong
with that approach. Yes, I know it's theoretically subject to
race conditions, but that hasn't seemed to be a problem in
practice. It's especially not a problem given that modern
testing practice tends to not open any TCP port at all, just
a Unix socket in a test-private directory, so that port
conflicts are a non-issue.

For TAP tests we have pretty much resolved the port collisions issue for
TCP ports too. See commit 9b4eafcaf4

Perhaps the OP could adapt that logic to his use case.

Thank you for referencing this commit. The point why I am suggesting my
patch is that I believe that my solution is a much better way to avoid
collisions in the first place. Implementing an algorithm similar to the one
in the referenced commit is error-pfone and can be difficult in
environments like shell script.

I'm trying to understand what's wrong with reading port from the pid file
(if Postgres writes information there, it's surely so that somebody can
read it, otherwise, why write it in the first placd)? The proposed solution
uses operating system's functionality to achieve collision-free mechanics
with none of the complexity introduced in the commit.

On Fri, Apr 7, 2023 at 5:34 PM Yurii Rashkovskii <yrashk@gmail.com> wrote:

I'm trying to understand what's wrong with reading port from the pid file (if Postgres writes information there, it's surely so that somebody can read it, otherwise, why write it in the first placd)? The proposed solution uses operating system's functionality to achieve collision-free mechanics with none of the complexity introduced in the commit.

I agree. We don't document the exact format of the postmaster.pid file
to my knowledge, but storage.sgml lists all the things it contains,
and runtime.sgml documents that the first line contains the postmaster
PID, so this is clearly not some totally opaque file that nobody
should ever touch. Consequently, I don't agree with Tom's statement
that this would be a "a horrid way to find out what was picked." There
is some question in my mind about whether this is a feature that we
want PostgreSQL to have, and if we do want it, there may be some room
for debate about how it's implemented, but I reject the idea that
extracting the port number from postmaster.pid is intrinsically a
terrible plan. It seems like a perfectly reasonable plan.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi Robert,

On Tue, Apr 11, 2023 at 12:54 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Apr 7, 2023 at 5:34 PM Yurii Rashkovskii <yrashk@gmail.com> wrote:

I'm trying to understand what's wrong with reading port from the pid

file (if Postgres writes information there, it's surely so that somebody
can read it, otherwise, why write it in the first placd)? The proposed
solution uses operating system's functionality to achieve collision-free
mechanics with none of the complexity introduced in the commit.

I agree. We don't document the exact format of the postmaster.pid file
to my knowledge, but storage.sgml lists all the things it contains,
and runtime.sgml documents that the first line contains the postmaster
PID, so this is clearly not some totally opaque file that nobody
should ever touch. Consequently, I don't agree with Tom's statement
that this would be a "a horrid way to find out what was picked." There
is some question in my mind about whether this is a feature that we
want PostgreSQL to have, and if we do want it, there may be some room
for debate about how it's implemented, but I reject the idea that
extracting the port number from postmaster.pid is intrinsically a
terrible plan. It seems like a perfectly reasonable plan.

I appreciate your support on the pid file concern. What questions do you
have about this feature with regard to its desirability and/or
implementation? I'd love to learn from your insight and address any of
those if I can.

--
Y.

On Tue, Apr 11, 2023 at 10:17 PM Yurii Rashkovskii <yrashk@gmail.com> wrote:

I appreciate your support on the pid file concern. What questions do you have about this feature with regard to its desirability and/or implementation? I'd love to learn from your insight and address any of those if I can.

I don't have any particularly specific concerns. But, you know, if a
bunch of other people, especially people already known the community
showed up on this thread to say "hey, I'd like that too" or "that
would be better than what we have now," well then that would make me
think "hey, we should probably move forward with this thing." But so
far the only people to comment are Tom and Andrew. Tom, in addition to
complaining about the PID file thing, also basically said that the
feature didn't seem necessary to him, and Andrew's comments seem to me
to suggest the same thing. So it kind of seems like you've convinced
zero people that this is a thing we should have, and that's not very
many.

It happens from time to time on this mailing list that somebody shows
up to propose a feature where I say to myself "hmm, that doesn't sound
like an intrinsically terrible idea, but it sounds like it might be
specific enough that only the person proposing it would ever use it."
For instance, someone might propose a new backslash command for psql
that runs an SQL query that produces some output which that person
finds useful. There is no big design problem there, but psql is
already pretty cluttered with commands that look like line noise, so
we shouldn't add a new one on the strength of one person wanting it.
Each feature, even if it's minor, has some cost. New releases need to
keep it working, which may mean that it needs a test, and then the
test is another thing that you have to keep working, and it also takes
time to run every time anyone does make check-world. These aren't big
costs and don't set a high bar for adding new features, but they do
mean, at least IMHO, that one person wanting a feature that isn't
obviously of general utility is not good enough. I think all of that
also applies to this feature.

I haven't reviewed the code in detail. It at least has some style
issues, but worrying about that seems premature.

I mostly just wanted to say that I disagreed with Tom about the
particular point on postmaster.pid, without really expressing an
opinion about anything else.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2023-04-12 We 11:01, Robert Haas wrote:

On Tue, Apr 11, 2023 at 10:17 PM Yurii Rashkovskii<yrashk@gmail.com> wrote:

I appreciate your support on the pid file concern. What questions do you have about this feature with regard to its desirability and/or implementation? I'd love to learn from your insight and address any of those if I can.

I don't have any particularly specific concerns. But, you know, if a
bunch of other people, especially people already known the community
showed up on this thread to say "hey, I'd like that too" or "that
would be better than what we have now," well then that would make me
think "hey, we should probably move forward with this thing." But so
far the only people to comment are Tom and Andrew. Tom, in addition to
complaining about the PID file thing, also basically said that the
feature didn't seem necessary to him, and Andrew's comments seem to me
to suggest the same thing. So it kind of seems like you've convinced
zero people that this is a thing we should have, and that's not very
many.

Not quite, I just suggested looking at a different approach.  I'm not
opposed to the idea in principle.

I agree with you that parsing the pid file shouldn't be hard or
unreasonable.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On Apr 12, 2023, at 8:01 AM, Robert Haas <robertmhaas@gmail.com> wrote:

"hey, I'd like that too"

I like the idea in principle. I have been developing a testing infrastructure in my spare time and would rather not duplicate Andrew's TAP logic. If we get this pushed down into the server itself, all the test infrastructure can use a single, shared solution.

As for the implementation, I just briefly scanned the patch.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Apr 7, 2023 at 5:07 AM Andrew Dunstan <andrew@dunslane.net> wrote:

For TAP tests we have pretty much resolved the port collisions issue for TCP ports too. See commit 9b4eafcaf4

The Cirrus config still has the following for the Windows tests:

# Avoids port conflicts between concurrent tap test runs
PG_TEST_USE_UNIX_SOCKETS: 1

Is that comment out of date, or would this proposal improve the
Windows situation too?

--Jacob

On Wed, 12 Apr 2023 at 11:02, Robert Haas <robertmhaas@gmail.com> wrote:

I mostly just wanted to say that I disagreed with Tom about the
particular point on postmaster.pid, without really expressing an
opinion about anything else.

I don't object to using the pid file as the mechanism -- but it is a
bit of an awkward UI for shell scripting. I imagine it would be handy
if pg_ctl had an option to just print the port number so you could get
it with a simple port=`pg_ctl -D <dir> status-port`

--
greg

On Wed, Apr 12, 2023 at 1:31 PM Greg Stark <stark@mit.edu> wrote:

I don't object to using the pid file as the mechanism -- but it is a
bit of an awkward UI for shell scripting. I imagine it would be handy
if pg_ctl had an option to just print the port number so you could get
it with a simple port=`pg_ctl -D <dir> status-port`

That's not a bad idea, and would provide some additional isolation to
reduce direct dependency on the PID file format.

However, awk 'NR==4' $PGDATA/postmaster.pid is hardly insanity. If it
can be done with a 5-character awk script, it's not too hard. The kind
of thing you're talking about is much more important with things like
pg_control or postgresql.conf that have much more complicated formats.
The format of the PID file is intentionally simple. But that's not to
say that I'm objecting.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Apr 12, 2023 at 1:31 PM Greg Stark <stark@mit.edu> wrote:

I don't object to using the pid file as the mechanism -- but it is a
bit of an awkward UI for shell scripting. I imagine it would be handy
if pg_ctl had an option to just print the port number so you could get
it with a simple port=`pg_ctl -D <dir> status-port`

That's not a bad idea, and would provide some additional isolation to
reduce direct dependency on the PID file format.

Yeah. My main concern here is with limiting our ability to change
the pidfile format in future. If we can keep the dependencies on that
localized to code we control, it'd be much better.

regards, tom lane

On Wed, Apr 12, 2023 at 1:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah. My main concern here is with limiting our ability to change
the pidfile format in future. If we can keep the dependencies on that
localized to code we control, it'd be much better.

I don't know if it's considered officially supported, but I often use
pg_ctl stop on a directory without worrying about whether I'm doing it
with the same server version that's running in that directory. I'd be
reluctant to break that property. So I bet our ability to modify the
file format is already quite limited.

But again, no issue with having a way for pg_ctl to fish the
information out of there.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Apr 12, 2023 at 1:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah. My main concern here is with limiting our ability to change
the pidfile format in future. If we can keep the dependencies on that
localized to code we control, it'd be much better.

I don't know if it's considered officially supported, but I often use
pg_ctl stop on a directory without worrying about whether I'm doing it
with the same server version that's running in that directory. I'd be
reluctant to break that property. So I bet our ability to modify the
file format is already quite limited.

IMO, the only aspect we consider "officially supported" for outside
use is that the first line contains the postmaster's PID. All the
rest is private (and has changed as recently as v10). Without
having actually checked the code, I think that "pg_ctl stop" relies
only on that aspect, or at least it could be made to do so at need.
So I think your example would survive other changes in the format.

I don't really want external code knowing that line 4 is the port,
because I foresee us breaking that someday --- what will happen
when we want to allow one postmaster to support multiple ports?
Maybe we'll decide that we don't have to reflect that in the
pidfile, but let's not constrain our decisions ahead of time.

regards, tom lane

On Wed, Apr 12, 2023 at 02:24:30PM -0400, Tom Lane wrote:

I don't really want external code knowing that line 4 is the port,
because I foresee us breaking that someday --- what will happen
when we want to allow one postmaster to support multiple ports?
Maybe we'll decide that we don't have to reflect that in the
pidfile, but let's not constrain our decisions ahead of time.

In the same fashion as something mentioned upthread, the format
portability would not matter much if all the information from the PID
file is wrapped around a pg_ctl command or something equivalent that
controls its output format, say:
pg_ctl -D $PGDATA --format={json,what_you_want} postmaster_file

To be more precise, storage.sgml documents the format of the PID file
in what seems like the correct order for each item, some of them being
empty depending on the setup.
--
Michael

Tom, Robert, Greg, Andrew,

On Thu, Apr 13, 2023 at 12:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Apr 12, 2023 at 1:31 PM Greg Stark <stark@mit.edu> wrote:

I don't object to using the pid file as the mechanism -- but it is a
bit of an awkward UI for shell scripting. I imagine it would be handy
if pg_ctl had an option to just print the port number so you could get
it with a simple port=`pg_ctl -D <dir> status-port`

That's not a bad idea, and would provide some additional isolation to
reduce direct dependency on the PID file format.

Yeah. My main concern here is with limiting our ability to change
the pidfile format in future. If we can keep the dependencies on that
localized to code we control, it'd be much better.

Thank you all for the feedback. It's quite useful. I think it is important
to separate this into two concerns:

1. Letting Postgres pick an unused port.
2. Retrieving the port it picked.

If I get this right, there's no significant opposition to (1) as this is
common functionality we're relying on. The most contention is around (2)
because I suggested using postmaster.pid
file, which may be considered private for the most part, at least for the
time being.

With this in mind, I still think that proceeding with (1) is a good idea,
as retrieving the port being listened on is still much easier than
involving a more complex lock file script. For example, on UNIX-like
systems, `lsof` can be typically used to do this:

```
# For IPv4
lsof -a -w -FPn -p $(head -n 1 postmaster.pid) -i4TCP -sTCP:LISTEN -P -n |
tail -n 1 | awk -F: '{print $NF}'
# For IPv6
lsof -a -w -FPn -p $(head -n 1postmaster.pid) -i6TCP -sTCP:LISTEN -P -n |
tail -n 1 | awk -F: '{print $NF}'
```

(There are also other tools that can be used to achieve much of the same)

On Windows, this can be done using PowerShell (and perhaps netstat, too):

```
# IPv4
PS> Get-NetTCPConnection -State Listen -OwningProcess (Get-Content
"postmaster.pid" -First 1) | Where-Object { $_.LocalAddress -notmatch ':' }
| Select-Object -ExpandProperty LocalPort
5432
PS> Get-NetTCPConnection -State Listen -OwningProcess (Get-Content
"postmaster.pid" -First 1) | Where-Object { $_.LocalAddress -match ':' } |
Select-Object -ExpandProperty LocalPort
5432
```

The above commands can be worked on to extract multiple ports should that
ever become a feature.

The bottom line is this decouples (1) from (2), and we can resolve them
separately if there's too much (understandable) hesitation to commit to a
particular approach to it (documenting postmaster.pid, changing its format,
amending pg_ctl functionality, etc.) I will be happy to participate in the
discovery and resolution of (2) as well.

This would allow people like myself or Mark (above in the thread) to let
Postgres pick the unused port and extract it using a oneliner for the time
being. When a better approach for server introspection will be agreed on,
we can use that.

I'll be happy to address any [styling or other] issues with the currently
proposed patch.

--
http://omnigres.org
Yurii

Yurii Rashkovskii <yrashk@gmail.com> writes:

Thank you all for the feedback. It's quite useful. I think it is important
to separate this into two concerns:

1. Letting Postgres pick an unused port.
2. Retrieving the port it picked.

Yeah, those are distinguishable implementation concerns, but ...

The bottom line is this decouples (1) from (2), and we can resolve them
separately if there's too much (understandable) hesitation to commit to a
particular approach to it (documenting postmaster.pid, changing its format,
amending pg_ctl functionality, etc.)

... AFAICS, there is exactly zero value in committing a solution for (1)
without also committing a solution for (2). I don't think any of the
alternative methods you proposed are attractive or things we should
recommend.

regards, tom lane