walreceiver is uninterruptible on win32

On Wed, Mar 10, 2010 at 10:09, Fujii Masao <masao.fujii@gmail.com> wrote:

Hi,

http://archives.postgresql.org/pgsql-hackers/2010-01/msg01672.php
On win32, the blocking libpq functions like PQconnectdb() and
PQexec() are uninterruptible since they use the vanilla select()
instead of our signal emulation layer compatible select().
Nevertheless, currently walreceiver uses them to establish a
connection, send a handshake message and wait for the reply.
So walreceiver also becomes uninterruptible for a while. This
is the must-fix problem for 9.0.

I replaced the blocking libpq functions currently used with
asynchronous ones, and used the emulated version of select()
to wait, to make walreceiver interruptible. Here is the patch.

These are issues that affect other things running libpq in the backend
as well, right? Such as dblink? Perhaps we can factor out most of this
into functions in backend/port/win32 so that we can re-use it fro
there?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Fri, Mar 12, 2010 at 8:13 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Wed, Mar 10, 2010 at 10:09, Fujii Masao <masao.fujii@gmail.com> wrote:

Hi,

http://archives.postgresql.org/pgsql-hackers/2010-01/msg01672.php
On win32, the blocking libpq functions like PQconnectdb() and
PQexec() are uninterruptible since they use the vanilla select()
instead of our signal emulation layer compatible select().
Nevertheless, currently walreceiver uses them to establish a
connection, send a handshake message and wait for the reply.
So walreceiver also becomes uninterruptible for a while. This
is the must-fix problem for 9.0.

I replaced the blocking libpq functions currently used with
asynchronous ones, and used the emulated version of select()
to wait, to make walreceiver interruptible. Here is the patch.

These are issues that affect other things running libpq in the backend
as well, right? Such as dblink?

Yes. So Heikki wrote the patch for dblink.
http://archives.postgresql.org/pgsql-hackers/2010-01/msg02072.php

Perhaps we can factor out most of this
into functions in backend/port/win32 so that we can re-use it fro
there?

Sorry. I couldn't get your point. Could you explain it in detail?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Mon, Mar 15, 2010 at 10:14, Fujii Masao <masao.fujii@gmail.com> wrote:

On Fri, Mar 12, 2010 at 8:13 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Wed, Mar 10, 2010 at 10:09, Fujii Masao <masao.fujii@gmail.com> wrote:

Hi,

http://archives.postgresql.org/pgsql-hackers/2010-01/msg01672.php
On win32, the blocking libpq functions like PQconnectdb() and
PQexec() are uninterruptible since they use the vanilla select()
instead of our signal emulation layer compatible select().
Nevertheless, currently walreceiver uses them to establish a
connection, send a handshake message and wait for the reply.
So walreceiver also becomes uninterruptible for a while. This
is the must-fix problem for 9.0.

I replaced the blocking libpq functions currently used with
asynchronous ones, and used the emulated version of select()
to wait, to make walreceiver interruptible. Here is the patch.

These are issues that affect other things running libpq in the backend
as well, right? Such as dblink?

Yes. So Heikki wrote the patch for dblink.
http://archives.postgresql.org/pgsql-hackers/2010-01/msg02072.php

IIRC that was never applied.

Perhaps we can factor out most of this
into functions in backend/port/win32 so that we can re-use it fro
there?

Sorry. I couldn't get your point. Could you explain it in detail?

