Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

On 2020-01-24 08:50, Michael Paquier wrote:
> On Wed, Jan 22, 2020 at 12:55:30AM +0300, Alexander Korotkov wrote:
>> On Sun, Jan 19, 2020 at 1:24 PM Michael Paquier <michael@paquier.xyz> 
>> wrote:
>>> +static int
>>> +RestoreArchivedWALFile(const char *path, const char *xlogfname,
>>> +                      off_t expectedSize, const char 
>>> *restoreCommand)
>>> Not a fan of putting that to pg_rewind/parsexlog.c.  It has nothing 
>>> to
>>> do with WAL parsing, and it seems to me that we could have an 
>>> argument
>>> for making this available as a frontend-only API in src/common/.
>> 
>> I've put it into src/common/fe_archive.c.
> 
> This split makes sense.  You have forgotten to update
> @pgcommonfrontendfiles in Mkvcbuild.pm for the MSVC build.  Still, I
> can see that we have a lot of duplication between the code of the
> frontend and the backend, which is annoying..  The execution part is
> tricky to split because the backend has pre- and post- callbacks, the
> interruption handling is not the same and there is the problem of
> elog() calls, with elevel that can be changed depending on the backend
> configuration.  However, I can see one portion of the code which is
> fully duplicated and could be refactored out: the construction of the
> command to execute, by having in input the restore_command string and
> the arguments that we expect to replace in the '%' markers which are
> part of the command.
> 

OK, I agree that duplication of this part directly is annoying. I did a 
refactoring and moved this restore_command building code into 
common/archive. Separate file was needed, since fe_archive is 
frontend-only, so I cannot put universal code there. I do not know is 
there any better place for it.

> 
> If NULL is specified for one of those values,
> then the construction routine returns NULL, synonym of failure.  And
> the result of the routine is the built command.  I think that you
> would need an extra argument to give space for the error message
> generated in the event of an error when building the command.
> 

I did it in a bit different way, but the overall logic is exactly as you 
proposed, I hope.
Also I have checked win/linux build in the similar to cfbot CI setup and 
now everything runs well.

> 
> +++ b/src/include/common/fe_archive.h
> +#ifndef ARCHIVE_H
> +#define ARCHIVE_H
> This should be FE_ARCHIVE_H.
> 

Changed.

> 
> -   while ((c = getopt_long(argc, argv, "D:nNPR", long_options,
>     &option_index)) != -1)
> +   while ((c = getopt_long(argc, argv, "D:nNPRC:c", long_options,
>     &option_index)) != -1)
> This still includes 'C', but that should not be the case.
> 
> +   <application>pg_rewind</application> with the
> <literal>-c</literal> or
> +   <literal>-C</literal> option to automatically retrieve them from
> the WAL
> [...]
> +      <term><option>-C <replaceable
> class="parameter">restore_command</replaceable></option></term>
> +      <term><option>--target-restore-command=<replaceable
> class="parameter">restore_command</replaceable></option></term>
> [...]
> +      available, try re-running <application>pg_rewind</application> 
> with
> +      the <option>-c</option> or <option>-C</option> option to search
> +      for the missing files in the WAL archive.
> Three places in the docs need to be cleaned up.
> 

I have fixed all the above, thanks.

> 
> Do we really need to test the new "archive" mode as well for
> 003_extrafiles and 002_databases?  I would imagine that only 001_basic
> is enough.
> 

We do not check any unique code paths with it, so I do it only for 
001_basic now. I have left it as a test mode, since it will be easy to 
turn this on for any other test in the future if needed and it fits well 
RewindTest.pm module.

> 
> +# Moves WAL files to the temporary location and returns 
> restore_command
> +# to get them back.
> +sub move_wal
> +{
> +   my ($tmp_dir, $master_pgdata) = @_;
> +   my $wal_archive_path = "$tmp_dir/master_wal_archive";
> +   my $wal_path = "$master_pgdata/pg_wal";
> +   my $wal_dir;
> +   my $restore_command;
> I think that this routine is wrongly designed.  First, in order to
> copy the contents from one path to another, we have
> RecursiveCopy::copy_path, and there is a long history about making
> it safe for the TAP tests.  Second, there is in
> PostgresNode::enable_restoring already a code path doing the
> decision-making on how building restore_command.  We should keep this
> code path unique.
> 

Yeah, thank you for pointing me to the RecursiveCopy::copypath and 
especially PostgresNode::enable_restoring, I have modified test to use 
them instead. The copypath does not work with existing destination 
directories and does not preserve initial permissions, so I had to do 
some dirty work to achieve what we need in the test and keep it simple 
in the same time. However, I think that using these unified routines is 
much better for a future support.

New version is attached. Do you have any other comments or objections?


Regards
--
Alexey