Re: [HACKERS] Fix handling of invalid sockets returned by PQsocket()

[Alvaro Herrera]

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 Herrerahttp://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

Re: [HACKERS] Fix handling of invalid sockets returned by PQsocket()

[Michael Paquier]

On Sun, Mar 6, 2016 at 12:52 PM, Alvaro Herrera
 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
>> >  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
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index f670957..4ee4d71 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -331,7 +331,7 @@ libpq_select(int timeout_ms)
 	if (PQsocket(streamConn) < 0)
 		ereport(ERROR,
 (errcode_for_socket_access(),
- errmsg("socket not open")));
+ errmsg("invalid socket: %s", PQerrorMessage(streamConn;
 
 	/* We use poll(2) if available, otherwise select(2) */
 	{
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 832f9f9..6d12705 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -360,6 +360,14 @@ StreamLogicalLog(void)
 			struct timeval timeout;
 			struct timeval *timeoutptr = NULL;
 
+			if (PQsocket(conn) < 0)
+			{
+fprintf(stderr,
+		_("%s: invalid socket: %s"),
+		progname, PQerrorMessage(conn));
+goto error;
+			}
+
 			FD_ZERO(_mask);
 			FD_SET(PQsocket(conn), _mask);
 
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 6d7e635..01c42fc 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -956,7 +956,8 @@ CopyStreamPoll(PGconn *conn, long timeout_ms)
 
 	if (PQsocket(conn) < 0)
 	{
-		fprintf(stderr, _("%s: socket not open"), progname);
+		fprintf(stderr, _("%s: invalid socket: %s"), progname,
+PQerrorMessage(conn));
 		return -1;
 	}
 
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 8b0b17a..92df750 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3797,7 +3797,7 @@ threadRun(void *arg)
 			sock = PQsocket(st->con);
 			if (sock < 0)
 			{
-fprintf(stderr, "bad socket: %s", PQerrorMessage(st->con));
+fprintf(stderr, "invalid socket: %s", PQerrorMessage(st->con));
 goto done;
 			}
 
@@ -3867,7 +3867,8 @@ threadRun(void *arg)
 
 if (sock < 0)
 {
-	fprintf(stderr, "bad socket: %s", PQerrorMessage(st->con));
+	fprintf(stderr, "invalid socket: %s",
+			PQerrorMessage(st->con));
 	goto done;
 }
 if (FD_ISSET(sock, _mask) ||
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index c6afcd5..b673be8 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -70,7 +70,7 @@ static void DisconnectDatabase(ParallelSlot *slot);
 
 static int	select_loop(int maxFd, fd_set *workerset, bool *aborting);
 
-static void init_slot(ParallelSlot *slot, PGconn *conn);
+static void init_slot(ParallelSlot *slot, PGconn *conn, const char *progname);
 
 static void help(const char *progname);
 
@@ -421,14 +421,14 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
 	 * array contains the connection.
 	 */
 	slots = (ParallelSlot *) pg_malloc(sizeof(ParallelSlot) * concurrentCons);
-	init_slot(slots, conn);
+	init_slot(slots, conn, progname);
 	if (parallel)
 	{
 		for (i = 1; i < concurrentCons; i++)
 		{
 			conn = connectDatabase(dbname, host, port, username, prompt_password,
    progname, false, true);
-			init_slot(slots + i, conn);
+			init_slot(slots + i, conn, progname);
 		}
 	}
 
@@ -917,11 +917,18 @@ select_loop(int maxFd, fd_set *workerset, bool *aborting)
 }
 
 static void
-init_slot(ParallelSlot *slot, PGconn *conn)

Re: [HACKERS] Fix handling of invalid sockets returned by PQsocket()

[Alvaro Herrera]

Peter Eisentraut wrote:
> On 2/17/16 10:52 PM, Michael Paquier wrote:
> > On Thu, Feb 18, 2016 at 1:58 AM, Alvaro Herrera
> >  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 Herrerahttp://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

Re: [HACKERS] Fix handling of invalid sockets returned by PQsocket()

[Peter Eisentraut]

On 2/17/16 10:52 PM, Michael Paquier wrote:
> On Thu, Feb 18, 2016 at 1:58 AM, Alvaro Herrera
>  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

Re: [HACKERS] Fix handling of invalid sockets returned by PQsocket()

[Michael Paquier]

On Thu, Feb 18, 2016 at 1:58 AM, Alvaro Herrera
 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


pqsocket-error-fix-v2.patch
Description: binary/octet-stream

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

Re: [HACKERS] Fix handling of invalid sockets returned by PQsocket()

[Alvaro Herrera]

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 Herrerahttp://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

[HACKERS] Fix handling of invalid sockets returned by PQsocket()

[Michael Paquier]

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/cab7npqttzoiuvygnonlvnzysstusofhkeo9ftrqbkwj36uc...@mail.gmail.com
Regards,
-- 
Michael
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 832f9f9..b4325b0 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -360,6 +360,14 @@ StreamLogicalLog(void)
 			struct timeval timeout;
 			struct timeval *timeoutptr = NULL;
 
+			if (PQsocket(conn) < 0)
+			{
+fprintf(stderr,
+		_("%s: bad socket: \"%s\"\n"),
+		progname, strerror(errno));
+goto error;
+			}
+
 			FD_ZERO(_mask);
 			FD_SET(PQsocket(conn), _mask);
 
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index c6afcd5..e81123f 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -70,7 +70,7 @@ static void DisconnectDatabase(ParallelSlot *slot);
 
 static int	select_loop(int maxFd, fd_set *workerset, bool *aborting);
 
-static void init_slot(ParallelSlot *slot, PGconn *conn);
+static void init_slot(ParallelSlot *slot, PGconn *conn, const char *progname);
 
 static void help(const char *progname);
 
@@ -421,14 +421,14 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
 	 * array contains the connection.
 	 */
 	slots = (ParallelSlot *) pg_malloc(sizeof(ParallelSlot) * concurrentCons);
-	init_slot(slots, conn);
+	init_slot(slots, conn, progname);
 	if (parallel)
 	{
 		for (i = 1; i < concurrentCons; i++)
 		{
 			conn = connectDatabase(dbname, host, port, username, prompt_password,
    progname, false, true);
-			init_slot(slots + i, conn);
+			init_slot(slots + i, conn, progname);
 		}
 	}
 
@@ -917,11 +917,18 @@ select_loop(int maxFd, fd_set *workerset, bool *aborting)
 }
 
 static void
-init_slot(ParallelSlot *slot, PGconn *conn)
+init_slot(ParallelSlot *slot, PGconn *conn, const char *progname)
 {
 	slot->connection = conn;
 	slot->isFree = true;
 	slot->sock = PQsocket(conn);
+
+	if (slot->sock < 0)
+	{
+		fprintf(stderr, _("%s: bad socket: %s\n"), progname,
+strerror(errno));
+		exit(1);
+	}
 }
 
 static void
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 0a9d25c..3e13a39 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -720,6 +720,12 @@ try_complete_step(Step *step, int flags)
 	PGresult   *res;
 	bool		canceled = false;
 
+	if (sock < 0)
+	{
+		fprintf(stderr, "bad socket: %s\n", strerror(errno));
+		exit_nicely();
+	}
+
 	gettimeofday(_time, NULL);
 	FD_ZERO(_set);
 

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