MIME-Version: 1.0
References: <PUrgXj3toL_CVxytc7fzYQNlY5FiUllklbS9UL1wU5ptiJAC2ChrF64EwO8neCKU9QnET5Zjv1HpMLFAgQPJQxOzM9f49jjkV56gEpUOYw8=@protonmail.com>
In-Reply-To: <PUrgXj3toL_CVxytc7fzYQNlY5FiUllklbS9UL1wU5ptiJAC2ChrF64EwO8neCKU9QnET5Zjv1HpMLFAgQPJQxOzM9f49jjkV56gEpUOYw8=@protonmail.com>
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Fri, 18 Oct 2024 15:56:53 +0200
Message-ID: <CAEze2Wi30VZDKxaR0wRGNx7LJ=heiwPpALhLFTu2Lg86QanCXA@mail.gmail.com>
Subject: Re: Recovery of .partial WAL segments
To: Stefan Fercot <stefan.fercot@protonmail.com>
Cc: "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAEze2Wi30VZDKxaR0wRGNx7LJ%3DheiwPpALhLFTu2Lg86QanCXA%40mail.gmail.com>
Precedence: bulk

On Fri, 5 Apr 2024 at 11:45, Stefan Fercot <stefan.fercot@protonmail.com> w=
rote:
>
> Dear hackers,
>
> Generating a ".partial" WAL segment is pretty common nowadays (using pg_r=
eceivewal or during standby promotion).
> However, we currently don't do anything with it unless the user manually =
removes that ".partial" extension.
>
> The 028_pitr_timelines tests are highlighting that fact: with test data b=
eing being in 000000020000000000000003 and 000000010000000000000003.partial=
, a recovery following the latest timeline (2) will succeed but fail if we =
follow the current timeline (1).
>
> By simply trying to fetch the ".partial" file in XLogFileRead, we can eas=
ily recover more data and also cover that (current timeline) recovery case.
>
> So, this proposed patch makes XLogFileRead try to restore ".partial" WAL =
archives and adds a test to 028_pitr_timelines using current recovery_targe=
t_timeline.

Does this path only get hit when we don't already have any WAL
segments (or partial segments) left for that timeline? I'm a bit
worried about overwriting existing (partial) segments that may have
more WAL than what we can get from archives.

(patch v2)
> +            restoredArchivedFile =3D !RestoreArchivedFile(path, xlogfnam=
e,
> +                                                        "RECOVERYXLOG",
> +                                                        wal_segment_size=
,
> +                                                        InRedo) &&
> +                !RestoreArchivedFile(path, partialxlogfname,
>                                      "RECOVERYXLOG",
>                                      wal_segment_size,
> -                                     InRedo))
> +                                     InRedo);

The value of restoredArchiveFile is inverted with what it indicates:
It is true when we failed to restore an archived xlog segment, and
false if we did succeed.

I'm also not a fan of the additional allocation of partialxlogfname in
this code. It could well do without, by "just" reusing the xlogfname
scratch space when we fail to recover the full segment.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)