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

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