On Mon, Dec 26, 2022 at 12:59 PM Michael Paquier <mich...@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