Обсуждение: 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

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

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

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