Fix handling of invalid sockets returned by PQsocket()

Michael Paquier wrote:

Hi all,

After looking at Alvaro's message mentioning the handling of
PQsocket() for invalid sockets, I just had a look by curiosity at
other calls of this routine, and found a couple of issues:
1) In vacuumdb.c, init_slot() does not check for the return value of PQsocket():
slot->sock = PQsocket(conn);
2) In isolationtester.c, try_complete_step() should do the same.
3) In pg_recvlogical.c for StreamLogicalLog() I am spotting the same problem.
I guess those ones should be fixed as well, no?

I patched pgbench to use PQerrorMessage rather than strerror(errno). I
think your patch should do the same.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Feb 18, 2016 at 1:58 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Michael Paquier wrote:

Hi all,

After looking at Alvaro's message mentioning the handling of
PQsocket() for invalid sockets, I just had a look by curiosity at
other calls of this routine, and found a couple of issues:
1) In vacuumdb.c, init_slot() does not check for the return value of PQsocket():
slot->sock = PQsocket(conn);
2) In isolationtester.c, try_complete_step() should do the same.
3) In pg_recvlogical.c for StreamLogicalLog() I am spotting the same problem.
I guess those ones should be fixed as well, no?

I patched pgbench to use PQerrorMessage rather than strerror(errno). I
think your patch should do the same.

OK, this looks like a good idea. I would suggest doing the same in
receivelog.c then.
--
Michael

On 2/17/16 10:52 PM, Michael Paquier wrote:

On Thu, Feb 18, 2016 at 1:58 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Michael Paquier wrote:

Hi all,

After looking at Alvaro's message mentioning the handling of
PQsocket() for invalid sockets, I just had a look by curiosity at
other calls of this routine, and found a couple of issues:
1) In vacuumdb.c, init_slot() does not check for the return value of PQsocket():
slot->sock = PQsocket(conn);
2) In isolationtester.c, try_complete_step() should do the same.
3) In pg_recvlogical.c for StreamLogicalLog() I am spotting the same problem.
I guess those ones should be fixed as well, no?

I patched pgbench to use PQerrorMessage rather than strerror(errno). I
think your patch should do the same.

OK, this looks like a good idea. I would suggest doing the same in
receivelog.c then.

Let's make the error messages consistent as "invalid socket". "bad
socket" isn't really our style, and pg_basebackup saying "socket not
open" is just plain incorrect.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut wrote:

On 2/17/16 10:52 PM, Michael Paquier wrote:

On Thu, Feb 18, 2016 at 1:58 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Michael Paquier wrote:

Hi all,

After looking at Alvaro's message mentioning the handling of
PQsocket() for invalid sockets, I just had a look by curiosity at
other calls of this routine, and found a couple of issues:
1) In vacuumdb.c, init_slot() does not check for the return value of PQsocket():
slot->sock = PQsocket(conn);
2) In isolationtester.c, try_complete_step() should do the same.
3) In pg_recvlogical.c for StreamLogicalLog() I am spotting the same problem.
I guess those ones should be fixed as well, no?

I patched pgbench to use PQerrorMessage rather than strerror(errno). I
think your patch should do the same.

OK, this looks like a good idea. I would suggest doing the same in
receivelog.c then.

Let's make the error messages consistent as "invalid socket". "bad
socket" isn't really our style, and pg_basebackup saying "socket not
open" is just plain incorrect.

You're completely right. Let's patch pgbench that way too.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Mar 6, 2016 at 12:52 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Peter Eisentraut wrote:

On 2/17/16 10:52 PM, Michael Paquier wrote:

On Thu, Feb 18, 2016 at 1:58 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Michael Paquier wrote:

Hi all,

After looking at Alvaro's message mentioning the handling of
PQsocket() for invalid sockets, I just had a look by curiosity at
other calls of this routine, and found a couple of issues:
1) In vacuumdb.c, init_slot() does not check for the return value of PQsocket():
slot->sock = PQsocket(conn);
2) In isolationtester.c, try_complete_step() should do the same.
3) In pg_recvlogical.c for StreamLogicalLog() I am spotting the same problem.
I guess those ones should be fixed as well, no?

I patched pgbench to use PQerrorMessage rather than strerror(errno). I
think your patch should do the same.

OK, this looks like a good idea. I would suggest doing the same in
receivelog.c then.

Let's make the error messages consistent as "invalid socket". "bad
socket" isn't really our style, and pg_basebackup saying "socket not
open" is just plain incorrect.

You're completely right. Let's patch pgbench that way too.

Here is v3 then, switching to "invalid socket" for those error
messages. There are two extra messages in fe-misc.c and
libpqwalreceiver.c that need a rewording that I have detected as well.
--
Michael

Michael Paquier wrote:

Here is v3 then, switching to "invalid socket" for those error
messages. There are two extra messages in fe-misc.c and
libpqwalreceiver.c that need a rewording that I have detected as well.

Peter Eisentraut pushed this as a40814d7a.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers