Re: [HACKERS] Wrong defeinition of pq_putmessage_noblock since 9.5

2016-08-02 Thread Robert Haas
On Fri, Jul 29, 2016 at 11:58 AM, Tom Lane  wrote:
> Maybe the real problem here is that the abstraction layer is so incomplete
> and apparently haphazard, so that it's not very obvious where you should
> use a pq_xxx name and where you should refer to socket_xxx.  For some of
> the functions in pqcomm.c, such as internal_flush, it's impossible to tell
> which side of the abstraction boundary they're supposed to be on.
> (I suspect that that particular function has simply been ignored on the
> undocumented assumption that a bgworker could never call it, but that's
> the kind of half-baked work that I don't find acceptable.)
>
> I think the core work that needs to be done here is to clearly identify
> each function in pqcomm.c as either above or below the PqCommMethods
> abstraction boundary.  Maybe we should split one set or the other out
> to a new source file.  In any case, the naming convention ought to
> reflect that hierarchy.
>
> I withdraw the idea that we should try to revert all these functions back
> to their old names, since that would obscure the layering distinction
> between socket-specific and general-usage functions.  I now think that
> the problem is that that distinction hasn't been drawn sharply enough.

If memory serves, there was some discussion of this at the time the
patch that changed this was originally submitted.  The original patch,
IIRC, just installed one or two hooks and it was argued that I should
instead create some kind of abstraction layer.  The present coding is
the result of my attempt to do just that.  I have to admit that I
wasn't very eager to churn the contents of this file more than
necessary.  It seems like old, crufty code to me.  I don't object if
you want to refactor it, but I'm not sure what problem it solves.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] Wrong defeinition of pq_putmessage_noblock since 9.5

2016-08-01 Thread Kyotaro HORIGUCHI
At Fri, 29 Jul 2016 11:58:04 -0400, Tom Lane  wrote in 
<14846.1469807...@sss.pgh.pa.us>
> Kyotaro HORIGUCHI  writes:
> > The many of the socket_* functions are required to be replaced
> > with mq_* functions for backgroud workers. So reverting the names
> > of socket_* functions should be accompanied by the need to, for
> > example, changing their callers to use them explicitly via
> > PqCommMethods, not hiding with macros, or renaming current pq_*
> > macros to, say, pqi_. (it stands for PQ-Indirect as a tentative)
> > I'm not confident on the new prefix since I'm not sure that what
> > the 'pq' stands for. (Postgres Query?)
> 
> Hmm.  Of course the problem with this approach is that we end up touching
> a whole bunch of call sites, which sort of negates the point of having
> installed a compatibility macro layer.
> 
> [ spends awhile longer looking around at this code ]
> 
> Maybe the real problem here is that the abstraction layer is so incomplete
> and apparently haphazard, so that it's not very obvious where you should
> use a pq_xxx name and where you should refer to socket_xxx.

Yes, 'haphazard' is the word I was seeking for.

> For some of
> the functions in pqcomm.c, such as internal_flush, it's impossible to tell
> which side of the abstraction boundary they're supposed to be on.
> (I suspect that that particular function has simply been ignored on the
> undocumented assumption that a bgworker could never call it, but that's
> the kind of half-baked work that I don't find acceptable.)
> 
> I think the core work that needs to be done here is to clearly identify
> each function in pqcomm.c as either above or below the PqCommMethods
> abstraction boundary.  Maybe we should split one set or the other out
> to a new source file.  In any case, the naming convention ought to
> reflect that hierarchy.

Agreed. I'm not sure if there's a clean boundary, though.

> I withdraw the idea that we should try to revert all these functions back
> to their old names, since that would obscure the layering distinction
> between socket-specific and general-usage functions.  I now think that
> the problem is that that distinction hasn't been drawn sharply enough.
> 
> > The set of functions in PQcommMethods doesn't seem clean. They
> > are chosen arbitrarily just so that other pq_* functions used in
> > parallel workers will work as expected. I suppose that it needs
> > some refactoring.
> 
> Yeah, exactly.
> 
> > Any work in this area is likely 10.0 material at this point.
> 
> I'm not really happy with that, since refactoring it again will create
> back-patch hazards.  But I see that a lot of the mess here was created
> in 9.5, which means we're probably stuck with back-patch hazards anyway.

I think we have an alternative not to backpatch that
refactoring. 9.5 works with it happily and won't get further
improovement in this area.

>   regards, tom lane
> 
> PS: "PQ" comes from PostQUEL, I believe.

Thanks, I've learned something new:)

https://en.wikipedia.org/wiki/QUEL_query_languages

> range of E is EMPLOYEE
> retrieve into W
> (COMP = E.Salary / (E.Age - 18))
> where E.Name = "Jones"

It looks more methematical or programming-language-like to me
than SQL.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
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] Wrong defeinition of pq_putmessage_noblock since 9.5

2016-08-01 Thread Kyotaro HORIGUCHI
At Fri, 29 Jul 2016 13:00:50 -0400, Tom Lane  wrote in 
<29430.1469811...@sss.pgh.pa.us>
> I wrote:
> > Kyotaro HORIGUCHI  writes:
> >> Any work in this area is likely 10.0 material at this point.
> 
> > I'm not really happy with that, since refactoring it again will create
> > back-patch hazards.  But I see that a lot of the mess here was created
> > in 9.5, which means we're probably stuck with back-patch hazards anyway.
> 
> I've pushed Kyotaro-san's original patch, which is clearly a bug fix.
> I think the additional changes discussed later in this thread are
> cosmetic, and probably should wait for a more general review of the
> layering decisions in pqcomm.c.

Agreed. It's not an abstraction or API, but it actually works
with no problem and changing it is an issue obviously for later
discussion.

Anyway thank you for committing this.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
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] Wrong defeinition of pq_putmessage_noblock since 9.5

