Re: [Patch] pg_rewind: options to use restore_command fromrecovery.conf or command line - Mailing list pgsql-hackers

On 2020-02-28 09:43, Michael Paquier wrote:
> On Thu, Feb 27, 2020 at 06:29:34PM +0300, Alexey Kondratov wrote:
>> On 2020-02-27 16:41, Alexey Kondratov wrote:
>> >
>> > New version of the patch is attached. Thanks again for your review.
>> >
>> 
>> Last patch (v18) got a conflict with one of today commits 
>> (05d8449e73).
>> Rebased version is attached.
> 
> The shape of the patch is getting better.  I have found some issues
> when reading through the patch, but nothing huge.
> 
> +   printf(_("  -c, --restore-target-wal       use restore_command in
> target config\n"));
> +   printf(_("                                 to retrieve WAL files
> from archive\n"));
> [...]
>         {"progress", no_argument, NULL, 'P'},
> +       {"restore-target-wal", no_argument, NULL, 'c'},
> It may be better to reorder that alphabetically.
> 

Sure, I put it in order. However, the recent -R option is out of order 
too.

> 
> +   if (rc != 0)
> +       /* Sanity check, should never happen. */
> +       elog(ERROR, "failed to build restore_command due to missing
> parameters");
> No point in having this comment IMO.
> 

I would prefer to keep it, since there are plenty of similar comments 
near Asserts and elogs all over the Postgres. Otherwise it may look like 
a valid error state. It may be obvious now, but for someone who is not 
aware of BuildRestoreCommand refactoring it may be not. So from my 
perspective there is nothing bad in this extra one line comment.

> 
> +/* logging support */
> +#define pg_fatal(...) do { pg_log_fatal(__VA_ARGS__); exit(1); } 
> while(0)
> Actually, I don't think that this is a good idea to name this
> pg_fatal() as we have the same think in pg_rewind so it could be
> confusing.
> 

I have added explicit exit(1) calls, since pg_fatal was used only twice 
in the archive.c. Probably, pg_log_fatal from common/logging should obey 
the same logic as FATAL log-level in the backend and do exit the 
process, but for now including pg_rewind.h inside archive.c or vice 
versa does not look like a solution.

> 
> -   while ((c = getopt_long(argc, argv, "D:nNPR", long_options,
>     &option_index)) != -1)
> +   while ((c = getopt_long(argc, argv, "D:nNPRc", long_options,
>     &option_index)) != -1)
> Alphabetical order here.
> 

Done.

> 
> +       rmdir($node_master->archive_dir);
> rmtree() is used in all our other tests.
> 

Done. There was an unobvious logic that rmdir only deletes empty 
directories, which is true in the case of archive_dir in that test, but 
I have unified it for consistency.

> 
> +               pg_log_error("archive file \"%s\" has wrong size: %lu
> instead of %lu, %s",
> +                            xlogfname, (unsigned long) 
> stat_buf.st_size,
> +                            (unsigned long) expectedSize, 
> strerror(errno));
> I think that the error message should be reworded: "unexpected WAL
> file size for \"%s\": %lu instead of %lu".  Please note that there is
> no need for strerror() here at all, as errno should be 0.
> 
> +    if (xlogfd < 0)
> +        pg_log_error("could not open file \"%s\" restored from 
> archive: %s\n",
> +                     xlogpath, strerror(errno));
> [...]
> +    pg_log_error("could not stat file \"%s\" restored from archive: 
> %s",
> +                        xlogpath, strerror(errno));
> No need for strerror() as you can just use %m.  And no need for the
> extra newline at the end as pg_log_* routines do that by themselves.
> 
> +   pg_log_error("could not restore file \"%s\" from archive\n",
> +                xlogfname);
> No need for a newline here.
> 

Thanks, I have cleaned up these log statements.


--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
The Russian Postgres Company