listening addresses

Andrew Dunstan <andrew@dunslane.net> writes:

Submitted for review.

Okay, some random comments:

! char *ListenAddresses = "localhost";

I think you made this mistake in the log_line_prefix patch too. The
contents of a GUC string var should always be either NULL or a pointer
to a malloc'd string --- ergo, its initial state must be NULL. It's
pure luck that GUC doesn't dump core trying to free() this pointer.

+ 	/* 
+ 	 * check if ListenAddresses is empty or all spaces
+ 	 */

Why do you need this test (or the NetServer bool) at all? Just scan
the string and bind to whatever it mentions.

BTW it'd be better to use isspace() instead of a hardwired test for ' '.

! if (strcmp(ListenAddresses,"*") != 0)

This seems a bit nonrobust since it will fail if any whitespace is
added. I think it'd be better to test whether any individual hostname
extracted from the string is '*'.

+ 	if (ListenSocket[0] == -1)
+ 		ereport(FATAL,
+ 				(errmsg("not listening on any socket")));

Good but I don't like the wording of the error very much. Maybe "could
not bind to any socket"? Doesn't seem quite right either. [thinks...]
There are really two cases here:
* listen_addresses is empty and the machine doesn't have Unix
sockets
* listen_addresses contains (only) bogus addresses, and the
machine doesn't have Unix sockets.
"not listening on any socket" seems to focus on the first case, "could
not bind to any socket" focuses on the second. Can we cover both?

Please revise, and update the docs too.

regards, tom lane

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Submitted for review.

Okay, some random comments:

! char *ListenAddresses = "localhost";

I think you made this mistake in the log_line_prefix patch too. The
contents of a GUC string var should always be either NULL or a pointer
to a malloc'd string --- ergo, its initial state must be NULL. It's
pure luck that GUC doesn't dump core trying to free() this pointer.

Noted. Thanks.

+ 	/* 
+ 	 * check if ListenAddresses is empty or all spaces
+ 	 */

Why do you need this test (or the NetServer bool) at all? Just scan
the string and bind to whatever it mentions.

It is used in the existing code to test if we can do SSL.

BTW it'd be better to use isspace() instead of a hardwired test for ' '.

Again, the existing code for VirtualHost uses hardcoded space. I don't
mind adjusting it, but was following style in the surrounding code.

! if (strcmp(ListenAddresses,"*") != 0)

This seems a bit nonrobust since it will fail if any whitespace is
added. I think it'd be better to test whether any individual hostname
extracted from the string is '*'.

Agree with the first point, disagree with the second. What does it mean
to specify "12.34.56.78 *"? I think "*" should be allowed only if it
is the only entry in the list.

+ 	if (ListenSocket[0] == -1)
+ 		ereport(FATAL,
+ 				(errmsg("not listening on any socket")));

Good but I don't like the wording of the error very much. Maybe "could
not bind to any socket"? Doesn't seem quite right either. [thinks...]
There are really two cases here:
* listen_addresses is empty and the machine doesn't have Unix
sockets
* listen_addresses contains (only) bogus addresses, and the
machine doesn't have Unix sockets.
"not listening on any socket" seems to focus on the first case, "could
not bind to any socket" focuses on the second. Can we cover both?

I will think about it. Bear in mind that they will have already had
warnings about specific failures.

Please revise, and update the docs too.

Will do.

Thanks

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

Tom Lane wrote:
+ * check if ListenAddresses is empty or all spaces

Why do you need this test (or the NetServer bool) at all? Just scan
the string and bind to whatever it mentions.

It is used in the existing code to test if we can do SSL.

That test seems entirely bogus given the new dispensation that we are
not going to error out on bad entries in listen_addresses. I'd counsel
just getting rid of it.

This seems a bit nonrobust since it will fail if any whitespace is
added. I think it'd be better to test whether any individual hostname
extracted from the string is '*'.