2016-07-29 Thread Tom Lane
I wrote:
> Kyotaro HORIGUCHI  writes:
>> Any work in this area is likely 10.0 material at this point.

> I'm not really happy with that, since refactoring it again will create
> back-patch hazards.  But I see that a lot of the mess here was created
> in 9.5, which means we're probably stuck with back-patch hazards anyway.

I've pushed Kyotaro-san's original patch, which is clearly a bug fix.
I think the additional changes discussed later in this thread are
cosmetic, and probably should wait for a more general review of the
layering decisions in pqcomm.c.

regards, tom lane


-- 
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] Wrong defeinition of pq_putmessage_noblock since 9.5

2016-07-29 Thread Tom Lane
Kyotaro HORIGUCHI  writes:
> At Fri, 29 Jul 2016 12:47:53 +0900, Michael Paquier 
>  wrote in 
> 
>>> At Thu, 28 Jul 2016 10:46:00 -0400, Tom Lane  wrote in 
>>> <4313.1469717...@sss.pgh.pa.us>
>>> I dunno, this seems like it's doubling down on some extremely poor
>>> decisions.  Why is it that you now have to flip a coin to guess whether
>>> the prefix is pq_ or socket_ for functions in this module?  I would
>>> rather see that renaming reverted.

>> Yes, I agree with that. I cannot understand the intention behind
>> 2bd9e41 to rename those routines as they are now, so getting them back
>> with pg_ as prefix looks like a good idea to me.

> The many of the socket_* functions are required to be replaced
> with mq_* functions for backgroud workers. So reverting the names
> of socket_* functions should be accompanied by the need to, for
> example, changing their callers to use them explicitly via
> PqCommMethods, not hiding with macros, or renaming current pq_*
> macros to, say, pqi_. (it stands for PQ-Indirect as a tentative)
> I'm not confident on the new prefix since I'm not sure that what
> the 'pq' stands for. (Postgres Query?)

Hmm.  Of course the problem with this approach is that we end up touching
a whole bunch of call sites, which sort of negates the point of having
installed a compatibility macro layer.

[ spends awhile longer looking around at this code ]

Maybe the real problem here is that the abstraction layer is so incomplete
and apparently haphazard, so that it's not very obvious where you should
use a pq_xxx name and where you should refer to socket_xxx.  For some of
the functions in pqcomm.c, such as internal_flush, it's impossible to tell
which side of the abstraction boundary they're supposed to be on.
(I suspect that that particular function has simply been ignored on the
undocumented assumption that a bgworker could never call it, but that's
the kind of half-baked work that I don't find acceptable.)

I think the core work that needs to be done here is to clearly identify
each function in pqcomm.c as either above or below the PqCommMethods
abstraction boundary.  Maybe we should split one set or the other out
to a new source file.  In any case, the naming convention ought to
reflect that hierarchy.

I withdraw the idea that we should try to revert all these functions back
to their old names, since that would obscure the layering distinction
between socket-specific and general-usage functions.  I now think that
the problem is that that distinction hasn't been drawn sharply enough.

> The set of functions in PQcommMethods doesn't seem clean. They
> are chosen arbitrarily just so that other pq_* functions used in
> parallel workers will work as expected. I suppose that it needs
> some refactoring.

Yeah, exactly.

> Any work in this area is likely 10.0 material at this point.

I'm not really happy with that, since refactoring it again will create
back-patch hazards.  But I see that a lot of the mess here was created
in 9.5, which means we're probably stuck with back-patch hazards anyway.

regards, tom lane

PS: "PQ" comes from PostQUEL, I believe.


-- 
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] Wrong defeinition of pq_putmessage_noblock since 9.5

