Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix assorted issues in parallel vacuumdb.

  1. [HACKERS][PATCH] adding simple sock check for windows

    DaeMyung Kang <charsyam@gmail.com> — 2018-03-30T14:40:36Z

    Hi, I found some missing check for windows int init_slot function in vacuumdb.c

in windows
SOCKET is unsigned type. so

slot->sock < 0 never can be under 0.

so this patch just check using slot->sock == PGINVALID_SOCKET

Thanks.

  2. Re: [HACKERS][PATCH] adding simple sock check for windows

    Amit Kapila <amit.kapila16@gmail.com> — 2018-03-31T05:38:37Z

    On Fri, Mar 30, 2018 at 8:10 PM, CharSyam <charsyam@gmail.com> wrote:
> Hi, I found some missing check for windows int init_slot function in vacuumdb.c
>
> in windows
> SOCKET is unsigned type. so
>
> slot->sock < 0 never can be under 0.
>
> so this patch just check using slot->sock == PGINVALID_SOCKET
>

- if (slot->sock < 0)
+ if (slot->sock == PGINVALID_SOCKET || slot->sock < 0)

If you are checking for PGINVALID_SOCKET, why do you need the second
part of check (slot->sock < 0)?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  3. Re: [HACKERS][PATCH] adding simple sock check for windows

    DaeMyung Kang <charsyam@gmail.com> — 2018-03-31T06:12:46Z

    Hi, Amit, It's good question. I also thought about it.

But, I want to leave original code intention.

Actually I think there is no case ( slot->sock is not PGINVALID_SOCKET
and slot->sock < 0)

but if original code want to check (sock < -1)

I think it is better to leave that condition.

but I think slot->sock == PGINVALID_SOCKET is enough

2018-03-31 14:38 GMT+09:00 Amit Kapila <amit.kapila16@gmail.com>:
> On Fri, Mar 30, 2018 at 8:10 PM, CharSyam <charsyam@gmail.com> wrote:
>> Hi, I found some missing check for windows int init_slot function in vacuumdb.c
>>
>> in windows
>> SOCKET is unsigned type. so
>>
>> slot->sock < 0 never can be under 0.
>>
>> so this patch just check using slot->sock == PGINVALID_SOCKET
>>
>
> - if (slot->sock < 0)
> + if (slot->sock == PGINVALID_SOCKET || slot->sock < 0)
>
> If you are checking for PGINVALID_SOCKET, why do you need the second
> part of check (slot->sock < 0)?
>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com

  4. Re: [HACKERS][PATCH] adding simple sock check for windows

    Amit Kapila <amit.kapila16@gmail.com> — 2018-03-31T06:17:47Z

    On Sat, Mar 31, 2018 at 11:42 AM, CharSyam <charsyam@gmail.com> wrote:
> Hi, Amit, It's good question. I also thought about it.
>
> But, I want to leave original code intention.
>
> Actually I think there is no case ( slot->sock is not PGINVALID_SOCKET
> and slot->sock < 0)
>
> but if original code want to check (sock < -1)
>

If you see the code of PQsocket, then that won't be possible.


> I think it is better to leave that condition.
>
> but I think slot->sock == PGINVALID_SOCKET is enough
>

It is up to you, but I don't see any reason to retain the old check.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  5. Re: [HACKERS][PATCH] adding simple sock check for windows

    DaeMyung Kang <charsyam@gmail.com> — 2018-03-31T06:35:04Z

    Amit, I agree with you.

I changed my patch :) to remove old check.

2018-03-31 15:17 GMT+09:00 Amit Kapila <amit.kapila16@gmail.com>:
> On Sat, Mar 31, 2018 at 11:42 AM, CharSyam <charsyam@gmail.com> wrote:
>> Hi, Amit, It's good question. I also thought about it.
>>
>> But, I want to leave original code intention.
>>
>> Actually I think there is no case ( slot->sock is not PGINVALID_SOCKET
>> and slot->sock < 0)
>>
>> but if original code want to check (sock < -1)
>>
>
> If you see the code of PQsocket, then that won't be possible.
>
>
>> I think it is better to leave that condition.
>>
>> but I think slot->sock == PGINVALID_SOCKET is enough
>>
>
> It is up to you, but I don't see any reason to retain the old check.
>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com

  6. Re: [HACKERS][PATCH] adding simple sock check for windows

    Amit Kapila <amit.kapila16@gmail.com> — 2018-03-31T10:33:07Z

    On Sat, Mar 31, 2018 at 12:05 PM, CharSyam <charsyam@gmail.com> wrote:
> Amit, I agree with you.
>
> I changed my patch :) to remove old check.
>

- if (slot->sock < 0)
+ if (slot->sock == PGINVALID_SOCKET || slot->sock < 0)

