Re: pg_rewind and log messages

On Mon, Apr 6, 2015 at 9:10 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> 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.

I think I addressed those things...

>          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.

initdb and psql do not for a couple of warning messages, but perhaps I
am just taking consistency with the original code too seriously.

> Why do we need to output \n at the head of the message?
> The second message "Failure, exiting" is really required?

I think that those things were here to highlight the fact that this is
a fatal bailout, but the error code would help the same way I guess...

>> 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.

Attached are new patches:
- 0001 fixes the restriction issues: restricted token on Windows, and
forbid non-root user on non-Windows.
- 0002 includes the improvements for logging, addressing the comments above.

Regards,
--
Michael