What I'm referring to is the part that Heikki writes as "The
implementation should be shared between the two, but I'm not sure
how". I think we should try to factor out things that can be shared
into separate functions and stick those in port/win32 (assuming
they're win32-specific, otherwise, in another suitable location), and
then call them from both. There seems to be a lot of things that
should be doable that way.

I notice for example that the dblink patch doesn't have the code for
timeout handling - shouldn't it?

I think we need to look at this as a single problem needing to be
solved, and then have the same solution applied to dblink and
walreceiver.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Mon, Mar 15, 2010 at 6:42 PM, Magnus Hagander <magnus@hagander.net> wrote:

Perhaps we can factor out most of this
into functions in backend/port/win32 so that we can re-use it fro
there?

Sorry. I couldn't get your point. Could you explain it in detail?

What I'm referring to is the part that Heikki writes as "The
implementation should be shared between the two, but I'm not sure
how". I think we should try to factor out things that can be shared
into separate functions and stick those in port/win32 (assuming
they're win32-specific, otherwise, in another suitable location), and
then call them from both. There seems to be a lot of things that
should be doable that way.

I notice for example that the dblink patch doesn't have the code for
timeout handling - shouldn't it?

I think we need to look at this as a single problem needing to be
solved, and then have the same solution applied to dblink and
walreceiver.

Thanks for the explanation. I agree that the code should be shared,
but am not sure how, too.

Something like libpq_select() which waits for the socket to become
ready would be required for walreceiver and dblink. But it's necessary
for walreceiver on not only win32 but also the other, so some functions
might need to be placed in the location other than port/win32.

I'll think of this issue for a while.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Fujii Masao wrote:

On Mon, Mar 15, 2010 at 6:42 PM, Magnus Hagander <magnus@hagander.net> wrote:

I think we need to look at this as a single problem needing to be
solved, and then have the same solution applied to dblink and
walreceiver.

Agreed.

Something like libpq_select() which waits for the socket to become
ready would be required for walreceiver and dblink. But it's necessary
for walreceiver on not only win32 but also the other, ...

Really, why? I thought this is a purely Windows specific problem.

Just replacing PQexec() with PQsendQuery() is pretty straightforward, we
could put that replacement in a file in port/win32. Replacing
PQconnectdb() is more complicated because you need to handle connection
timeout. I suggest that we only add the replacement for PQexec(), and
live with the situation for PQconnectdb(), that covers 99% of the
scenarios anyway.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On 03/15/2010 02:42 AM, Magnus Hagander wrote:

I think we need to look at this as a single problem needing to be
solved, and then have the same solution applied to dblink and
walreceiver.

+1

Joe

On Tue, Mar 16, 2010 at 12:32 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Something like libpq_select() which waits for the socket to become
ready would be required for walreceiver and dblink. But it's necessary
for walreceiver on not only win32 but also the other, ...

Really, why? I thought this is a purely Windows specific problem.

Because, on all the platforms, libpq_receive() needs to call libpq_select().
Or you mean that we should leave the existing libpq_select() as it is, and
create new win32 specific function which waits for the socket to become ready,
for this issue?

Just replacing PQexec() with PQsendQuery() is pretty straightforward, we
could put that replacement in a file in port/win32. Replacing
PQconnectdb() is more complicated because you need to handle connection
timeout. I suggest that we only add the replacement for PQexec(), and
live with the situation for PQconnectdb(), that covers 99% of the
scenarios anyway.

I'll try to replace PQexec() first, and PQconnectdb() second if I have
enough time.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Tue, Mar 16, 2010 at 10:35 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

Just replacing PQexec() with PQsendQuery() is pretty straightforward, we
could put that replacement in a file in port/win32. Replacing
PQconnectdb() is more complicated because you need to handle connection
timeout. I suggest that we only add the replacement for PQexec(), and
live with the situation for PQconnectdb(), that covers 99% of the
scenarios anyway.

I'll try to replace PQexec() first, and PQconnectdb() second if I have
enough time.

Sorry for the delay. The attached patch replaces PQexec() used by dblink
and libpqwalreceiver with pgwin32_PQexec() which is the win32 version of
PQexec().

pgwin32_PQexec() is provided as the library 'libpqbe.dll', which is created
only on win32. dblink.dll and libpqwalreceiver.dll refer to libpqbe.dll.
Also libpqbe.dll refers to libpq.dll.

I'm not sure if my patch is in the right way. If you notice anything,
please feel free to comment!

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Thu, Mar 25, 2010 at 15:33, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Mar 16, 2010 at 10:35 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

Just replacing PQexec() with PQsendQuery() is pretty straightforward, we
could put that replacement in a file in port/win32. Replacing
PQconnectdb() is more complicated because you need to handle connection
timeout. I suggest that we only add the replacement for PQexec(), and
live with the situation for PQconnectdb(), that covers 99% of the
scenarios anyway.

I'll try to replace PQexec() first, and PQconnectdb() second if I have
enough time.

Sorry for the delay. The attached patch replaces PQexec() used by dblink
and libpqwalreceiver with pgwin32_PQexec() which is the win32 version of
PQexec().

pgwin32_PQexec() is provided as the library 'libpqbe.dll', which is created
only on win32. dblink.dll and libpqwalreceiver.dll refer to libpqbe.dll.
Also libpqbe.dll refers to libpq.dll.

I'm not sure if my patch is in the right way. If you notice anything,
please feel free to comment!

Well, first of all, it would require the addition of the new target to
the msvc build system, but that's easy - I can do that for you.

More to the point, I'm not sure I like the creation of yet another DLL
to deal with this. The reason this isn't just exported from the main
backend is the same reason we created the libpqwalreceiver library I'm
sure - bt that means we already have one.

How about we just use this same source file, but compile and link it
directly into both dblink and libpqwalreceiver? That'd leave us with
one DLL less, making life easier.

The downside would be that other third-party modules who need to call
libpq wouldn't be able to access this without copying the function.
But is this really something that's useful for third party modules?

As for the code itself, don't we need a CHECK_FOR_INTERRUPTS in there
for it to be actually useful?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:

On Thu, Mar 25, 2010 at 15:33, Fujii Masao <masao.fujii@gmail.com> wrote:

Sorry for the delay. The attached patch replaces PQexec() used by dblink
and libpqwalreceiver with pgwin32_PQexec() which is the win32 version of
PQexec().

pgwin32_PQexec() is provided as the library 'libpqbe.dll', which is created
only on win32. dblink.dll and libpqwalreceiver.dll refer to libpqbe.dll.
Also libpqbe.dll refers to libpq.dll.

[ assorted objections ]

I disapprove of the whole approach, actually. The right way to fix this
is to not touch or replace libpq at all, but to change walreceiver to
use libpq's async-query facilities directly. Instead of PQexec, use
PQsendQuery and then a loop involving PQisBusy, PQgetResult, etc.
You've more or less done that loop, but you've put it in the wrong
place.

The larger point is that I don't believe this issue exists only on
Windows. I think we're going to want something like this on all
platforms, and that implies supporting poll() not just select() for the
waiting part.

The patch also seems confused about whether it's intending to be a
general-purpose solution or not. You can maybe take some shortcuts
if it's only going to be for walreceiver, but if you're going to put
it in dblink it is *not* acceptable to take shortcuts like not
processing errors completely.

regards, tom lane

On Fri, Apr 2, 2010 at 17:26, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

On Thu, Mar 25, 2010 at 15:33, Fujii Masao <masao.fujii@gmail.com> wrote:

Sorry for the delay. The attached patch replaces PQexec() used by dblink
and libpqwalreceiver with pgwin32_PQexec() which is the win32 version of
PQexec().

pgwin32_PQexec() is provided as the library 'libpqbe.dll', which is created
only on win32. dblink.dll and libpqwalreceiver.dll refer to libpqbe.dll.
Also libpqbe.dll refers to libpq.dll.

[ assorted objections ]

I disapprove of the whole approach, actually.  The right way to fix this
is to not touch or replace libpq at all, but to change walreceiver to
use libpq's async-query facilities directly.  Instead of PQexec, use
PQsendQuery and then a loop involving PQisBusy, PQgetResult, etc.
You've more or less done that loop, but you've put it in the wrong
place.

Any particular reason not to wrap that in a function? Not called
pgwin32_PQexec() then, but something more generic? And not doing any
#defines to change PQexec, but call that wrapper directly?

The larger point is that I don't believe this issue exists only on
Windows.  I think we're going to want something like this on all
platforms, and that implies supporting poll() not just select() for the
waiting part.

The most important part of the issue doesn't (because PQexec will be
interrupted by a signal), but there may certainly be others.

The patch also seems confused about whether it's intending to be a
general-purpose solution or not.  You can maybe take some shortcuts
if it's only going to be for walreceiver, but if you're going to put
it in dblink it is *not* acceptable to take shortcuts like not
processing errors completely.

Yeah, good point.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:

On Fri, Apr 2, 2010 at 17:26, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I disapprove of the whole approach, actually. �The right way to fix this
is to not touch or replace libpq at all, but to change walreceiver to
use libpq's async-query facilities directly. �Instead of PQexec, use
PQsendQuery and then a loop involving PQisBusy, PQgetResult, etc.
You've more or less done that loop, but you've put it in the wrong
place.

Any particular reason not to wrap that in a function? Not called
pgwin32_PQexec() then, but something more generic? And not doing any
#defines to change PQexec, but call that wrapper directly?

Yeah, that's fine. I just think it's easier to deal with this as a
local issue in walreceiver and dblink than to try to pretend we're
changing libpq's API.

The larger point is that I don't believe this issue exists only on
Windows. �I think we're going to want something like this on all
platforms, and that implies supporting poll() not just select() for the
waiting part.

The most important part of the issue doesn't (because PQexec will be
interrupted by a signal), but there may certainly be others.

Really? As you pointed out yourself, if control doesn't reach a
CHECK_FOR_INTERRUPTS then we have a problem. We also know that select
isn't interrupted by signals on all platforms.

regards, tom lane

On Fri, Apr 2, 2010 at 11:11 PM, Magnus Hagander <magnus@hagander.net> wrote:

More to the point, I'm not sure I like the creation of yet another DLL
to deal with this. The reason this isn't just exported from the main
backend is the same reason we created the libpqwalreceiver library I'm
sure - bt that means we already have one.

How about we just use this same source file, but compile and link it
directly into both dblink and libpqwalreceiver? That'd leave us with
one DLL less, making life easier.

ISTM that we cannot compile dblink using USE_PGXS=1, if that DLL doesn't
exist in the installation directory. No?

As for the code itself, don't we need a CHECK_FOR_INTERRUPTS in there
for it to be actually useful?

Yes. I'll add a CHECK_FOR_INTERRUPTS in the loop.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Sat, Apr 3, 2010 at 12:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I disapprove of the whole approach, actually.  The right way to fix this
is to not touch or replace libpq at all, but to change walreceiver to
use libpq's async-query facilities directly.  Instead of PQexec, use
PQsendQuery and then a loop involving PQisBusy, PQgetResult, etc.

Yes.

