Re: pg_rewind and log messages

On 04/07/2015 05:59 AM, Michael Paquier wrote:
> On Mon, Apr 6, 2015 at 9:10 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> I eliminated a bunch of newlines in the log messages that seemed
>>> really unnecessary to me, simplifying a bit the whole.

So the patch removed the newlines from the error messages, and added the 
newline into pg_fatal/log instead. Perhaps that's good idea, but it's 
not actually consistent with what we do in other utilities in src/bin. 
See write_msg() and exit_horribly() in pg_dump, write_stderr() in 
pg_ctl, and psql_error() in psql. If we want to change that in pg_rewind 
(IMHO we shouldn't), let's do that as a completely separate patch.

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

Committed this one.

> - 0002 includes the improvements for logging, addressing the comments above.

This is a bit of a mixed bag. I'll comment on the three points from the 
commit message separately:

> Fix inconsistent handling of logs in pg_rewind
>
> pg_rewind was handling a couple of things differently compared to the
> other src/bin utilities:
> - Logging output needs to be flushed on stderr, not stdout

Agreed in general. But there's also precedent in printing some stuff to 
stdout: pg_ctl does that for the status message, like "server starting". 
As does initdb.

I'm pretty unclear on what the rule here is.

> - Logging messages should be prefixed with the application name

We tend to do that for error messages, but not for other messages. Seems 
inappropriate for the progress messages.

> - pg_fatal exits with error code of 1, but it was sometimes followed by
> subsequent logs, making this information lost in the process.

Fixed.

- Heikki