2016-07-28 Thread Kyotaro HORIGUCHI
At Fri, 29 Jul 2016 12:47:53 +0900, Michael Paquier  
wrote in 
> On Fri, Jul 29, 2016 at 12:18 PM, Kyotaro HORIGUCHI
>  wrote:
> > At Thu, 28 Jul 2016 10:46:00 -0400, Tom Lane  wrote in 
> > <4313.1469717...@sss.pgh.pa.us>
> >> Fujii Masao  writes:
> >> > 3. Several source comments in pqcomm.c have not been updated.
> >> > Some comments still use the old function name like pq_putmessage().
> >>
> >> > Attached patch fixes the above issues.
> >>
> >> I dunno, this seems like it's doubling down on some extremely poor
> >> decisions.  Why is it that you now have to flip a coin to guess whether
> >> the prefix is pq_ or socket_ for functions in this module?  I would
> >> rather see that renaming reverted.
> 
> Yes, I agree with that. I cannot understand the intention behind
> 2bd9e41 to rename those routines as they are now, so getting them back
> with pg_ as prefix looks like a good idea to me.

The many of the socket_* functions are required to be replaced
with mq_* functions for backgroud workers. So reverting the names
of socket_* functions should be accompanied by the need to, for
example, changing their callers to use them explicitly via
PqCommMethods, not hiding with macros, or renaming current pq_*
macros to, say, pqi_. (it stands for PQ-Indirect as a tentative)
I'm not confident on the new prefix since I'm not sure that what
the 'pq' stands for. (Postgres Query?)

Attached patch is a rush work to revert the names of socket_
functions and replace the prefix of the macros with "pqi". pq_
names no longer points to mq_ functions. Is this roughly on the
right way? Even though the prefix is not appropriate.

> > The set of functions in PQcommMethods doesn't seem clean. They
> > are chosen arbitrarily just so that other pq_* functions used in
> > parallel workers will work as expected. I suppose that it needs
> > some refactoring.
> 
> Any work in this area is likely 10.0 material at this point.
> 
> > By the way, pq_start/endcopyout() are used only in FE protocols
> > below 3.0, which had already bacome obsolete as of PG7.4. While
> > the next dev cycle is for PG10, if there is no particular reason
> > to support such ancient protocols, removing them would make things
> > easier and cleaner.
> 
> Remove support for protocol 2 has been in the air for some time, but
> that's a separate discussion. If you want to discuss this issue
> particularly, raising a new thread would be a good idea.

Thanks. I saw in somewhere that the protocol 2 no longer
works. I'll raise a new thread later.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From b38b9a21d8cc73f3c20df46fb0db17db237c27ac Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Fri, 29 Jul 2016 14:45:15 +0900
Subject: [PATCH] Revert socket_ functions' name to pq_ names.

---
 src/backend/access/transam/parallel.c |  2 +-
 src/backend/commands/async.c  |  2 +-
 src/backend/commands/copy.c   | 12 ++---
 src/backend/libpq/auth.c  |  2 +-
 src/backend/libpq/pqcomm.c| 84 +--
 src/backend/libpq/pqformat.c  |  8 ++--
 src/backend/replication/basebackup.c  | 14 +++---
 src/backend/replication/walsender.c   | 44 +-
 src/backend/tcop/dest.c   |  6 +--
 src/backend/tcop/postgres.c   |  4 +-
 src/backend/utils/error/elog.c|  4 +-
 src/include/libpq/libpq.h | 16 +++
 12 files changed, 99 insertions(+), 99 deletions(-)

diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index eef1dc2..1ab2921 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -1072,7 +1072,7 @@ ParallelWorkerMain(Datum main_arg)
 	EndParallelWorkerTransaction();
 
 	/* Report success. */