You've more or less done that loop, but you've put it in the wrong
place.

OK. I'll reconsider about how to use those asynchronous libpq functions.
But, if possible, could you point out where "the right place" is?

The larger point is that I don't believe this issue exists only on
Windows.  I think we're going to want something like this on all
platforms, and that implies supporting poll() not just select() for the
waiting part.

OK. I'll change the part so that poll() is used if HAVE_POLL is defined,
select() otherwise.

The patch also seems confused about whether it's intending to be a
general-purpose solution or not.  You can maybe take some shortcuts
if it's only going to be for walreceiver, but if you're going to put
it in dblink it is *not* acceptable to take shortcuts like not
processing errors completely.

OK. I'll address this problem. Since PGconn->errorMessage cannot be
updated from outside of libpq, I'm thinking of making the caller give
the StringInfo variable as a parameter to pgwin32_PQexec(), and
putting error messages in it. Then the caller use the StringInfo
instead of PQerrorMessage(PGconn), to get the error messages.

And ISTM that dblink needs to hold the StringInfo using hash as
do the PGconn.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Mon, Apr 5, 2010 at 3:18 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Fri, Apr 2, 2010 at 11:11 PM, Magnus Hagander <magnus@hagander.net> wrote:

More to the point, I'm not sure I like the creation of yet another DLL
to deal with this. The reason this isn't just exported from the main
backend is the same reason we created the libpqwalreceiver library I'm
sure - bt that means we already have one.

How about we just use this same source file, but compile and link it
directly into both dblink and libpqwalreceiver? That'd leave us with
one DLL less, making life easier.

ISTM that we cannot compile dblink using USE_PGXS=1, if that DLL doesn't
exist in the installation directory. No?

I might have misinterpreted your point. You mean that the same source
file defining something like pgwin32_PQexec should be placed in both
contrib/dblink and src/backend/replication/libpqwalreceiver? If so,
we can compile dblink using USE_PGXS without the DLL.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Tue, Apr 6, 2010 at 2:25 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Mon, Apr 5, 2010 at 3:18 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Fri, Apr 2, 2010 at 11:11 PM, Magnus Hagander <magnus@hagander.net> wrote:

More to the point, I'm not sure I like the creation of yet another DLL
to deal with this. The reason this isn't just exported from the main
backend is the same reason we created the libpqwalreceiver library I'm
sure - bt that means we already have one.

How about we just use this same source file, but compile and link it
directly into both dblink and libpqwalreceiver? That'd leave us with
one DLL less, making life easier.

ISTM that we cannot compile dblink using USE_PGXS=1, if that DLL doesn't
exist in the installation directory. No?

I might have misinterpreted your point. You mean that the same source
file defining something like pgwin32_PQexec should be placed in both
contrib/dblink and src/backend/replication/libpqwalreceiver? If so,
we can compile dblink using USE_PGXS without the DLL.

No, I don't mean that. I mean store it in one place, and copy/link it
into where it's used. Look at for example how crypt.c and
getaddrinfo.c are handled in libpq.

Not sure how that will play with PGXS, though, but I'm not entirely
sure we care if it can be built that way? If it does, there should be
some way to get PGXS to execute that rule as well, I'm sure.

Also note that per Tom's comments this is not a win32 only fix, so it
shouldn't be called pgwin32_*().

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Wed, Apr 7, 2010 at 1:45 AM, Magnus Hagander <magnus@hagander.net> wrote:

No, I don't mean that. I mean store it in one place, and copy/link it
into where it's used. Look at for example how crypt.c and
getaddrinfo.c are handled in libpq.

Thanks for the advice!

Not sure how that will play with PGXS, though, but I'm not entirely
sure we care if it can be built that way?

Probably Yes.

If it does, there should be
some way to get PGXS to execute that rule as well, I'm sure.

If we can copy/link the source file defining "new PQexec" when
we compile the dblink, DLL doesn't seem to be required. So I
stop creating new DLL for PGXS.

Also note that per Tom's comments this is not a win32 only fix, so it
shouldn't be called pgwin32_*().

Yep.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Thu, Apr 8, 2010 at 5:01 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

If it does, there should be
some way to get PGXS to execute that rule as well, I'm sure.

If we can copy/link the source file defining "new PQexec" when
we compile the dblink, DLL doesn't seem to be required. So I
stop creating new DLL for PGXS.

On second thought, ISTM that we cannot use any source files which exist
in places other than contrib/dblink and installation directory when we
compile dblink under USE_PGXS=1. But we can put the file implementing
new PQexec on those neither. So I'm thinking again that it should be
provided as the shared library and be linked from walreceiver and dblink.
Is this right?

