Обсуждение: Fix handling of invalid sockets returned by PQsocket()

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?

The patch attached addresses those issues.

This has been raised in this message, but beginning a new thread makes
more sense:
http://www.postgresql.org/message-id/CAB7nPqTTZoiuVYGNonLVnZysStUSOfhKeO9FTrQbKWJ36UCdOA@mail.gmail.com
Regards,
--
Michael

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

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.

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

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