On Wed, Oct 26, 2016 at 1:53 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Mon, Oct 24, 2016 at 12:25 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
>> On Mon, Oct 24, 2016 at 1:39 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:
>>>
>>> I think what you are saying is not completely right, because we do
>>> update minRecoveryPoint when we don't perform a new restart point.
>>> When we perform restart point, then it assumes that flushing the
>>> buffers will take care of updating minRecoveryPoint.
>>
>> Yep, minRecoveryPoint still gets updated when the last checkpoint
>> record is the last restart point to avoid a hot standby to allow
>> read-only connections at a LSN-point earlier than the last shutdown.
>> Anyway, we can clearly reject 1. in the light of
>> https://www.postgresql.org/message-id/caa4ek1kmjtsxqf0cav7cs4d4vwv2h_pc8d8q1bucqdzaf+7...@mail.gmail.com
>> when playing with different stop locations at recovery.
>>
>
> That point holds good only for cases when we try to update minimum
> recovery point beyond what is required (like earlier we were thinking
> to update it unconditionally), however what is being discussed here is
> to update only if it is not updated by flush of buffers. I think that
> is okay, but I feel Kyotaro-San's fix is a good fix for the problem
> and we don't need to add some more code (additional update of control
> file) to fix the problem.
Reading ten times my last paragraph and staring at my screen for 15
long weird minutes, I cannot recall what I had in mind when I wrote
those lines in particular..
[...scratches head...]
But yes, thinking *harder*, I agree that updating minRecoveryPoint
just after the checkpoint record would be fine and removes the need to
have more WAL than necessary in for a backup taken from a standby.
That will also prevent cases where minRecoveryPoint is older than the
recovery start point. On top of that the cost of an extra call to
UpdateControlFile() looks cheap considering that CreateRestartPoint()
is called only by the checkpointer or at shutdown.
Just coding things this solution gives roughtly the attached? The TAP
test passes btw.
--
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index d6c057a..33485de 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8829,10 +8829,6 @@ CreateRestartPoint(int flags)
* the database opened up for read-only connections at a point-in-time
* before the last shutdown. Such time travel is still possible in case of
* immediate shutdown, though.
- *
- * We don't explicitly advance minRecoveryPoint when we do create a
- * restartpoint. It's assumed that flushing the buffers will do that as a
- * side-effect.
*/
if (XLogRecPtrIsInvalid(lastCheckPointRecPtr) ||
lastCheckPoint.redo <= ControlFile->checkPointCopy.redo)
@@ -8988,6 +8984,15 @@ CreateRestartPoint(int flags)
if (EnableHotStandby)
TruncateSUBTRANS(GetOldestXmin(NULL, false));
+ /*
+ * Update minRecoveryPoint just past the last redo checkpoint if
+ * necessary. This ensures that at next startup minRecoveryPoint will
+ * not be past the next point it would start at, preventing any
+ * weird behaviors with for example backups taken from standbys that
+ * rely on minRecoveryPoint as end backup location.
+ */
+ UpdateMinRecoveryPoint(RedoRecPtr, false);
+
/* Real work is done, but log and update before releasing lock. */
LogCheckpointEnd(true);
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index fd71095..981c00b 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -24,6 +24,11 @@ $node_standby_1->start;
# pg_basebackup works on a standby).
$node_standby_1->backup($backup_name);
+# Take a second backup of the standby while the master is offline.
+$node_master->stop;
+$node_standby_1->backup('my_backup_2');
+$node_master->start;
+
# Create second standby node linking to standby 1
my $node_standby_2 = get_new_node('standby_2');
$node_standby_2->init_from_backup($node_standby_1, $backup_name,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
