Re: Corruption during WAL replay

On 14/04/2020 22:04, Teja Mupparti wrote:
> Thanks Kyotaro and Masahiko for the feedback. I think there is a
> consensus on the critical-section around truncate,

+1

> but I just want to emphasize the need for reversing the order of the
> dropping the buffers and the truncation.
>
>  Repro details (when full page write = off)
>
>          1) Page on disk has empty LP 1, Insert into page LP 1
>          2) checkpoint START (Recovery REDO eventually starts here)
>          3) Delete all rows on the page (page is empty now)
>          4) Autovacuum kicks in and truncates the pages
>                  DropRelFileNodeBuffers - Dirty page NOT written, LP 1
> on disk still empty
>          5) Checkpoint completes
>          6) Crash
>          7) smgrtruncate - Not reached (this is where we do the
> physical truncate)
>
>  Now the crash-recovery starts
>
>          Delete-log-replay (above step-3) reads page with empty LP 1
> and the delete fails with PANIC (old page on disk with no insert)
>
> Doing recovery, truncate is even not reached, a WAL replay of the
> truncation will happen in the future but the recovery fails (repeatedly)
> even before reaching that point.

Hmm. I think simply reversing the order of DropRelFileNodeBuffers() and
truncating the file would open a different issue:

1) Page on disk has empty LP 1, Insert into page LP 1
2) checkpoint START (Recovery REDO eventually starts here)
3) Delete all rows on the page (page is empty now)
4) Autovacuum kicks in and starts truncating
5) smgrtruncate() truncates the file
6) checkpoint writes out buffers for pages that were just truncated
away, expanding the file again.

Your patch had a mechanism to mark the buffers as io-in-progress before
truncating the file to fix that, but I'm wary of that approach. Firstly,
it requires scanning the buffers that are dropped twice, which can take
a long time. I remember that people have already complained that
DropRelFileNodeBuffers() is slow, when it has to scan all the buffers
once. More importantly, abusing the BM_IO_INPROGRESS flag for this seems
bad. For starters, because you're not holding buffer's I/O lock, I
believe the checkpointer would busy-wait on the buffers until the
truncation has completed. See StartBufferIO() and AbortBufferIO().

Perhaps a better approach would be to prevent the checkpoint from
completing, until all in-progress truncations have completed. We have a
mechanism to wait out in-progress commits at the beginning of a
checkpoint, right after the redo point has been established. See
comments around the GetVirtualXIDsDelayingChkpt() function call in
CreateCheckPoint(). We could have a similar mechanism to wait out the
truncations before *completing* a checkpoint.

- Heikki