-	pq_putmessage('X', NULL, 0);
+	pqi_putmessage('X', NULL, 0);
 }
 
 /*
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 716f1c3..987c05e 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -2062,7 +2062,7 @@ ProcessIncomingNotify(void)
 	/*
 	 * Must flush the notify messages to ensure frontend gets them promptly.
 	 */
-	pq_flush();
+	pqi_flush();
 
 	set_ps_display("idle", false);
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f45b330..a41eb00 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -362,7 +362,7 @@ SendCopyBegin(CopyState cstate)
 			errmsg("COPY BINARY is not supported to stdout or from stdin")));
 		pq_putemptymessage('H');
 		/* grottiness needed for old COPY OUT protocol */
-		pq_startcopyout();
+		pqi_startcopyout();
 		cstate->copy_dest = COPY_OLD_FE;
 	}
 	else
@@ -374,7 +374,7 @@ SendCopyBegin(CopyState cstate)
 			errmsg("COPY BINARY is not supported to stdout or from stdin")));
 		pq_putemptymessage('B');
 		/* grottiness needed for old COPY OUT protocol */
-		pq_startcopyout();
+		pqi_startcopyout();
 		cstate->copy_dest = COPY_OLD_FE;

Re: [HACKERS] Wrong defeinition of pq_putmessage_noblock since 9.5

2016-07-28 Thread Michael Paquier
On Fri, Jul 29, 2016 at 12:18 PM, Kyotaro HORIGUCHI
 wrote:
> At Thu, 28 Jul 2016 10:46:00 -0400, Tom Lane  wrote in 
> <4313.1469717...@sss.pgh.pa.us>
>> Fujii Masao  writes:
>> > 3. Several source comments in pqcomm.c have not been updated.
>> > Some comments still use the old function name like pq_putmessage().
>>
>> > Attached patch fixes the above issues.
>>
>> I dunno, this seems like it's doubling down on some extremely poor
>> decisions.  Why is it that you now have to flip a coin to guess whether
>> the prefix is pq_ or socket_ for functions in this module?  I would
>> rather see that renaming reverted.

Yes, I agree with that. I cannot understand the intention behind
2bd9e41 to rename those routines as they are now, so getting them back
with pg_ as prefix looks like a good idea to me.

> The set of functions in PQcommMethods doesn't seem clean. They
> are chosen arbitrarily just so that other pq_* functions used in
> parallel workers will work as expected. I suppose that it needs
> some refactoring.

Any work in this area is likely 10.0 material at this point.

> By the way, pq_start/endcopyout() are used only in FE protocols
> below 3.0, which had already bacome obsolete as of PG7.4. While
> the next dev cycle is for PG10, if there is no particular reason
> to support such ancient protocols, removing them would make things
> easier and cleaner.

Remove support for protocol 2 has been in the air for some time, but
that's a separate discussion. If you want to discuss this issue
particularly, raising a new thread would be a good idea.
-- 
Michael


-- 
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] Wrong defeinition of pq_putmessage_noblock since 9.5

2016-07-28 Thread Kyotaro HORIGUCHI
At Thu, 28 Jul 2016 10:46:00 -0400, Tom Lane  wrote in 
<4313.1469717...@sss.pgh.pa.us>
> Fujii Masao  writes:
> > 3. Several source comments in pqcomm.c have not been updated.
> > Some comments still use the old function name like pq_putmessage().
> 
> > Attached patch fixes the above issues.
> 
> I dunno, this seems like it's doubling down on some extremely poor
> decisions.  Why is it that you now have to flip a coin to guess whether
> the prefix is pq_ or socket_ for functions in this module?  I would
> rather see that renaming reverted.

The set of functions in PQcommMethods doesn't seem clean. They
are choosed arbitrary just so that other pq_* functions used in
parallel workers will work as expected. I suppose that it needs
some refactoring.

By the way, pq_start/endcopyout() are used only in FE protocols
below 3.0, which had already bacome obsolete as of PG7.4. While
the next dev cycle is for PG10, if there is no particular reason
to support such acient protocols, removing them would make things
easier and cleaner.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
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] Wrong defeinition of pq_putmessage_noblock since 9.5

2016-07-28 Thread Tom Lane
Fujii Masao  writes:
> 3. Several source comments in pqcomm.c have not been updated.
> Some comments still use the old function name like pq_putmessage().

> Attached patch fixes the above issues.

I dunno, this seems like it's doubling down on some extremely poor
decisions.  Why is it that you now have to flip a coin to guess whether
the prefix is pq_ or socket_ for functions in this module?  I would
rather see that renaming reverted.

