From: Antonin Houska <ah@cybertec.at>
To: pgsql-hackers@postgresql.org
Subject: Re: Slow I/O can break throttling of base backup
In-reply-to: <10526.1481818893@localhost>
References: 
 <CABUevEy_-e0YvL4ayoX8bH_Ja9w+BHoP6jUgdxZuG2nEj3uAfQ@mail.gmail.com>
 <10526.1481818893@localhost>
Comments: In-reply-to Antonin Houska <ah@cybertec.at>
   message dated "Thu, 15 Dec 2016 17:21:33 +0100."
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Date: Fri, 16 Dec 2016 11:24:30 +0100
Message-ID: <27929.1481883870@localhost>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

--=-=-=
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Antonin Houska <ah@cybertec.at> wrote:

> It seems to be my bug. I'll check tomorrow.

I could reproduce the problem by adding sufficient sleep time to the
loop.

> Magnus Hagander <magnus@hagander.net> wrote:
>> I wonder if the else if (sleep > 0) at the bottom of throttle() should j=
ust
>> be a simple else and always run, resetting last_throttle?

I agree. In fact, I could simplify the code even more.

Since (elapsed + sleep) almost equals to GetCurrentIntegerTimestamp(), we c=
an
use the following statement unconditionally (I think I tried too hard to av=
oid
calling GetCurrentIntegerTimestamp too often in the original patch):

throttled_last =3D GetCurrentIntegerTimestamp();

Thus we can also get rid of the "else" branch that clears both "sleep" and
"wait_result".

(The patch contains minor comment refinement that I found useful when seeing
the code after years.)

--=20
Antonin Houska
Cybertec Sch=C3=B6nig & Sch=C3=B6nig GmbH
Gr=C3=B6hrm=C3=BChlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline; filename=throttling.patch

diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
new file mode 100644
index ffc7e58..40b3c11
*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
*************** throttle(size_t increment)
*** 1370,1395 ****
  		if (wait_result & WL_LATCH_SET)
  			CHECK_FOR_INTERRUPTS();
  	}
- 	else
- 	{
- 		/*
- 		 * The actual transfer rate is below the limit.  A negative value
- 		 * would distort the adjustment of throttled_last.
- 		 */
- 		wait_result = 0;
- 		sleep = 0;
- 	}
  
  	/*
! 	 * Only a whole multiple of throttling_sample was processed. The rest will
! 	 * be done during the next call of this function.
  	 */
  	throttling_counter %= throttling_sample;
  
! 	/* Once the (possible) sleep has ended, new period starts. */
! 	if (wait_result & WL_TIMEOUT)
! 		throttled_last += elapsed + sleep;
! 	else if (sleep > 0)
! 		/* Sleep was necessary but might have been interrupted. */
! 		throttled_last = GetCurrentIntegerTimestamp();
  }
--- 1370,1385 ----
  		if (wait_result & WL_LATCH_SET)
  			CHECK_FOR_INTERRUPTS();
  	}
  
  	/*
! 	 * As we work with integers, only whole multiple of throttling_sample was
! 	 * processed. The rest will be done during the next call of this function.
  	 */
  	throttling_counter %= throttling_sample;
  
! 	/*
! 	 * Time interval for the remaining amount and possible next increments
! 	 * starts now.
! 	 */
! 	throttled_last = GetCurrentIntegerTimestamp();
  }

--=-=-=
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


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

--=-=-=--