Re: PITR potentially broken in 9.2

On 2012-12-05 13:34:05 +0000, Simon Riggs wrote:
> On 5 December 2012 02:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Andres Freund <andres@2ndquadrant.com> writes:
> >>> But the key is, the database was not actually consistent at that
> >>> point, and so opening hot standby was a dangerous thing to do.
> >>>
> >>> The bug that allowed the database to open early (the original topic if
> >>> this email chain) was masking this secondary issue.
> >
> >> Could you check whether the attached patch fixes the behaviour?
> >
> > Yeah, I had just come to the same conclusion: the bug is not with
> > Heikki's fix, but with the pause logic.  The comment says that it
> > shouldn't pause unless users can connect to un-pause, but the actual
> > implementation of that test is several bricks shy of a load.
>
> OK, I've had a look at this now.
>
> Andres correctly identified the bug above, which was that the backup
> end is noted by the XLOG_BACKUP_END record. recoveryStopsHere() was
> not changed when that record type was added, so it ignores the
> situation that we are waiting for end of backup and yet it stop
> anyway. So I concur with Jeff that there is a bug and think that
> Andres has provided the clue to a fix. I'll patch that now. Aboriginal
> bug extends back to 9.0.

I think we may have multiple issues here...

RecoveryStopsHere doesn't need to check for backupEndRequired itself -
it's handled in StartupXLOG itself, just below "Complain if we did not
roll forward far enough" where it actually already checks for
backupEndRequired.

> Pause has got nothing at all to do with this, even if there are other
> problems there.

I think the problem of not erroring out properly as described in
CAMkU=1wo9+TvTy-w9UkSEgwc49kXuu=V=8cEdyaH8CBFyUVeww@mail.gmail.com and
shown in
https://docs.google.com/file/d/0Bzqrh1SO9FcEaTQ2QXhFdDZYaUE/edit?pli=1
can be attributed to the premature recoveryPausesHere() in the if()
below recoveryStopsHere() in StartupXLOG().

> > Your patch is better, but it's still missing a bet: what we need to be
> > sure of is not merely that we *could* have told the postmaster to start
> > hot standby, but that we *actually have done so*.  Otherwise, it's
> > flow-of-control-dependent whether it works or not; we could fail if the
> > main loop hasn't gotten to CheckRecoveryConsistency since the conditions
> > became true.  Looking at the code in CheckRecoveryConsistency, testing
> > LocalHotStandbyActive seems appropriate.
> >
> > I also thought it was pretty dangerous that this absolutely critical
> > test was not placed in recoveryPausesHere() itself, rather than relying
> > on the call sites to remember to do it.
> >
> > So the upshot is that I propose a patch more like the attached.
>
> I've reworked pause logic along the lines you suggest. Attached here
> for further discussion.

> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -5008,6 +5008,12 @@ recoveryStopsHere(XLogRecord *record, bool *includeThis)
>  static void
>  recoveryPausesHere(void)
>  {
> +    /*
> +     * Pause only if users can connect to send a resume message
> +     */
> +    if (!LocalHotStandbyActive)
> +        return;
> +
>      ereport(LOG,
>              (errmsg("recovery has paused"),
>               errhint("Execute pg_xlog_replay_resume() to continue.")));
> @@ -5783,10 +5789,11 @@ StartupXLOG(void)
>                  if (recoveryStopsHere(record, &recoveryApply))
>                  {
>                      /*
> -                     * Pause only if users can connect to send a resume
> -                     * message
> +                     * We've reached stop point, but not yet applied last
> +                     * record. Pause BEFORE final apply, if requested, but
> +                     * only if users can connect to send a resume message
>                       */
> -                    if (recoveryPauseAtTarget && standbyState == STANDBY_SNAPSHOT_READY)
> +                    if (recoveryPauseAtTarget && !recoveryApply)
>                      {
>                          SetRecoveryPause(true);
>                          recoveryPausesHere();

I personally would find it easier to read if we did a
SetRecoveryPause(true) regardless of !recoveryApply and just made the
recoveryPausesHere() conditional.

> @@ -5820,28 +5827,26 @@ StartupXLOG(void)
>                  }
>
>                  /*
> +                 * If we are attempting to enter Hot Standby mode, check
> +                 * for pauses and process incoming transactionids.
> +                 */
> +                if (standbyState >= STANDBY_INITIALIZED)
> +                {
> +                    if (recoveryPause)
> +                        recoveryPausesHere();
> +
> +                    if (TransactionIdIsValid(record->xl_xid))
> +                        RecordKnownAssignedTransactionIds(record->xl_xid);
> +                }
> +
> +                /*

It feels wrong to do a RecordKnownAssignedTransactionIds before setting
the replayEndRecPtr. Don't think its really hurtful, but...


>                   * Update shared replayEndRecPtr before replaying this record,
>                   * so that XLogFlush will update minRecoveryPoint correctly.
>                   */
>                  SpinLockAcquire(&xlogctl->info_lck);
>                  xlogctl->replayEndRecPtr = EndRecPtr;
> -                recoveryPause = xlogctl->recoveryPause;
>                  SpinLockRelease(&xlogctl->info_lck);
>
> -                /*
> -                 * Pause only if users can connect to send a resume message
> -                 */
> -                if (recoveryPause && standbyState == STANDBY_SNAPSHOT_READY)
> -                    recoveryPausesHere();
> -
> -                /*
> -                 * If we are attempting to enter Hot Standby mode, process
> -                 * XIDs we see
> -                 */
> -                if (standbyState >= STANDBY_INITIALIZED &&
> -                    TransactionIdIsValid(record->xl_xid))
> -                    RecordKnownAssignedTransactionIds(record->xl_xid);
> -
>                  /* Now apply the WAL record itself */
>                  RmgrTable[record->xl_rmid].rm_redo(EndRecPtr, record);
>
> @@ -5875,6 +5880,7 @@ StartupXLOG(void)
>                   */
>                  SpinLockAcquire(&xlogctl->info_lck);
>                  xlogctl->recoveryLastRecPtr = EndRecPtr;
> +                recoveryPause = xlogctl->recoveryPause;
>                  SpinLockRelease(&xlogctl->info_lck);
>
>                  LastRec = ReadRecPtr;

I think it would be better to set recoveryPause before the if
(recoveryPause) above, otherwise we may need to wait for new records to
stream in. That will only change when we see the log message about
waiting, but still...

> @@ -5883,6 +5889,17 @@ StartupXLOG(void)
>              } while (record != NULL && recoveryContinue);
>
>              /*
> +             * We've reached stop point, but not yet applied last
> +             * record. Pause AFTER final apply, if requested, but
> +             * only if users can connect to send a resume message
> +             */
> +            if (reachedStopPoint && recoveryPauseAtTarget && recoveryApply)
> +            {
> +                SetRecoveryPause(true);
> +                recoveryPausesHere();
> +            }
> +
> +            /*

I find the above comment a bit misleading because by now we have in fact
applied the last record... What about:

When we had reached the stop point we hadn't yet applied the last record
but pause *after stop* was requested, so pause now.



Independent of this patch, I am slightly confused about the whole stop
logic. Isn't the idea that you can stop/start/stop/start/... recovery?
Because if !recoveryApply we break out of the whole recovery loop and
are done with things.

Greetings,

Andres Freund

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services