Re: Ability to listen on two unix sockets

Honza Horak <hhorak(at)redhat(dot)com> writes:
> On 06/15/2012 05:40 PM, Honza Horak wrote:
>> I realized the patch has some difficulties -- namely the socket path in the data dir lock file, which currently uses one port for socket and the same for interface. So to allow users to use arbitrary port for all unix sockets, we'd need to add another line only for unix socket, which doesn't apply for other platforms. Or we could just say that the first socket will allways use the default port (PostPortNumber), which is a solution I prefer currently, but will be glad for any other opinion. This is also why there is still un-necesary string splitting in pg_ctl.c, which will be removed after the issue above is solved.

I did a review pass over this patch.

> This is an enhanced patch, which forbids using a port number in the
> first socket directory entry.

Well, not so much "forbids" as "silently ignores", which doesn't seem like
great user-interface design to me. If we're going to adopt this solution
I think we need it to throw an error instead of just ignoring the port
specification.

On the whole I prefer the solution you mention above: let's generalize
the postmaster.pid format (and pg_ctl) so that we don't need to assume
anything about port numbers matching up. The nearby discussion about
allowing listen_addresses to specify port number would break this
assumption anyway. If we just add two port numbers into postmaster.pid,
one for the Unix socket and one for the TCP port, we could get rid of
the problem entirely.

I read through the patch for awhile and noticed some other smaller
issues:

* the patch does not compile warning-free:

postgres.c: In function 'PostgresMain':
postgres.c:3669:3: warning: implicit declaration of function 'SplitUnixDirectories' [-Wimplicit-function-declaration]
varlena.c: In function 'SplitUnixDirectories':
varlena.c:2577:3: warning: suggest parentheses around assignment used as truth value [-Wparentheses]

* the changes in bootstrap.c and postgres.c won't compile at all when not
HAVE_UNIX_SOCKETS, since mainSocket is referred to even though it won't
be defined in such a case.

* I'm not especially thrilled with propagating SplitUnixDirectories calls
into those two places anyway, nor with the weird decision for
SplitUnixDirectories to return a separate "mainSocket" value. Perhaps
what would be most sensible is to attach an assign hook to the
unix_socket_directories GUC parameter that would automatically split the
string and store the components into a globally-visible List variable
(which could replace the globally-visible string value we have now).
Then the places that need to reference the "main socket" could just
look at the first list entry if any. Also, the hook could enforce valid
parameter syntax.

* Also, I'm inclined to think it will work better if SplitUnixDirectories
takes care of separating out the port number data, which it could return
in an integer list parallel to the string list of directory names. For
one thing, if you don't then the places that currently are looking at
"mainSocket" are going to need to duplicate the port-splitting logic.

* Also, CreateDataDirLockFile generally gets all its information about GUC
settings by looking directly at the GUCs (eg, DataDir). It seems
inconsistent to pass it just this one value rather than having it find the
info for itself. So on the whole I'd drop the bootstrap.c and postgres.c
changes altogether and do whatever's needful inside CreateLockFile, or
perhaps even better, not write the Unix socket info at file creation but
add it with AddToDataDirLockFile later.

* It might be a good idea to s/unixSocketName/unixSocketDir/ throughout
pqcomm.c, since AFAICS none of those variables are actually the full path
name of the socket file. Also, I'm a bit disturbed that StreamServerPort
is now being called with UnixSocketDirs in a lot of places; that's either
wrong or useless. Maybe we could pass NULL in the places where it's not
meant to be a sensible value?

* Having Lock_AF_UNIX pass back a socket path seems rather grotty.
Possibly better to move the UNIXSOCK_PATH call to its caller and just pass
it a sock_path, similar to Setup_AF_UNIX? The placement of the addition
to the sock_paths list seems a bit random (or at least undercommented),
too.

* In postmaster.c, is it really possible for UnixSocketDirs to be null?
I'm inclined to think that code path is unreachable, since guc.c
initializes the value to empty-string not null. But in any case,
unix_socket_directories being an empty string should specify creating
zero sockets, I think. We need to change the default value to be
DEFAULT_PGSOCKET_DIR, or empty on Windows. (Likewise, if we're going to
pass a socket file name to CreateLockFile, it should be a socket file
name, not something that might need to be replaced by
DEFAULT_PGSOCKET_DIR.)

* Not sure about adding an is_absolute_path() insistence as you have done
in postmaster.c. We never required that before. I'm also inclined to
think that the canonicalize_path work should be done in
SplitDirectoriesString not here.

regards, tom lane