Skip site navigation (1) Skip section navigation (2)

Re: Review of "pg_basebackup and pg_receivexlog to use non-blocking socket communication", was: Re: Re: [BUGS] BUG #7534: walreceiver takes long time to detect n/w breakdown

From: Boszormenyi Zoltan <zb(at)cybertec(dot)at>
To: Hari Babu <haribabu(dot)kommi(at)huawei(dot)com>
Cc: 'Fujii Masao' <masao(dot)fujii(at)gmail(dot)com>, 'Heikki Linnakangas' <hlinnakangas(at)vmware(dot)com>, pgsql-hackers(at)postgresql(dot)org, 'Amit kapila' <amit(dot)kapila(at)huawei(dot)com>
Subject: Re: Review of "pg_basebackup and pg_receivexlog to use non-blocking socket communication", was: Re: Re: [BUGS] BUG #7534: walreceiver takes long time to detect n/w breakdown
Date: 2013-01-07 14:23:02
Message-ID: 50EADA46.60802@cybertec.at (view raw or flat)
Thread:
Lists: pgsql-bugspgsql-hackers
2013-01-04 13:43 keltezéssel, Hari Babu írta:
> On January 02, 2013 12:41 PM Hari Babu wrote:
>> On January 01, 2013 10:19 PM Boszormenyi Zoltan wrote:
>>> I am reviewing your patch.
>>> • Is the patch in context diff format?
>>> Yes.
>> Thanks for reviewing the patch.
>>
>>> • Does it apply cleanly to the current git master?
>>> Not quite cleanly but it doesn't produce rejects or fuzz, only offset
> warnings:
>> Will rebase the patch to head.
>>
>>> • Does it include reasonable tests, necessary doc patches, etc?
>>> The test cases are not applicable. There is no test framework for
>>> testing network outage in "make check".
>>>
>>> There are no documentation patches for the new --recvtimeout=INTERVAL
>>> and --conntimeout=INTERVAL options for either pg_basebackup or
>>> pg_receivexlog.
>> I will add the documentation for the same.
>>
>>> Per the previous comment, no. But those are for the backend
>>> to notice network breakdowns and as such, they need a
>>> separate patch.
>> I also think it is better to handle it as a separate patch for walsender.
>>
>>> • Are the comments sufficient and accurate?
>>> This chunk below removes a comment which seems obvious enough
>>> so it's not needed:
>>> ***************
>>> *** 518,524 **** ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos,
> uint32 timeline,
>>>                          goto error;
>>>                  }
>>>    
>>> !               /* Check the message type. */
>>>                  if (copybuf[0] == 'k')
>>>                  {
>>>                          int             pos;
>>> --- 559,568 ----
>>>                          goto error;
>>>                  }
>>>    
>>> !               /* Set the last reply timestamp */
>>> !               last_recv_timestamp = localGetCurrentTimestamp();
>>> !               ping_sent = false;
>>> !
>>>                  if (copybuf[0] == 'k')
>>>                  {
>>>                          int             pos;
>>> ***************
>>>
>>> Other comments are sufficient and accurate.
>> I will fix and update the patch.
> The attached V2 patch in the mail handles all the review comments identified
> above.
>
> Regards,
> Hari babu.

Since my other patch against pg_basebackup is now committed,
this patch doesn't apply cleanly, patch rejects 2 hunks.
The fixed up patch is attached.

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/


Attachment: pg_basebkup_recvxlog_noblock_comm_v3.patch
Description: text/x-patch (25.6 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Merlin MoncureDate: 2013-01-07 15:25:16
Subject: Re: json api WIP patch
Previous:From: Andres FreundDate: 2013-01-07 14:03:21
Subject: Re: Improve compression speeds in pg_lzcompress.c

pgsql-bugs by date

Next:From: afonitDate: 2013-01-07 14:29:43
Subject: BUG #7797: datetime + '1 month'::interval is going outside of amonth's bounds
Previous:From: Magnus HaganderDate: 2013-01-07 12:09:49
Subject: Re: BUG #7792: pg_dump does not treat -c flag correctly when using tar format

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group