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.
Comments?
--
Michael