I still see the same check. I think by mistake you have attached old patch.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  7. Re: [HACKERS][PATCH] adding simple sock check for windows

    DaeMyung Kang <charsyam@gmail.com> — 2018-03-31T12:38:55Z

    Thanks Amit.
I had a mistake. Thank you again to point it out :)


2018-03-31 19:33 GMT+09:00 Amit Kapila <amit.kapila16@gmail.com>:
> On Sat, Mar 31, 2018 at 12:05 PM, CharSyam <charsyam@gmail.com> wrote:
>> Amit, I agree with you.
>>
>> I changed my patch :) to remove old check.
>>
>
> - if (slot->sock < 0)
> + if (slot->sock == PGINVALID_SOCKET || slot->sock < 0)
>
> I still see the same check. I think by mistake you have attached old patch.
>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com

  8. Re: [HACKERS][PATCH] adding simple sock check for windows

    Amit Kapila <amit.kapila16@gmail.com> — 2018-03-31T13:04:15Z

    On Sat, Mar 31, 2018 at 6:08 PM, CharSyam <charsyam@gmail.com> wrote:
> Thanks Amit.
> I had a mistake. Thank you again to point it out :)
>

Thanks, your new patch looks good.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  9. Re: [HACKERS][PATCH] adding simple sock check for windows

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-31T17:16:07Z

    CharSyam <charsyam@gmail.com> writes:
> [ simple_check.patch ]

This is a good catch.  However, it looks to me like the reason nobody
has noticed a problem here is that actually, this error test is useless;
we can never get here with a bad connection, because connectDatabase
would have failed.  I'm inclined to think we should just delete the whole
if-statement, instead.

BTW, I tried cross-checking for other errors of this ilk by doing

diff --git a/src/include/port.h b/src/include/port.h
index a514ab758b..9ba6a0df79 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -28,9 +28,9 @@
 
 /* socket has a different definition on WIN32 */
 #ifndef WIN32
-typedef int pgsocket;
+typedef unsigned int pgsocket;
 
-#define PGINVALID_SOCKET (-1)
+#define PGINVALID_SOCKET ((pgsocket) -1)
 #else
 typedef SOCKET pgsocket;
 
and then compiling with a compiler that would warn about "unsigned int < 0"
tests (I used Apple's current compiler, but probably any recent clang
would do as well).  It found this case and no others, which is good.
This is not a 100% cross-check, because I don't think it would notice
"unsigned int == -1" which is another way somebody might misspell the
test, and it definitely would not catch cases where somebody stored a
socket identifier in plain int instead of pgsocket.  But it's better
than nothing...

			regards, tom lane

  10. Re: [HACKERS][PATCH] adding simple sock check for windows

    DaeMyung Kang <charsyam@gmail.com> — 2018-03-31T17:37:28Z

    Hi, Tom, Thank you for your review.
so Do you think it is better to remove if statement like below

diff --git src/bin/scripts/vacuumdb.c src/bin/scripts/vacuumdb.c
index 887fa48fbd..243d842d06 100644
--- src/bin/scripts/vacuumdb.c
+++ src/bin/scripts/vacuumdb.c
@@ -947,13 +947,6 @@ 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: invalid socket: %s"), progname,
-                PQerrorMessage(conn));
-        exit(1);
-    }
 }

 static void

2018-04-01 2:16 GMT+09:00 Tom Lane <tgl@sss.pgh.pa.us>:
> CharSyam <charsyam@gmail.com> writes:
>> [ simple_check.patch ]
>
> This is a good catch.  However, it looks to me like the reason nobody
> has noticed a problem here is that actually, this error test is useless;
> we can never get here with a bad connection, because connectDatabase
> would have failed.  I'm inclined to think we should just delete the whole
> if-statement, instead.
>
> BTW, I tried cross-checking for other errors of this ilk by doing
>
> diff --git a/src/include/port.h b/src/include/port.h
> index a514ab758b..9ba6a0df79 100644
> --- a/src/include/port.h
> +++ b/src/include/port.h
> @@ -28,9 +28,9 @@
>
>  /* socket has a different definition on WIN32 */
>  #ifndef WIN32
> -typedef int pgsocket;
> +typedef unsigned int pgsocket;
>
> -#define PGINVALID_SOCKET (-1)
> +#define PGINVALID_SOCKET ((pgsocket) -1)
>  #else
>  typedef SOCKET pgsocket;
>
> and then compiling with a compiler that would warn about "unsigned int < 0"
> tests (I used Apple's current compiler, but probably any recent clang
> would do as well).  It found this case and no others, which is good.
> This is not a 100% cross-check, because I don't think it would notice
> "unsigned int == -1" which is another way somebody might misspell the
> test, and it definitely would not catch cases where somebody stored a
> socket identifier in plain int instead of pgsocket.  But it's better
> than nothing...
>
>                         regards, tom lane

  11. Re: [HACKERS][PATCH] adding simple sock check for windows

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-31T18:03:38Z

    ... Oh, just to make things even more fun, PQsocket() returns int, not
