Re: pg_rewind and log messages - Mailing list pgsql-hackers

On Tue, Apr 7, 2015 at 11:59 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> 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.

Thanks for the patch! I'd like to push this patch at first. But one comment is

+                 "You must run %s as the PostgreSQL superuser.\n",

Isn't the term "PostgreSQL superuser" confusing? I'm afraid that a user might
confuse "PostgreSQL superuser" with a database superuser. I see you just
borrowed that term from pg_resetxlog.c, though. BTW, initdb and pg_ctl also
have the same check and the error message is as follows. Isn't this better?
Thought?
           ("%s: cannot be run as root\n"                      "Please log in (using, e.g., \"su\") as the "
         "(unprivileged) user that will\n"                      "own the server process.\n
 

Regards,

-- 
Fujii Masao