If adding new shared library is too big change at this point, I think
that we should postpone the fix only for dblink to 9.1 or later. Since
no one has complained about this long-term problem of dblink, I'm not
sure it really should be fixed right now. Thought?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Fujii Masao wrote:

If adding new shared library is too big change at this point, I think
that we should postpone the fix only for dblink to 9.1 or later. Since
no one has complained about this long-term problem of dblink, I'm not
sure it really should be fixed right now. Thought?

I would agree with this. No one has ever complained that I am aware of.

Joe

On Mon, Apr 12, 2010 at 13:54, Fujii Masao <masao.fujii@gmail.com> wrote:

On Thu, Apr 8, 2010 at 5:01 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

If it does, there should be
some way to get PGXS to execute that rule as well, I'm sure.

If we can copy/link the source file defining "new PQexec" when
we compile the dblink, DLL doesn't seem to be required. So I
stop creating new DLL for PGXS.

On second thought, ISTM that we cannot use any source files which exist
in places other than contrib/dblink and installation directory when we
compile dblink under USE_PGXS=1. But we can put the file implementing
new PQexec on those neither. So I'm thinking again that it should be
provided as the shared library and be linked from walreceiver and dblink.
Is this right?

If adding new shared library is too big change at this point, I think
that we should postpone the fix only for dblink to 9.1 or later. Since
no one has complained about this long-term problem of dblink, I'm not
sure it really should be fixed right now. Thought?

+1. Let's fix walreceiver for now, and we can revisit dblink later.
Since we haven't had any complaints so far...

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Tue, Apr 13, 2010 at 1:56 AM, Magnus Hagander <magnus@hagander.net> wrote:

If adding new shared library is too big change at this point, I think
that we should postpone the fix only for dblink to 9.1 or later. Since
no one has complained about this long-term problem of dblink, I'm not
sure it really should be fixed right now. Thought?

+1. Let's fix walreceiver for now, and we can revisit dblink later.
Since we haven't had any complaints so far...

OK. I'll focus on walreceiver now.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Tue, Apr 13, 2010 at 9:21 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Apr 13, 2010 at 1:56 AM, Magnus Hagander <magnus@hagander.net> wrote:

If adding new shared library is too big change at this point, I think
that we should postpone the fix only for dblink to 9.1 or later. Since
no one has complained about this long-term problem of dblink, I'm not
sure it really should be fixed right now. Thought?

+1. Let's fix walreceiver for now, and we can revisit dblink later.
Since we haven't had any complaints so far...

OK. I'll focus on walreceiver now.

The attached patch changes walreceiver so that it uses new function
libpqrcv_PQexec() instead of PQexec() for sending handshake message.
libpqrcv_PQexec() sends a query and wait for the results by using
the asynchronous libpq functions and the backend version of select().
It's interruptible by signals. You would be able to shut the standby
server down in the middle of handshake for replication connection.

http://archives.postgresql.org/pgsql-hackers/2010-03/msg00625.php

Just replacing PQexec() with PQsendQuery() is pretty straightforward, we
could put that replacement in a file in port/win32. Replacing
PQconnectdb() is more complicated because you need to handle connection
timeout. I suggest that we only add the replacement for PQexec(), and
live with the situation for PQconnectdb(), that covers 99% of the
scenarios anyway.

According to the suggestion, I replaced only the PQexec() and didn't
add the replacement of PQconnect() for now.

http://archives.postgresql.org/pgsql-hackers/2010-04/msg00077.php

As for the code itself, don't we need a CHECK_FOR_INTERRUPTS in there
for it to be actually useful?

Since the backend version of select() receives any incoming signals
on Win32, CHECK_FOR_INTERRUPTS seems not to be needed in the loop,
at least in walreceiver. No? The patch doesn't put it in there, and
I was able to interrupt walreceiver executing libpqrcv_PQexec() when
I tested the patch on Win32.

http://archives.postgresql.org/pgsql-hackers/2010-04/msg00084.php

The larger point is that I don't believe this issue exists only on
Windows. I think we're going to want something like this on all
platforms, and that implies supporting poll() not just select() for the
waiting part.

The patch uses libpq_select() to check for the socket. It supports
poll() and select().

The patch also seems confused about whether it's intending to be a
general-purpose solution or not. You can maybe take some shortcuts
if it's only going to be for walreceiver, but if you're going to put
it in dblink it is *not* acceptable to take shortcuts like not
processing errors completely.

The patch still takes some shortcuts since we decided to postpone
the fix for dblink to 9.1 or later.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Wed, Apr 14, 2010 at 10:02 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Apr 13, 2010 at 9:21 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Apr 13, 2010 at 1:56 AM, Magnus Hagander <magnus@hagander.net> wrote:

