Horton

Re: standby recovery fails (tablespace related) (tentative patch and discussion)

P <apraveen@pivotal.io>

View thread View on postgr.es
From: Asim R P <apraveen@pivotal.io>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>, Paul Guo <pguo@pivotal.io>, Thomas Munro <thomas.munro@gmail.com>, PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Date: 2019-09-12T12:02:41Z
Lists: pgsql-hackers

Attachments

On Thu, Sep 12, 2019 at 2:05 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:
>
> Hello.
>
> At Wed, 11 Sep 2019 17:26:44 +0300, Anastasia Lubennikova <
a.lubennikova@postgrespro.ru> wrote in <
a82a896b-93f0-c26c-b941-f5665131381b@postgrespro.ru>
> > 10.09.2019 14:42, Asim R P wrote:
> > > Hi Anastasia
> > >
> > > On Thu, Aug 22, 2019 at 9:43 PM Anastasia Lubennikova
> > > <a.lubennikova@postgrespro.ru <mailto:a.lubennikova@postgrespro.ru>>
> > > wrote:
> > > >
> > > > But during the review, I found a bug in the current implementation.
> > > > New behavior must apply to crash-recovery only, now it applies to
> > > > archiveRecovery too.
> > > > That can cause a silent loss of a tablespace during regular standby
> > > > operation
> > > > since it never calls CheckRecoveryConsistency().
>
> Yeah. We should take the same steps with redo operations on
> missing pages. Just record failure during inconsistent state then
> forget it if underlying tablespace is gone. If we had a record
> when we reached concsistency, we're in a serious situation and
> should stop recovery.  log_invalid_page forget_invalid_pages and
> CheckRecoveryConsistency are the entry points of the feature to
> understand.
>

Yes, I get it now.  I will adjust the patch written by Paul accordingly.

>
> # I remember that the start point of this patch is a crash after
> # table space drop subsequent to several operations within the
> # table space. Then, crash recovery fails at an operation in the
> # finally-removed tablespace. Is it right?
>

That's correct.  Once the directories are removed from filesystem, any
attempt to replay WAL records that depend on their existence fails.


> > > But before that I'm revisiting another solution upthread, that of
> > > creating restart points when replaying create/drop database commands
> > > before making filesystem changes such as removing a directory.  The
> > > restart points should align with checkpoints on master.  The concern
> > > against this solution was creation of restart points will slow down
> > > recovery.  I don't think crash recovery is affected by this solution
> > > because of the already existing enforcement of checkpoints.  WAL
> > > records prior to a create/drop database will not be seen by crash
> > > recovery due to the checkpoint enforced during the command's normal
> > > execution.
> > >
> >
> > I haven't measured the impact of generating extra restart points in
> > previous solution, so I cannot tell whether concerns upthread are
> > justified.  Still, I enjoy latest design more, since it is clear and
> > similar with the code of checking unexpected uninitialized pages. In
> > principle it works. And the issue, I described in previous review can
> > be easily fixed by several additional checks of InHotStandby macro.
>
> Generally we shouldn't trigger useless restart point for
> uncertain reasons. If standby crashes, it starts the next
> recovery from the latest *restart point*.  Even in that case what
> we should do is the same.
>

The reason is quite clear to me - removing directories from filesystem
break the ability to replay WAL records second time.  And we already create
checkpoints during normal operation in such a case, so crash recovery on a
master node does not suffer from this bug.  I've attached a patch that
performs restart points during drop database replay, just for reference.
It passes both the TAP tests written by Kyotaro and Paul.  I had to modify
drop database WAL record a bit.

Asim

Commits

  1. Improve recently-added test reliability

  2. Fix new recovery test for log_error_verbosity=verbose case

  3. Fix test instability

  4. Fix replay of create database records on standby

  5. Allow "in place" tablespaces.

  6. Fix get_dirent_type() for Windows junction points.

  7. Revert "Fix replay of create database records on standby"

  8. Add end-to-end testing of pg_basebackup's tar-format output.

  9. Make DROP DATABASE command generate less WAL records.

  10. Consolidate methods for translating a Perl path to a Windows path.