On Mon, Dec 26, 2022 at 12:59 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> I have done a review of that, and here are my notes:
> - The variable names were a bit inconsistent, so I have switched most
> of the new code to use "fullpage".
>
> - The new test has been renamed.
>
> - RestoreBlockImage() would report a failure and the code would just
> skip it and continue its work. This could point out to a compression
> failure for example, so like any code paths calling this routine I
> think that we'd better do a pg_fatal() and fail hard.
>
> - XLogRecordHasFPW() could be checked directly in the function saving
> the blocks. Still, there is no need for it as we apply the same
> checks again in the inner loop of the routine.
>
> - Few tweaks to the docs, the --help output, the comments and the
> tests.
> - Indentation applied.
>
> - I did not understand why there is a reason to make this option
> conditional on the record prints or even the stats, so I have moved
> the FPW save routine into a separate code path. The other two could
> be silenced (or not) using --quiet for example, for the same result as
> v12 without impacting the usability of this feature.
Looks good.
> - The code was not able to handle the case of a target directory
> existing but empty, so I have added a wrapper on pg_check_dir().
Looks okay and with the following, we impose the user-provided target
directory must be empty.
+ case 4:
+ /* Exists and not empty */
+ pg_fatal("directory \"%s\" exists but is not empty", path);
> Being able to filter the blocks saved using start/end LSNs or just
> --relation is really cool, especially as the file names use the same
> order as what's needed for this option.
>
> Comments?
+1. I think this feature will also be useful in pg_walinspect.
However, I'm a bit concerned that it can flood the running database
disk - if someone generates a lot of FPI files.
Overall, the v13 patch LGTM.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
