shouldn't we log permission errors when accessing the configured trigger file?
Hi,
For some reason CheckForStandbyTrigger() doesn't report permission
errors when stat()int the trigger file. Shouldn't we fix that?
static bool
CheckForStandbyTrigger(void)
{
...
if (stat(TriggerFile, &stat_buf) == 0)
{
ereport(LOG,
(errmsg("trigger file found: %s", TriggerFile)));
unlink(TriggerFile);
triggered = true;
fast_promote = true;
return true;
}
Imo the stat() should warn about all errors but ENOENT?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On Sun, Jan 26, 2014 at 1:03 PM, Andres Freund <andres@2ndquadrant.com> wrote:
For some reason CheckForStandbyTrigger() doesn't report permission
errors when stat()int the trigger file. Shouldn't we fix that?static bool
CheckForStandbyTrigger(void)
{
...
if (stat(TriggerFile, &stat_buf) == 0)
{
ereport(LOG,
(errmsg("trigger file found: %s", TriggerFile)));
unlink(TriggerFile);
triggered = true;
fast_promote = true;
return true;
}Imo the stat() should warn about all errors but ENOENT?
Seems reasonable. It could lead to quite a bit of log spam, I
suppose, but the way things are now could be pretty mystifying if
you've located your trigger file somewhere outside $PGDATA, and a
parent directory is lacking permissions.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jan 27, 2014 at 3:43 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Jan 26, 2014 at 1:03 PM, Andres Freund <andres@2ndquadrant.com>
wrote:For some reason CheckForStandbyTrigger() doesn't report permission
errors when stat()int the trigger file. Shouldn't we fix that?static bool
CheckForStandbyTrigger(void)
{
...
if (stat(TriggerFile, &stat_buf) == 0)
{
ereport(LOG,
(errmsg("trigger file found: %s",TriggerFile)));
unlink(TriggerFile);
triggered = true;
fast_promote = true;
return true;
}Imo the stat() should warn about all errors but ENOENT?
Seems reasonable. It could lead to quite a bit of log spam, I
suppose, but the way things are now could be pretty mystifying if
you've located your trigger file somewhere outside $PGDATA, and a
parent directory is lacking permissions.
+1. Since it actually indicates something that's quite broken (since with
that you can never make the trigger work until you fix it), the log spam
seems like it would be appropriate. (Logspam is never nice, but a single
log line is also very easy to miss - this should log enough that you
wouldn't)
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Mon, Jan 27, 2014 at 03:45:38PM +0100, Magnus Hagander wrote:
On Mon, Jan 27, 2014 at 3:43 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Jan 26, 2014 at 1:03 PM, Andres Freund <andres@2ndquadrant.com>
wrote:For some reason CheckForStandbyTrigger() doesn't report permission
errors when stat()int the trigger file. Shouldn't we fix that?static bool
CheckForStandbyTrigger(void)
{
...
if (stat(TriggerFile, &stat_buf) == 0)
{
ereport(LOG,
(errmsg("trigger file found: %s",TriggerFile)));
unlink(TriggerFile);
triggered = true;
fast_promote = true;
return true;
}Imo the stat() should warn about all errors but ENOENT?
Seems reasonable. It could lead to quite a bit of log spam, I
suppose, but the way things are now could be pretty mystifying if
you've located your trigger file somewhere outside $PGDATA, and a
parent directory is lacking permissions.+1. Since it actually indicates something that's quite broken (since with that
you can never make the trigger work until you fix it), the log spam seems like
it would be appropriate. (Logspam is never nice, but a single log line is also
very easy to miss - this should log enough that you wouldn't)
I have developed the attached patch to address this issue.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Attachments:
trigger.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
new file mode 100644
index 0106cdf..88ad51f
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
*************** CheckForStandbyTrigger(void)
*** 11102,11107 ****
--- 11102,11113 ----
fast_promote = true;
return true;
}
+ else if (errno != ENOENT)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not stat trigger file \"%s\": %m",
+ TriggerFile)));
+
return false;
}
On Wed, Apr 16, 2014 at 06:55:14PM -0400, Bruce Momjian wrote:
Seems reasonable. It could lead to quite a bit of log spam, I
suppose, but the way things are now could be pretty mystifying if
you've located your trigger file somewhere outside $PGDATA, and a
parent directory is lacking permissions.+1. Since it actually indicates something that's quite broken (since with that
you can never make the trigger work until you fix it), the log spam seems like
it would be appropriate. (Logspam is never nice, but a single log line is also
very easy to miss - this should log enough that you wouldn't)I have developed the attached patch to address this issue.
Patch applied.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers