> On Dec 26, 2022, at 1:29 AM, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, Dec 24, 2022 at 06:23:29PM +0530, Bharath Rupireddy wrote:
>> Thanks for the patch. I've made the above change as well as renamed
>> the test file name to be save_fpi.pl, everything else remains the same
>> as v11. Here's the v12 patch which LGTM. I'll mark it as RfC -
>> https://commitfest.postgresql.org/41/3628/.
>
> 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 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().
> - 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.
> - 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.
> - 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.
> - Few tweaks to the docs, the --help output, the comments and the
> tests.
> - Indentation applied.
>
> 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.
Sounds good, definitely along the ideas of what I’d originally envisioned.
Thanks,
David
