Re: [HACKERS] pg_rewind and log messages
On Tue, Apr 7, 2015 at 11:59 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Mon, Apr 6, 2015 at 9:10 PM, Fujii Masao masao.fu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind and log messages
On Tue, Apr 7, 2015 at 4:33 PM, Fujii Masao wrote: 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 I'm fine with that as well. Why not updating the message of pg_resetxlog as well then? It would be good to be consistent. I guess that the call to set_pglocale_pgservice() should be added as well in the first patch (see last version upthread), this has nothing to do with the logging issues. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind and log messages
On Tue, Apr 7, 2015 at 4:16 PM, Fujii Masao masao.fu...@gmail.com wrote: On Mon, Apr 6, 2015 at 10:01 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Fujii Masao wrote: On Mon, Apr 6, 2015 at 5:33 PM, Michael Paquier michael.paqu...@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. It's not necessary for pg_fatal and the like, because those functions are marked to have their first argument automatically translated in nls.mk. This means that the string literal is automatically extracted into pg_rewind.pot for translators. Of course, the function itself must call _() (or some variant thereof) to actually fetch the translated string at run time. Understood. Thanks! BTW, as far as I read pg_rewind's nls.mk correctly, it also has two problems. (1) file_ops.c should be added into GETTEXT_FILES. And ../../common/restricted_tokens.c. (2) pg_log should be pg_log:2 in GETTEXT_TRIGGERS Seems so. -- Michael From e5e08188c33adb74fc722c29e660832d88fdd765 Mon Sep 17 00:00:00 2001 From: Michael Paquier mich...@otacoo.com Date: Mon, 6 Apr 2015 17:18:21 +0900 Subject: [PATCH 1/2] Fix process handling of pg_rewind To begin with, pg_rewind should not be allowed to run as root on non-Windows platforms as it manipulates data folders, and file permissions. On Windows platforms, it can run under a user that has Administrator rights but in this case a restricted token needs to be used. Also add a call to set_pglocale_pgservice() that was missing. --- src/bin/pg_rewind/nls.mk | 2 +- src/bin/pg_rewind/pg_rewind.c | 17 + 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/bin/pg_rewind/nls.mk b/src/bin/pg_rewind/nls.mk index e43f3b9..69e87d1 100644 --- a/src/bin/pg_rewind/nls.mk +++ b/src/bin/pg_rewind/nls.mk @@ -1,7 +1,7 @@ # src/bin/pg_rewind/nls.mk CATALOG_NAME = pg_rewind AVAIL_LANGUAGES = -GETTEXT_FILES= copy_fetch.c datapagemap.c fetch.c filemap.c libpq_fetch.c logging.c parsexlog.c pg_rewind.c timeline.c ../../common/fe_memutils.c ../../../src/backend/access/transam/xlogreader.c +GETTEXT_FILES= copy_fetch.c datapagemap.c fetch.c filemap.c libpq_fetch.c logging.c parsexlog.c pg_rewind.c timeline.c ../../common/fe_memutils.c ../../common/restricted_token.c ../../../src/backend/access/transam/xlogreader.c GETTEXT_TRIGGERS = pg_log pg_fatal report_invalid_record:2 GETTEXT_FLAGS= pg_log:2:c-format \ diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c index dda3a79..04d6a46 100644 --- a/src/bin/pg_rewind/pg_rewind.c +++ b/src/bin/pg_rewind/pg_rewind.c @@ -24,6 +24,7 @@ #include access/xlog_internal.h #include catalog/catversion.h #include catalog/pg_control.h +#include common/restricted_token.h #include getopt_long.h #include storage/bufpage.h @@ -102,6 +103,7 @@ main(int argc, char **argv) TimeLineID endtli; ControlFileData ControlFile_new; + set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN(pg_rewind)); progname = get_progname(argv[0]); /* Process command-line arguments */ @@ -174,6 +176,21 @@ main(int argc, char **argv) exit(1); } + /* + * Don't allow pg_rewind to be run as root, to avoid overwriting the + * ownership of files in the data directory. We need only check for root + * -- any other user won't have sufficient permissions to modify files in + * the data directory. + */ +#ifndef WIN32 + if (geteuid() == 0) + pg_fatal(cannot be executed by \root\\n + You must run %s as the PostgreSQL superuser.\n, + progname); +#endif + + get_restricted_token(progname); + /* Connect to remote server */ if (connstr_source) libpqConnect(connstr_source); -- 2.3.5 From 9bd5eec75cceb8a12d26a9e16dabc2e23294c148 Mon Sep 17 00:00:00 2001 From: Michael Paquier mich...@otacoo.com Date: Mon, 6 Apr 2015 17:18:21 +0900 Subject: [PATCH 2/2] 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 - Logging messages should be prefixed with the application name - pg_fatal exits with error code of 1, but it was sometimes followed by subsequent logs, making this information lost in the process. --- src/bin/pg_rewind/copy_fetch.c | 24 +- src/bin/pg_rewind/datapagemap.c | 4 ++- src/bin/pg_rewind/file_ops.c| 30 +++--- src/bin/pg_rewind/filemap.c | 16 ++-- src/bin/pg_rewind/libpq_fetch.c | 52
Re: [HACKERS] pg_rewind and log messages
On Mon, Apr 6, 2015 at 10:01 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Fujii Masao wrote: On Mon, Apr 6, 2015 at 5:33 PM, Michael Paquier michael.paqu...@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. It's not necessary for pg_fatal and the like, because those functions are marked to have their first argument automatically translated in nls.mk. This means that the string literal is automatically extracted into pg_rewind.pot for translators. Of course, the function itself must call _() (or some variant thereof) to actually fetch the translated string at run time. Understood. Thanks! BTW, as far as I read pg_rewind's nls.mk correctly, it also has two problems. (1) file_ops.c should be added into GETTEXT_FILES. (2) pg_log should be pg_log:2 in GETTEXT_TRIGGERS Another thing is compound messages like foo has happened\nSee --help for usage.\n and bar didn't happen\.See --help for usage. In those cases, the see --help part would need to be translated over and over, so it's best to separate them in phrases to avoid repetitive work for translators. Not sure how to do this -- maybe something like _(foo has happened\n) _(See --help) but I'm not sure how to appease the compiler. Having them in two separate pg_log() calls (or whatever) was handy for this reason. Yep. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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.fu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind and log messages
Heikki Linnakangas wrote: On 04/07/2015 05:59 AM, Michael Paquier wrote: 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. One principle that sometimes helps is to consider what happens if you use the command as part of a larger pipeline; progress messages can be read by some other command further down (and perhaps report them in a dialog box, if you embed the program in a GUI, say), but error messages should probably be processed differently; normally the pipeline would be aborted as a whole. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind and log messages
On Wed, Apr 8, 2015 at 5:53 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Heikki Linnakangas wrote: On 04/07/2015 05:59 AM, Michael Paquier wrote: 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. One principle that sometimes helps is to consider what happens if you use the command as part of a larger pipeline; progress messages can be read by some other command further down (and perhaps report them in a dialog box, if you embed the program in a GUI, say), but error messages should probably be processed differently; normally the pipeline would be aborted as a whole. Make sense. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind and log messages
On Mon, Apr 6, 2015 at 10:01 PM, Alvaro Herrera wrote: I'm not sure about translation of generic strings such as %s: %s. My first impression is that they shouldn't be translated, but maybe it is important that they are for languages I don't know nothing about such as Japanese. I misunderstood things here, pg_fatal and others should not use _() directly for generic strings. It seems like an overkill. Another thing is compound messages like foo has happened\nSee --help for usage.\n and bar didn't happen\.See --help for usage. In those cases, the see --help part would need to be translated over and over, so it's best to separate them in phrases to avoid repetitive work for translators. Not sure how to do this -- maybe something like _(foo has happened\n) _(See --help) but I'm not sure how to appease the compiler. Having them in two separate pg_log() calls (or whatever) was handy for this reason. OK, let's switch to two calls to pg_log or similar in those cases to make the life of translators easier. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind and log messages
On Mon, Apr 6, 2015 at 9:10 PM, Fujii Masao masao.fu...@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 From 6fa9c9d427352a01d589ce1871b6adecd88cf49c Mon Sep 17 00:00:00 2001 From: Michael Paquier mich...@otacoo.com Date: Tue, 7 Apr 2015 11:21:17 +0900 Subject: [PATCH 1/2] Fix process handling of pg_rewind To begin with, pg_rewind should not be allowed to run as root on non-Windows platforms as it manipulates data folders, and file permissions. On Windows platforms, it can run under a user that has Administrator rights but in this case a restricted token needs to be used. --- src/bin/pg_rewind/pg_rewind.c | 16 1 file changed, 16 insertions(+) diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c index dda3a79..200e001 100644 --- a/src/bin/pg_rewind/pg_rewind.c +++ b/src/bin/pg_rewind/pg_rewind.c @@ -24,6 +24,7 @@ #include access/xlog_internal.h #include catalog/catversion.h #include catalog/pg_control.h +#include common/restricted_token.h #include getopt_long.h #include storage/bufpage.h @@ -174,6 +175,21 @@ main(int argc, char **argv) exit(1); } + /* + * Don't allow pg_rewind to be run as root, to avoid overwriting the + * ownership of files in the data directory. We need only check for root + * -- any other user won't have sufficient permissions to modify files in + * the data directory. + */ +#ifndef WIN32 + if (geteuid() == 0) + pg_fatal(cannot be executed by \root\\n + You must run %s as the PostgreSQL superuser.\n, + progname); +#endif + + get_restricted_token(progname); + /* Connect to remote server */ if (connstr_source) libpqConnect(connstr_source); -- 2.3.5 From 79103f93393e34e1a795001afd3f43a3a8201500 Mon Sep 17 00:00:00 2001 From: Michael Paquier mich...@otacoo.com Date: Mon, 6 Apr 2015 17:18:21 +0900 Subject: [PATCH 2/2] 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 - Logging messages should be prefixed with the application name - pg_fatal exits with error code of 1, but it was sometimes followed by subsequent logs, making this information lost in the process. --- src/bin/pg_rewind/copy_fetch.c | 24 - src/bin/pg_rewind/datapagemap.c | 4 ++- src/bin/pg_rewind/file_ops.c| 30 +++--- src/bin/pg_rewind/filemap.c | 16 ++-- src/bin/pg_rewind/libpq_fetch.c | 52 ++--- src/bin/pg_rewind/logging.c | 13 +- src/bin/pg_rewind/logging.h | 2 +- src/bin/pg_rewind/parsexlog.c | 20 +++ src/bin/pg_rewind/pg_rewind.c | 57 ++--- src/bin/pg_rewind/pg_rewind.h | 2 ++ src/bin/pg_rewind/timeline.c| 27 +++ 11 files changed, 122 insertions(+), 125 deletions(-) diff --git a/src/bin/pg_rewind/copy_fetch.c b/src/bin/pg_rewind/copy_fetch.c index 887fec9..d3430e5 100644 --- a/src/bin/pg_rewind/copy_fetch.c +++ b/src/bin/pg_rewind/copy_fetch.c @@ -58,7 +58,7 @@ recurse_dir(const char *datadir, const char *parentpath, xldir =
Re: [HACKERS] pg_rewind and log messages
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. 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.
Regards,
--
Michael
From 32b0aeef2602f7388c7e9fca6290046d8d9dfe67 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Mon, 6 Apr 2015 17:18:21 +0900
Subject: [PATCH] Fix inconsistent error and process handling in pg_rewind
pg_rewind was handling a couple of things differently compared to the
other src/bin utilities:
- pg_rewind manipulates files in a data folder, hence it should not run
as root on Non-Windows platforms. On Windows, it should use a restricted
token
- Logging output needs to be flushed on stderr, not stdout
- Logging messages should be prefixed with the application name
- pg_fatal exits with error code of 1, but it was sometimes followed by
subsequent logs, making this information lost in the process.
---
src/bin/pg_rewind/copy_fetch.c | 22 +--
src/bin/pg_rewind/datapagemap.c | 4 +-
src/bin/pg_rewind/file_ops.c| 30 +++---
src/bin/pg_rewind/filemap.c | 18 -
src/bin/pg_rewind/libpq_fetch.c | 52 -
src/bin/pg_rewind/logging.c | 16 +---
src/bin/pg_rewind/logging.h | 1 +
src/bin/pg_rewind/parsexlog.c | 20 +-
src/bin/pg_rewind/pg_rewind.c | 86 +++--
src/bin/pg_rewind/pg_rewind.h | 2 +
src/bin/pg_rewind/timeline.c| 27 +
11 files changed, 144 insertions(+), 134 deletions(-)
diff --git a/src/bin/pg_rewind/copy_fetch.c b/src/bin/pg_rewind/copy_fetch.c
index 887fec9..de5fe4a 100644
--- a/src/bin/pg_rewind/copy_fetch.c
+++ b/src/bin/pg_rewind/copy_fetch.c
@@ -58,7 +58,7 @@ recurse_dir(const char *datadir, const char *parentpath,
xldir = opendir(fullparentpath);
if (xldir == NULL)
- pg_fatal(could not open directory \%s\: %s\n,
+ pg_fatal(could not open directory \%s\: %s,
fullparentpath, strerror(errno));
while (errno = 0, (xlde = readdir(xldir)) != NULL)
@@ -113,13 +113,13 @@ recurse_dir(const char *datadir, const char *parentpath,
len = readlink(fullpath, link_target, sizeof(link_target) - 1);
if (len == -1)
-pg_fatal(readlink() failed on \%s\: %s\n,
+pg_fatal(readlink() failed on \%s\: %s,
fullpath, strerror(errno));
if (len == sizeof(link_target) - 1)
{
/* path was truncated */
-pg_fatal(symbolic link \%s\ target path too long\n,
+pg_fatal(symbolic link \%s\ target path too long,
fullpath);
}
@@ -132,18 +132,18 @@ recurse_dir(const char *datadir, const char *parentpath,
if (strcmp(parentpath, pg_tblspc) == 0)
recurse_dir(datadir, path, callback);
#else
- pg_fatal(\%s\ is a symbolic link, but symbolic links are not supported on this platform\n,
+ pg_fatal(\%s\ is a symbolic link, but symbolic links are not supported on this platform,
fullpath);
#endif /* HAVE_READLINK */
}
}
if (errno)
- pg_fatal(could not read directory \%s\: %s\n,
+ pg_fatal(could not read directory \%s\: %s,
fullparentpath, strerror(errno));
if (closedir(xldir))
- pg_fatal(could not close archive location \%s\: %s\n,
+ pg_fatal(could not close archive location \%s\: %s,
fullparentpath, strerror(errno));
}
@@ -163,11 +163,11 @@ copy_file_range(const char *path, off_t begin, off_t end, bool trunc)
srcfd = open(srcpath, O_RDONLY | PG_BINARY, 0);
if (srcfd 0)
- pg_fatal(could not open source file \%s\: %s\n,
+ pg_fatal(could not open source file \%s\: %s,
srcpath, strerror(errno));
if (lseek(srcfd, begin, SEEK_SET) == -1)
- pg_fatal(could not seek in source file: %s\n, strerror(errno));
+ pg_fatal(could not seek in source file: %s, strerror(errno));
open_target_file(path, trunc);
@@ -184,17 +184,17 @@ copy_file_range(const char *path, off_t begin, off_t end, bool trunc)
readlen = read(srcfd, buf, len);
if (readlen 0)
- pg_fatal(could not read file \%s\: %s\n,
+ pg_fatal(could not read file \%s\: %s,
srcpath, strerror(errno));
else if (readlen == 0)
- pg_fatal(unexpected EOF while reading file \%s\\n, srcpath);
+ pg_fatal(unexpected EOF while reading file \%s\, srcpath);
write_target_range(buf, begin, readlen);
begin += readlen;
}
if (close(srcfd) != 0)
- pg_fatal(error closing file \%s\: %s\n, srcpath, strerror(errno));
+ pg_fatal(error closing file \%s\: %s, srcpath, strerror(errno));
}
/*
diff --git a/src/bin/pg_rewind/datapagemap.c b/src/bin/pg_rewind/datapagemap.c
index 3477366..34868f9 100644
---
Re: [HACKERS] pg_rewind and log messages
Fujii Masao wrote: On Mon, Apr 6, 2015 at 5:33 PM, Michael Paquier michael.paqu...@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. It's not necessary for pg_fatal and the like, because those functions are marked to have their first argument automatically translated in nls.mk. This means that the string literal is automatically extracted into pg_rewind.pot for translators. Of course, the function itself must call _() (or some variant thereof) to actually fetch the translated string at run time. I'm not sure about translation of generic strings such as %s: %s. My first impression is that they shouldn't be translated, but maybe it is important that they are for languages I don't know nothing about such as Japanese. Another thing is compound messages like foo has happened\nSee --help for usage.\n and bar didn't happen\.See --help for usage. In those cases, the see --help part would need to be translated over and over, so it's best to separate them in phrases to avoid repetitive work for translators. Not sure how to do this -- maybe something like _(foo has happened\n) _(See --help) but I'm not sure how to appease the compiler. Having them in two separate pg_log() calls (or whatever) was handy for this reason. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind and log messages
On Mon, Apr 6, 2015 at 5:33 PM, Michael Paquier michael.paqu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_rewind and log messages
On Mon, Apr 6, 2015 at 12:57 PM, Fujii Masao masao.fu...@gmail.com wrote:
(1)
It outputs an error message to stdout not stderr.
(2)
The tool name should be added at the head of log message as follows,
but not in pg_rewind.
pg_basebackup: no target directory specified
Agreed. That's inconsistent.
(3)
if (datadir_source == NULL connstr_source == NULL)
{
pg_fatal(no source specified (--source-pgdata or
--source-server)\n);
pg_fatal(Try \%s --help\ for more information.\n, progname);
exit(1);
Since the first call of pg_fatal exits with 1, the subsequent pg_fatal and
exit
will never be called.
True.
(4)
ISTM that set_pglocale_pgservice() needs to be called, but not in pg_rewind.
True. I think that we should consider a call to get_restricted_token()
as well now that I look at it.
(5)
printf() is used to output an error in some files, e.g., timeline.c and
parsexlog.c. These printf() should be replaced with pg_log or pg_fatal?
On top of that, datapagemap.c has a debug message as well using
printf, as well as filemap.c in print_filemap().
I guess that you are working on a patch? If not, you are looking for one?
Regards,
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
