> On Dec 26, 2022, at 1:29 AM, Michael Paquier <mich...@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
