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: Hari Babu <haribabu(dot)kommi(at)huawei(dot)com>
To: "'Boszormenyi Zoltan'" <zb(at)cybertec(dot)at>
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-02 07:11:15
Message-ID: 01f001cde8b8$5b457ec0$11d07c40$@kommi@huawei.com (view raw or flat)
Thread:
Lists: pgsql-bugspgsql-hackers
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.

Please let me know if anything apart from above needs to be taken care.

Regards,
Hari babu.




In response to

pgsql-hackers by date

Next:From: Brendan JurdDate: 2013-01-02 07:37:58
Subject: Function to get size of notification queue?
Previous:From: Andrew DunstanDate: 2013-01-02 03:14:08
Subject: Re: pgsql: Unify some tar functionality across different parts

pgsql-bugs by date

Next:From: Sell, KennethDate: 2013-01-03 02:22:08
Subject: Re: Database crash in Postgres 8.4.3
Previous:From: giorgio.galloDate: 2013-01-01 22:09:05
Subject: BUG #7780: unaccent extension - dictionary not filtering?

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