If adding new shared library is too big change at this point, I think
that we should postpone the fix only for dblink to 9.1 or later. Since
no one has complained about this long-term problem of dblink, I'm not
sure it really should be fixed right now. Thought?

+1. Let's fix walreceiver for now, and we can revisit dblink later.
Since we haven't had any complaints so far...

OK. I'll focus on walreceiver now.

The attached patch changes walreceiver so that it uses new function
libpqrcv_PQexec() instead of PQexec() for sending handshake message.
libpqrcv_PQexec() sends a query and wait for the results by using
the asynchronous libpq functions and the backend version of select().
It's interruptible by signals. You would be able to shut the standby
server down in the middle of handshake for replication connection.

http://archives.postgresql.org/pgsql-hackers/2010-03/msg00625.php

Just replacing PQexec() with PQsendQuery() is pretty straightforward, we
could put that replacement in a file in port/win32. Replacing
PQconnectdb() is more complicated because you need to handle connection
timeout. I suggest that we only add the replacement for PQexec(), and
live with the situation for PQconnectdb(), that covers 99% of the
scenarios anyway.

According to the suggestion, I replaced only the PQexec() and didn't
add the replacement of PQconnect() for now.

http://archives.postgresql.org/pgsql-hackers/2010-04/msg00077.php

As for the code itself, don't we need a CHECK_FOR_INTERRUPTS in there
for it to be actually useful?

Since the backend version of select() receives any incoming signals
on Win32, CHECK_FOR_INTERRUPTS seems not to be needed in the loop,
at least in walreceiver. No? The patch doesn't put it in there, and
I was able to interrupt walreceiver executing libpqrcv_PQexec() when
I tested the patch on Win32.

It will call the signal handler, yes. Normally, the signal handler
just sets a flag somewhere, which needs to be checked with
CHECK_FOR_INTERRUPTS.

From how I read the walreceiver.c code (which I'm not that familiar
with), the signal handlers call ProcessWalRcvInterrupts() which in
turn has CHECK_FOR_INTERRUPTS in it, and this is where it ends up
being called.

The patch also seems confused about whether it's intending to be a
general-purpose solution or not.  You can maybe take some shortcuts
if it's only going to be for walreceiver, but if you're going to put
it in dblink it is *not* acceptable to take shortcuts like not
processing errors completely.

The patch still takes some shortcuts since we decided to postpone
the fix for dblink to 9.1 or later.