regards, tom lane


-- 
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] Wrong defeinition of pq_putmessage_noblock since 9.5

2016-07-28 Thread Fujii Masao
On Thu, Jul 28, 2016 at 9:08 PM, Fujii Masao  wrote:
> On Thu, Jul 28, 2016 at 6:52 PM, Kyotaro HORIGUCHI
>  wrote:
>> Hello,
>>
>> While testing replication for 9.5, we found that repl-master can
>> ignore wal_sender_timeout and seemingly waits for TCP
>> retransmission timeout for the case of sudden power-off of a
>> standby.
>>
>> My investigation told me that the immediate cause could be that
>> secure_write() is called with *blocking mode* (that is,
>> port->noblock = false) under *pq_putmessage_noblock* macro called
>> from XLogSendPhysical().
>>
>> libpq.h of 9.5 and newer defines it as the following,
>>
>>> #define pq_putmessage(msgtype, s, len) \
>>>   (PqCommMethods->putmessage(msgtype, s, len))
>>> #define pq_putmessage_noblock(msgtype, s, len) \
>>>   (PqCommMethods->putmessage(msgtype, s, len))
>>
>> which is apparently should be the following.
>>
>>> #define pq_putmessage_noblock(msgtype, s, len) \
>>>   (PqCommMethods->putmessage_noblock(msgtype, s, len))
>>
>> The attached patch fixes it.
>
> Good catch! Barring objection, I will push this to both master and 9.5.

Regarding this patch, while reading pqcomm.c, I found the following things.

1. socket_comm_reset() calls pq_endcopyout().
I think that socket_endcopyout() should be called, instead.

2. socket_putmessage_noblock() calls pq_putmessage().
I think that socket_putmessage() should be called, instead.

3. Several source comments in pqcomm.c have not been updated.
Some comments still use the old function name like pq_putmessage().

Attached patch fixes the above issues.

Regards,

-- 
Fujii Masao
*** a/src/backend/libpq/pqcomm.c
--- b/src/backend/libpq/pqcomm.c
***
*** 18,24 
   * NOTE: generally, it's a bad idea to emit outgoing messages directly with
   * pq_putbytes(), especially if the message would require multiple calls
   * to send.  Instead, use the routines in pqformat.c to construct the message
!  * in a buffer and then emit it in one call to pq_putmessage.  This ensures
   * that the channel will not be clogged by an incomplete message if execution
   * is aborted by ereport(ERROR) partway through the message.  The only
   * non-libpq code that should call pq_putbytes directly is old-style COPY OUT.
--- 18,24 
   * NOTE: generally, it's a bad idea to emit outgoing messages directly with
   * pq_putbytes(), especially if the message would require multiple calls
   * to send.  Instead, use the routines in pqformat.c to construct the message
!  * in a buffer and then emit it in one call to socket_putmessage.  This ensures
   * that the channel will not be clogged by an incomplete message if execution
   * is aborted by ereport(ERROR) partway through the message.  The only
   * non-libpq code that should call pq_putbytes directly is old-style COPY OUT.
***
*** 44,50 
   *		StreamClose			- Close a client/backend connection
   *		TouchSocketFiles	- Protect socket files against /tmp cleaners
   *		pq_init			- initialize libpq at backend startup
!  *		pq_comm_reset	- reset libpq during error recovery
   *		pq_close		- shutdown libpq at backend exit
   *
   * low-level I/O:
--- 44,50 
   *		StreamClose			- Close a client/backend connection
   *		TouchSocketFiles	- Protect socket files against /tmp cleaners
   *		pq_init			- initialize libpq at backend startup
!  *		socket_comm_reset	- reset libpq during error recovery
   *		pq_close		- shutdown libpq at backend exit
   *
   * low-level I/O:
***
*** 53,68 
   *		pq_getmessage	- get a message with length word from connection
   *		pq_getbyte		- get next byte from connection
   *		pq_peekbyte		- peek at next byte from connection
!  *		pq_putbytes		- send bytes to connection (not flushed until pq_flush)
!  *		pq_flush		- flush pending output
!  *		pq_flush_if_writable - flush pending output if writable without blocking
   *		pq_getbyte_if_available - get a byte if available without blocking
   *
   * message-level I/O (and old-style-COPY-OUT cruft):
