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