Given those shortcuts, can't we simplify it even further like
attached? (If nothing else, your code did PQclear() on an
uninitialized pointer - must've been pure luck that it worked)

Looking at the call-sites, there are bugs now - if PQexec() returns
NULL, we don't deal with it. It also doesn't always free the result
properly. I've added checks for that.

Finally, I've updated some of the comments.

Note: I've only tested this patch as far as that it compiles. I don't
have a SR env around right now, so I'll have to complete with that
later. But if you have a chance to test that it fixes your test case,
please do!

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:

Looking at the call-sites, there are bugs now - if PQexec() returns
NULL, we don't deal with it. It also doesn't always free the result
properly. I've added checks for that.

I think you're just adding useless complexity there. PQresultStatus
defends itself just fine against a NULL input, and most other libpq
functions likewise.

regards, tom lane

On Wed, Apr 14, 2010 at 11:15 PM, Magnus Hagander <magnus@hagander.net> wrote:

http://archives.postgresql.org/pgsql-hackers/2010-04/msg00077.php

As for the code itself, don't we need a CHECK_FOR_INTERRUPTS in there
for it to be actually useful?

Since the backend version of select() receives any incoming signals
on Win32, CHECK_FOR_INTERRUPTS seems not to be needed in the loop,
at least in walreceiver. No? The patch doesn't put it in there, and
I was able to interrupt walreceiver executing libpqrcv_PQexec() when
I tested the patch on Win32.

It will call the signal handler, yes. Normally, the signal handler
just sets a flag somewhere, which needs to be checked with
CHECK_FOR_INTERRUPTS.

From how I read the walreceiver.c code (which I'm not that familiar
with), the signal handlers call ProcessWalRcvInterrupts() which in
turn has CHECK_FOR_INTERRUPTS in it, and this is where it ends up
being called.

Yes. While establishing replication connection (i.e., executing
walrcv_connect function), the SIGTERM signal handler directly calls
ProcessWalRcvInterrupts() which does CHECK_FOR_INTERRUPTS() and
elog(FATAL).

The patch also seems confused about whether it's intending to be a
general-purpose solution or not.  You can maybe take some shortcuts
if it's only going to be for walreceiver, but if you're going to put
it in dblink it is *not* acceptable to take shortcuts like not
processing errors completely.

The patch still takes some shortcuts since we decided to postpone
the fix for dblink to 9.1 or later.

Given those shortcuts, can't we simplify it even further like
attached?

No, we need to repeat PQgetResult() at least two times. The first call
of it reads the RowDescription, DataRow and CommandComplete messages
and switches the state to PGASYNC_READY. The second one reads the
ReadyForQuery message and switches the state to PGASYNC_IDLE. So if we
don't repeat it, libpqrcv_PQexec() would end in a half-finished state.

(If nothing else, your code did PQclear() on an
uninitialized pointer - must've been pure luck that it worked)

PQclear(NULL) might be called in my patch, but this is not a problem
since PQclear() does nothing if the specified PGresult argument is NULL.

Looking at the call-sites, there are bugs now - if PQexec() returns
NULL, we don't deal with it. It also doesn't always free the result
properly. I've added checks for that.

As Tom pointed out in another post, we don't need to treat the
"result is NULL" case as special. OTOH, as you pointed out, I
forgot calling PQclear() when the second call of libpqrcv_PQexec()
in libpqrcv_connect() fails. I added it to the patch. Thanks!

Finally, I've updated some of the comments.

Thanks a lot! I applied that improvements to the patch.

I attached the revised patch.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Thu, Apr 15, 2010 at 4:17 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

Looking at the call-sites, there are bugs now - if PQexec() returns
NULL, we don't deal with it. It also doesn't always free the result
properly. I've added checks for that.

I think you're just adding useless complexity there.  PQresultStatus
defends itself just fine against a NULL input, and most other libpq
functions likewise.

Yeah, I realized that after posting it. I was still stuck in ancient
times when at least some of those functions couldn't be called with
NULL pointers, so I just put that in there by default :-)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Thu, Apr 15, 2010 at 5:13 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Wed, Apr 14, 2010 at 11:15 PM, Magnus Hagander <magnus@hagander.net> wrote:

The patch also seems confused about whether it's intending to be a
general-purpose solution or not.  You can maybe take some shortcuts
if it's only going to be for walreceiver, but if you're going to put
it in dblink it is *not* acceptable to take shortcuts like not
processing errors completely.

The patch still takes some shortcuts since we decided to postpone
the fix for dblink to 9.1 or later.

Given those shortcuts, can't we simplify it even further like
attached?

No, we need to repeat PQgetResult() at least two times. The first call
of it reads the RowDescription, DataRow and CommandComplete messages
and switches the state to PGASYNC_READY. The second one reads the
ReadyForQuery message and switches the state to PGASYNC_IDLE. So if we
don't repeat it, libpqrcv_PQexec() would end in a half-finished state.

Ah, ok. That's what I get for not testing it :-)

I still think that could be implemented in a much clearer way though.
Just calling PQgetResult() twice, and checking the return values
sequentially would be much easier to read, imho. Looking through taht
set of "break" statements at the end of the loop is just confusing. If
nothing else, it needs more comments.

But maybe I'm just bikeshedding ;)

(If nothing else, your code did PQclear() on an
uninitialized pointer - must've been pure luck that it worked)

PQclear(NULL) might be called in my patch, but this is not a problem
since PQclear() does nothing if the specified PGresult argument is NULL.

Ah, I missed that you initialized it to NULL.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Wed, Apr 14, 2010 at 11:13 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Wed, Apr 14, 2010 at 11:15 PM, Magnus Hagander <magnus@hagander.net> wrote:

http://archives.postgresql.org/pgsql-hackers/2010-04/msg00077.php

As for the code itself, don't we need a CHECK_FOR_INTERRUPTS in there
for it to be actually useful?

Since the backend version of select() receives any incoming signals
on Win32, CHECK_FOR_INTERRUPTS seems not to be needed in the loop,
at least in walreceiver. No? The patch doesn't put it in there, and
I was able to interrupt walreceiver executing libpqrcv_PQexec() when
I tested the patch on Win32.

It will call the signal handler, yes. Normally, the signal handler
just sets a flag somewhere, which needs to be checked with
CHECK_FOR_INTERRUPTS.

From how I read the walreceiver.c code (which I'm not that familiar
with), the signal handlers call ProcessWalRcvInterrupts() which in
turn has CHECK_FOR_INTERRUPTS in it, and this is where it ends up
being called.

Yes. While establishing replication connection (i.e., executing
walrcv_connect function), the SIGTERM signal handler directly calls
ProcessWalRcvInterrupts() which does CHECK_FOR_INTERRUPTS() and
elog(FATAL).

The patch also seems confused about whether it's intending to be a
general-purpose solution or not.  You can maybe take some shortcuts
if it's only going to be for walreceiver, but if you're going to put
it in dblink it is *not* acceptable to take shortcuts like not
processing errors completely.

The patch still takes some shortcuts since we decided to postpone
the fix for dblink to 9.1 or later.

Given those shortcuts, can't we simplify it even further like
attached?

No, we need to repeat PQgetResult() at least two times. The first call
of it reads the RowDescription, DataRow and CommandComplete messages
and switches the state to PGASYNC_READY. The second one reads the
ReadyForQuery message and switches the state to PGASYNC_IDLE. So if we
don't repeat it, libpqrcv_PQexec() would end in a half-finished state.

(If nothing else, your code did PQclear() on an
uninitialized pointer - must've been pure luck that it worked)

PQclear(NULL) might be called in my patch, but this is not a problem
since PQclear() does nothing if the specified PGresult argument is NULL.

Looking at the call-sites, there are bugs now - if PQexec() returns
NULL, we don't deal with it. It also doesn't always free the result
properly. I've added checks for that.

As Tom pointed out in another post, we don't need to treat the
"result is NULL" case as special. OTOH, as you pointed out, I
forgot calling PQclear() when the second call of libpqrcv_PQexec()
in libpqrcv_connect() fails. I added it to the patch. Thanks!

Finally, I've updated some of the comments.

Thanks a lot! I applied that improvements to the patch.

I attached the revised patch.

I have to admit to finding this confusing. According to the comments:

+ 		/*
+ 		 * Don't emulate the PQexec()'s behavior of returning the last
+ 		 * result when there are many, since walreceiver never sends a
+ 		 * query returning multiple results.
+ 		 */

...but it looks like the code actually is implementing some sort of
loop-that-returns-the-last result.

...Robert

On Thu, Apr 15, 2010 at 11:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I have to admit to finding this confusing.  According to the comments:

+               /*
+                * Don't emulate the PQexec()'s behavior of returning the last
+                * result when there are many, since walreceiver never sends a
+                * query returning multiple results.
+                */

...but it looks like the code actually is implementing some sort of
loop-that-returns-the-last result.

Yeah, it's not a very accurate description. And I found another problem:
libpqrcv_PQexec() ends as soon as an error result arrives even if its
state has not been PGASYNC_IDLE yet.

So I changed libpqrcv_PQexec() so that it emulates the PQexec()'s behavior
except the concatenation of error messages. How about the attached patch?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Fri, Apr 16, 2010 at 3:03 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Thu, Apr 15, 2010 at 11:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I have to admit to finding this confusing.  According to the comments:

+               /*
+                * Don't emulate the PQexec()'s behavior of returning the last
+                * result when there are many, since walreceiver never sends a
+                * query returning multiple results.
+                */

...but it looks like the code actually is implementing some sort of
loop-that-returns-the-last result.

Yeah, it's not a very accurate description. And I found another problem:
libpqrcv_PQexec() ends as soon as an error result arrives even if its
state has not been PGASYNC_IDLE yet.

So I changed libpqrcv_PQexec() so that it emulates the PQexec()'s behavior
except the concatenation of error messages. How about the attached patch?

Well, the comment definitely makes more sense in this version. I
can't speak to the behavior.

...Robert

On Fri, Apr 16, 2010 at 4:03 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Thu, Apr 15, 2010 at 11:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I have to admit to finding this confusing.  According to the comments:

+               /*
+                * Don't emulate the PQexec()'s behavior of returning the last
+                * result when there are many, since walreceiver never sends a
+                * query returning multiple results.
+                */

...but it looks like the code actually is implementing some sort of
loop-that-returns-the-last result.

Yeah, it's not a very accurate description. And I found another problem:
libpqrcv_PQexec() ends as soon as an error result arrives even if its
state has not been PGASYNC_IDLE yet.

So I changed libpqrcv_PQexec() so that it emulates the PQexec()'s behavior
except the concatenation of error messages. How about the attached patch?

BTW, even if you apply the patch, you would not be able to interrupt the
walreceiver by using smart shutdown because of the bug reported in another
thread. Please be careful about that when you test the patch.
http://archives.postgresql.org/pgsql-hackers/2010-04/msg00592.php

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Fri, Apr 16, 2010 at 09:03, Fujii Masao <masao.fujii@gmail.com> wrote:

On Thu, Apr 15, 2010 at 11:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I have to admit to finding this confusing.  According to the comments:

+               /*
+                * Don't emulate the PQexec()'s behavior of returning the last
+                * result when there are many, since walreceiver never sends a
+                * query returning multiple results.
+                */

...but it looks like the code actually is implementing some sort of
loop-that-returns-the-last result.

Yeah, it's not a very accurate description. And I found another problem:
libpqrcv_PQexec() ends as soon as an error result arrives even if its
state has not been PGASYNC_IDLE yet.

So I changed libpqrcv_PQexec() so that it emulates the PQexec()'s behavior
except the concatenation of error messages. How about the attached patch?

Applied with only minor stylistic changes. Thanks!

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/