Re: pg_rewind and log messages

On Mon, Apr 6, 2015 at 5:33 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Apr 6, 2015 at 1:41 PM, Michael Paquier wrote:
>> I guess that you are working on a patch? If not, you are looking for one?
>
> Code-speaking, this gives the patch attached.

Thanks! Here are the review comments:

I'm not familiar with native language support (sorry), but don't we need to
add the shortcut of gettext into every calls of pg_log and pg_fatal, e.g.,
change pg_fatal("xxx") to pg_fatal(_("xxx"))? I know that fprintf() in
pg_Log_v() has such shortcut, but I'm not sure if that's enough or not.
        case PG_FATAL:
-            printf("\n%s", _(message));
-            printf("%s", _("Failure, exiting\n"));
+            fprintf(stderr, _("\n%s: fatal: %s\n"), progname, message);
+            fprintf(stderr, _("Failure, exiting\n"));

Why do we need to output the error level like fatal? This seems also
inconsistent with the log message policy of other tools.

Why do we need to output \n at the head of the message?

The second message "Failure, exiting" is really required?

> I eliminated a bunch of
> newlines in the log messages that seemed really unnecessary to me,
> simplifying a bit the whole. While hacking this stuff, I noticed as
> well that pg_rewind could be called as root on non-Windows platform,
> that's dangerous from a security point of view as process manipulates
> files in PGDATA. Hence let's block that. On Windows, a restricted
> token should be used.

Good catch! I think it's better to implement it as a separate patch
because it's very different from log message problem.

Regards,

-- 
Fujii Masao