Agree with the first point, disagree with the second. What does it mean
to specify "12.34.56.78 *"? I think "*" should be allowed only if it
is the only entry in the list.

What does it mean to specify "12.34.56.78 12.34.56.78 12.34.56.78"?
If we want to prevent people from writing redundant listen_addresses
lists, we'll have to work a lot harder than this. But I don't see the
point of trying.

regards, tom lane

Tom Lane wrote:

Please revise, and update the docs too.

Second attempt attached. The fatal message now reads "no configured
listening socket available", but I am not wedded to the wording. I also
was not sure how to mark up * in the docs.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

Second attempt attached. The fatal message now reads "no configured
listening socket available", but I am not wedded to the wording. I also
was not sure how to mark up * in the docs.

Reviewed and committed.

I found a bunch more references in the docs to the postmaster -i and -h
switches, and I'm far from sure we've hit them all yet; anyone want to
make another pass to check?

Code-wise it looked great, except that you need to remember to cast the
argument of isspace() to unsigned char; on most machines with signed
chars, failing to do this causes big trouble for 8th-bit-set characters.

I went with "no socket configured to listen on" for the failure message,
but am not wedded to that either.

regards, tom lane

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Second attempt attached. The fatal message now reads "no configured
listening socket available", but I am not wedded to the wording. I also
was not sure how to mark up * in the docs.

Reviewed and committed.

I found a bunch more references in the docs to the postmaster -i and -h
switches, and I'm far from sure we've hit them all yet; anyone want to
make another pass to check?

Code-wise it looked great, except that you need to remember to cast the
argument of isspace() to unsigned char; on most machines with signed
chars, failing to do this causes big trouble for 8th-bit-set characters.

I went with "no socket configured to listen on" for the failure message,
but am not wedded to that either.

I updated the error text to:

(errmsg("no socket configured for listening")));

Hope you like it.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian said:

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Second attempt attached. The fatal message now reads "no configured
listening socket available", but I am not wedded to the wording. I
also was not sure how to mark up * in the docs.

I went with "no socket configured to listen on" for the failure
message, but am not wedded to that either.

I updated the error text to:

(errmsg("no socket configured for listening")));

Hope you like it.

Both of these are not always correct - they might well have configured a
listening address but we were unable to bind to it, although in that case
a warning would have already been issued. To be strictly correct either we
need to use a flag to distinguish the these cases, or we need an error
message that contemplates both cases.

cheers

andrew

"Andrew Dunstan" <andrew@dunslane.net> writes:

Both of these are not always correct - they might well have configured a
listening address but we were unable to bind to it, although in that case
a warning would have already been issued. To be strictly correct either we
need to use a flag to distinguish the these cases, or we need an error
message that contemplates both cases.

How about "no socket created for listening" or some such?

regards, tom lane

Tom Lane wrote:

"Andrew Dunstan" <andrew@dunslane.net> writes:

Both of these are not always correct - they might well have configured a
listening address but we were unable to bind to it, although in that case
a warning would have already been issued. To be strictly correct either we
need to use a flag to distinguish the these cases, or we need an error
message that contemplates both cases.

How about "no socket created for listening" or some such?

That works.

cheers

andrew

Tom Lane wrote:

"Andrew Dunstan" <andrew@dunslane.net> writes:

Both of these are not always correct - they might well have configured a
listening address but we were unable to bind to it, although in that case
a warning would have already been issued. To be strictly correct either we
need to use a flag to distinguish the these cases, or we need an error
message that contemplates both cases.

How about "no socket created for listening" or some such?

Or "no socket available for listening"?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

How about "no socket created for listening" or some such?

Or "no socket available for listening"?

"Created" seems better since it's a verb, and focuses attention on the
probability that we tried and failed to make a socket. "Available" is
too passive; it seems to suggest that the problem is a kernel limitation
or some other outside force, when of course the problem is bogus
configuration parameters given us by the DBA.

But we've already spent far more time on this one message than is
justified ;-)

regards, tom lane