!  *		pq_putmessage	- send a normal message (suppressed in COPY OUT mode)
!  *		pq_putmessage_noblock - buffer a normal message (suppressed in COPY OUT)
!  *		pq_startcopyout - inform libpq that a COPY OUT transfer is beginning
!  *		pq_endcopyout	- end a COPY OUT transfer
   *
   *
   */
--- 53,68 
   *		pq_getmessage	- get a message with length word from connection
   *		pq_getbyte		- get next byte from connection
   *		pq_peekbyte		- peek at next byte from connection
!  *		pq_putbytes		- send bytes to connection (not flushed until socket_flush)
!  *		socket_flush		- flush pending output
!  *		socket_flush_if_writable - flush pending output if writable without blocking
   *		pq_getbyte_if_available - get a byte if available without blocking
   *
   * message-level I/O (and old-style-COPY-OUT cruft):
!  *		socket_putmessage	- send a normal message (suppressed in COPY OUT mode)
!  *		socket_putmessage_noblock - buffer a normal message (s

Re: [HACKERS] Wrong defeinition of pq_putmessage_noblock since 9.5

2016-07-28 Thread Fujii Masao
On Thu, Jul 28, 2016 at 6:52 PM, Kyotaro HORIGUCHI
 wrote:
> Hello,
>
> While testing replication for 9.5, we found that repl-master can
> ignore wal_sender_timeout and seemingly waits for TCP
> retransmission timeout for the case of sudden power-off of a
> standby.
>
> My investigation told me that the immediate cause could be that
> secure_write() is called with *blocking mode* (that is,
> port->noblock = false) under *pq_putmessage_noblock* macro called
> from XLogSendPhysical().
>
> libpq.h of 9.5 and newer defines it as the following,
>
>> #define pq_putmessage(msgtype, s, len) \
>>   (PqCommMethods->putmessage(msgtype, s, len))
>> #define pq_putmessage_noblock(msgtype, s, len) \
>>   (PqCommMethods->putmessage(msgtype, s, len))
>
> which is apparently should be the following.
>
>> #define pq_putmessage_noblock(msgtype, s, len) \
>>   (PqCommMethods->putmessage_noblock(msgtype, s, len))
>
> The attached patch fixes it.

Good catch! Barring objection, I will push this to both master and 9.5.

Regards,

-- 
Fujii Masao


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


[HACKERS] Wrong defeinition of pq_putmessage_noblock since 9.5

2016-07-28 Thread Kyotaro HORIGUCHI
Hello,

While testing replication for 9.5, we found that repl-master can
ignore wal_sender_timeout and seemingly waits for TCP
retransmission timeout for the case of sudden power-off of a
standby.

My investigation told me that the immediate cause could be that
secure_write() is called with *blocking mode* (that is,
port->noblock = false) under *pq_putmessage_noblock* macro called
from XLogSendPhysical().

libpq.h of 9.5 and newer defines it as the following,

> #define pq_putmessage(msgtype, s, len) \
>   (PqCommMethods->putmessage(msgtype, s, len))
> #define pq_putmessage_noblock(msgtype, s, len) \
>   (PqCommMethods->putmessage(msgtype, s, len))

which is apparently should be the following.

> #define pq_putmessage_noblock(msgtype, s, len) \
>   (PqCommMethods->putmessage_noblock(msgtype, s, len))

The attached patch fixes it.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From b15db80734215b7e3b4bbae849cf89ffd990e8be Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 28 Jul 2016 18:28:57 +0900
Subject: [PATCH] Fix the defeinition of pq_putmessage_noblock macro.

pq_putmessage_noblock marcro is mistakenly defined as the same with
pg_putmessage. Fix it.
---
 src/include/libpq/libpq.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
index 547d3b8..18052cb 100644
--- a/src/include/libpq/libpq.h
+++ b/src/include/libpq/libpq.h
@@ -43,7 +43,7 @@ extern PGDLLIMPORT PQcommMethods *PqCommMethods;
 #define pq_putmessage(msgtype, s, len) \
 	(PqCommMethods->putmessage(msgtype, s, len))
 #define pq_putmessage_noblock(msgtype, s, len) \
-	(PqCommMethods->putmessage(msgtype, s, len))
+	(PqCommMethods->putmessage_noblock(msgtype, s, len))
 #define pq_startcopyout() (PqCommMethods->startcopyout())
 #define pq_endcopyout(errorAbort) (PqCommMethods->endcopyout(errorAbort))
 
-- 
1.8.3.1


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