pgsocket; see its header comment.  Therefore, that test is correctly
coded as-is (though it's still useless), and the real problem is that
ParallelSlot.sock ought to be declared int not pgsocket.  If you look
around at our other places that read PQsocket() and use its result in
select() masks, they uniformly use int variables, and I think they're
right.

Actually, the more I look at this code, the more problems I'm finding.
The wait-for-a-free-connection code is just broken on its face, because
it supposes it can retry select() waits without reinitializing the
FD_SET.  The loop in vacuum_one_database() uses "conn" to call
prepare_vacuum_command() without any thought as to whether that is a
free connection --- which, typically, it isn't.  It looks to me like
libpq silently copes with this silliness, just waiting for the active
command to finish and then doing what it's told, but the net result
is that supposedly-concurrent vacuums get serialized.

			regards, tom lane

  12. Re: [HACKERS][PATCH] adding simple sock check for windows

    Amit Kapila <amit.kapila16@gmail.com> — 2018-04-01T03:29:30Z

    On Sat, Mar 31, 2018 at 11:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> ... Oh, just to make things even more fun, PQsocket() returns int, not
> pgsocket; see its header comment.  Therefore, that test is correctly
> coded as-is (though it's still useless), and the real problem is that
> ParallelSlot.sock ought to be declared int not pgsocket.  If you look
> around at our other places that read PQsocket() and use its result in
> select() masks, they uniformly use int variables, and I think they're
> right.
>

There is one other place where we are using pgsocket, see
walrcv_receive, but we are using Assert in that place.  I think it
would be better if the output of PQsocket() can be consistently used
everywhere.  If there is no particular restriction, then it will be
good to make it as 'int' everywhere.


> Actually, the more I look at this code, the more problems I'm finding.
> The wait-for-a-free-connection code is just broken on its face, because
> it supposes it can retry select() waits without reinitializing the
> FD_SET.  The loop in vacuum_one_database() uses "conn" to call
> prepare_vacuum_command() without any thought as to whether that is a
> free connection --- which, typically, it isn't.  It looks to me like
> libpq silently copes with this silliness, just waiting for the active
> command to finish and then doing what it's told, but the net result
> is that supposedly-concurrent vacuums get serialized.
>

I think it would have been better if this code would have found the
free_slot before preparing the command and then used the connection
from free slot.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  13. Re: [HACKERS][PATCH] adding simple sock check for windows

    Amit Kapila <amit.kapila16@gmail.com> — 2018-04-01T03:39:20Z

    On Sun, Apr 1, 2018 at 8:59 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sat, Mar 31, 2018 at 11:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... Oh, just to make things even more fun, PQsocket() returns int, not
>> pgsocket; see its header comment.  Therefore, that test is correctly
>> coded as-is (though it's still useless), and the real problem is that
>> ParallelSlot.sock ought to be declared int not pgsocket.  If you look
>> around at our other places that read PQsocket() and use its result in
>> select() masks, they uniformly use int variables, and I think they're
>> right.
>>
>
> There is one other place where we are using pgsocket, see
> walrcv_receive, but we are using Assert in that place.  I think it
> would be better if the output of PQsocket() can be consistently used
> everywhere.  If there is no particular restriction, then it will be
> good to make it as 'int' everywhere.
>
>
>> Actually, the more I look at this code, the more problems I'm finding.
>> The wait-for-a-free-connection code is just broken on its face, because
>> it supposes it can retry select() waits without reinitializing the
>> FD_SET.  The loop in vacuum_one_database() uses "conn" to call
>> prepare_vacuum_command() without any thought as to whether that is a
>> free connection --- which, typically, it isn't.  It looks to me like
>> libpq silently copes with this silliness, just waiting for the active
>> command to finish and then doing what it's told, but the net result
>> is that supposedly-concurrent vacuums get serialized.
>>
>
> I think it would have been better if this code would have found the
> free_slot before preparing the command and then used the connection
> from free slot.
>

oops, I just saw that you have already pushed a fix for it.  I am not
sure if we should try to do anything about walrcv_receive's output
still uses pgsocket instead of int as the usage in itself doesn't have
any problem.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  14. Re: [HACKERS][PATCH] adding simple sock check for windows

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-01T05:30:07Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> oops, I just saw that you have already pushed a fix for it.  I am not
> sure if we should try to do anything about walrcv_receive's output
> still uses pgsocket instead of int as the usage in itself doesn't have
> any problem.

I see a few places where we're still assigning PQsocket's result to a
pgsocket variable, but none of them are worrying about the possibility
that it's not a valid socket, and I'm not sure they need to.

			regards, tom lane