Re: PANIC during crash recovery of a recently promoted standby

On Fri, May 11, 2018 at 8:39 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Michael Paquier wrote:
> > On Thu, May 10, 2018 at 10:52:12AM +0530, Pavan Deolasee wrote:
> > > I propose that we should always clear the minRecoveryPoint after
> promotion
> > > to ensure that crash recovery always run to the end if a just-promoted
> > > standby crashes before completing its first regular checkpoint. A WIP
> patch
> > > is attached.
> >
> > I have been playing with your patch and upgraded the test to check as
> > well for cascading standbys. We could use that in the final patch.
> > That's definitely something to add in the recovery test suite, and the
> > sleep phases should be replaced by waits on replay and/or flush.
> >
> > Still, that approach looks sensitive to me. A restart point could be
> > running while the end-of-recovery record is inserted, so your patch
> > could update minRecoveryPoint to InvalidXLogRecPtr, and then a restart
> > point would happily update again the control file's minRecoveryPoint to
> > lastCheckPointEndPtr because it would see that the former is older than
> > lastCheckPointEndPtr (let's not forget that InvalidXLogRecPtr is 0), so
> > you could still crash on invalid pages?
>
> Yeah, I had this exact comment, but I was unable to come up with a test
> case that would cause a problem.
>

Looks like I didn't understand Alvaro's comment when he mentioned it to me
off-list. But I now see what Michael and Alvaro mean and that indeed seems
like a problem. I was thinking that the test for (ControlFile->state ==
DB_IN_ARCHIVE_RECOVERY) will ensure that minRecoveryPoint can't be updated
after the standby is promoted. While that's true for a DB_IN_PRODUCTION, the
RestartPoint may finish after we have written end-of-recovery record, but
before we're in production and thus the minRecoveryPoint may again be set.

>
> > I need to think a bit more about that stuff, but one idea would be to
> > use a special state in the control file to mark it as ending recovery,
> > this way we would control race conditions with restart points.
>
> Hmm. Can we change the control file in released branches? (It should
> be possible to make the new server understand both old and new formats,
> but I think this is breaking new ground and it looks easy to introduce
> more bugs there.)
>
>
Can't we just remember that in shared memory state instead of writing to
the control file? So if we've already performed end-of-recovery, we don't
update the minRecoveryPoint when RestartPoint completes.

Thanks,
Pavan

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