Thread: PITR Error Message assistance

PITR Error Message assistance

From
Simon Riggs
Date:
Could I ask for feedback on the error messages used with the archiver
and restore functionality? Possibly those of you with a hand in the
Error Message style guide?

The messages need the eye of some administrators to suggest some better
phrases. Some probably need some explanation, but I'll leave that for
you to decide which...and what you think it should say instead. Thinking
that if they need explanation, they probably are worded wrongly...

These are copied straight from recent patch:

+ elog(WARNING, "could not set notify for archiver to read log file %u,
segment %u",


+   ereport(LOG,
+   (errmsg("recovery.conf found...starting archive recovery")));

+  elog(LOG, "redo: restored \"%s\" from archive", restoreXlog);

+                 elog(LOG, "rename failed %s %s",recoveryXlog, lastrecoXlog);

+  elog(LOG, "redo: cannot restore \"%s\" from archive", restoreXlog);


+  ereport(ERROR,
+                         (errcode_for_file_access(),
+                          errmsg("could not write archive_status file \"%s\"
",
+                               tmppath)));

+  elog(LOG, "redo: moving last restored xlog to %s", tmppath);

+  elog(LOG, "redo: rename failed");

+  elog(LOG, "redo: archive chain ends; using local copy of \"%s\"",
restoreXlog);


+   (errmsg("archive recovery complete")));


+   ereport(LOG,
! (errmsg("too many transaction log files, removing \"%s\"",
xlde->d_name)));

+                 ereport(WARNING,
+                 (errcode_for_file_access(),
+                  errmsg("chkpt: cannot find archive_status file: %s ",
+                         rlogpath)));


+                  elog(WARNING, "chkpt: archiving not yet started for log file
%s",
+                         xlog);



DEBUG MESSAGES

+         elog(LOG, "backend: written %s", rlogpath );
+

+   elog(LOG, "postmaster: WAKEN_ARCHIVER received, sending SIGUSR1 to
archiver");

+             elog(LOG, "chkpt: archiving done for log file %s",
+                            xlog);

+             elog(LOG, "redo: system(%s)", xlogRestoreCmd);


Thanks,

Best regards, Simon Riggs


Re: PITR Error Message assistance

From
Peter Eisentraut
Date:
Simon Riggs wrote:
> + elog(WARNING, "could not set notify for archiver to read log file
> %u, segment %u",

Reason? (disk full, network down, leap year?)

I think elog() calls don't get translated.  You should always use
ereport.  Tom would know more about the distinction.

> +   ereport(LOG,
> +   (errmsg("recovery.conf found...starting archive recovery")));

comma

> +  elog(LOG, "redo: cannot restore \"%s\" from archive",
> restoreXlog);

Reason?

> +  ereport(ERROR,
> +                         (errcode_for_file_access(),
> +                          errmsg("could not write archive_status file
> \"%s\" ",
> +                               tmppath)));

Reason?

> +  elog(LOG, "redo: moving last restored xlog to %s", tmppath);

Probably a user doesn't know what "xlog" is.  Also, I don't like the
prefix: style you use.  Just tell what happened.

> +  elog(LOG, "redo: rename failed");

We don't write "foo failed", but "could not do foo".  And again: reason?

> +  elog(LOG, "redo: archive chain ends; using local copy of \"%s\"",
> restoreXlog);

> +   ereport(LOG,
> ! (errmsg("too many transaction log files, removing \"%s\"",
> xlde->d_name)));

How/where is the limit defined?

> +                 ereport(WARNING,
> +                 (errcode_for_file_access(),
> +                  errmsg("chkpt: cannot find archive_status file: %s ",
> +                         rlogpath)));

There is enough space that you can write "checkpoint".  Or actually
don't write anything.  What is the archive_status file?

> +                  elog(WARNING, "chkpt: archiving not yet started for log
> file %s",
> +                         xlog);

What does that tell me?


> DEBUG MESSAGES

Debug messages should have a DEBUG severity.

>
> +         elog(LOG, "backend: written %s", rlogpath );
> +
>
> +   elog(LOG, "postmaster: WAKEN_ARCHIVER received, sending SIGUSR1
> to archiver");
>
> +             elog(LOG, "chkpt: archiving done for log file %s",
> +                            xlog);
>
> +             elog(LOG, "redo: system(%s)", xlogRestoreCmd);


Re: PITR Error Message assistance

From
Tom Lane
Date:
Peter Eisentraut <peter_e@gmx.net> writes:
> Simon Riggs wrote:
>> + elog(WARNING, "could not set notify for archiver to read log file
>> %u, segment %u",

> Reason? (disk full, network down, leap year?)

> I think elog() calls don't get translated.  You should always use
> ereport.  Tom would know more about the distinction.

elog is deprecated except for debugging or "can't-happen" messages.
Anything user-facing ought to be reported with ereport.  In this case
elog might be okay --- it's not clear to me from this snippet whether
the condition is one a user would be likely to see.

There's plenty of detail about all this in chapter 45 of the docs:
http://www.postgresql.org/docs/7.4/static/source.html
and I think most of Peter's comments trace directly to items in the
message style guide there.

            regards, tom lane

Re: PITR Error Message assistance

From
Simon Riggs
Date:
On Wed, 2004-06-30 at 15:58, Tom Lane wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:

Thanks very much for your comments.

> > Simon Riggs wrote:
> >> + elog(WARNING, "could not set notify for archiver to read log file
> >> %u, segment %u",
>
> > Reason? (disk full, network down, leap year?)

? "could not increment year number (additional day found)" :)

>
> > I think elog() calls don't get translated.  You should always use
> > ereport.  Tom would know more about the distinction.
>
> elog is deprecated except for debugging or "can't-happen" messages.
> Anything user-facing ought to be reported with ereport.  In this case
> elog might be okay --- it's not clear to me from this snippet whether
> the condition is one a user would be likely to see.

Thanks for clarifying that, I wasn't clear on that. I'll go through and
make any required changes.

> There's plenty of detail about all this in chapter 45 of the docs:
> http://www.postgresql.org/docs/7.4/static/source.html
> and I think most of Peter's comments trace directly to items in the
> message style guide there.

Yes, I've read that and this post was all about discussing this and
applying it as part of polishing work.

I was really looking for some suggestions rather than a critique - the
messages were not exactly the bit I was focusing on in dev. Peter has
highlighted the extent of improvement, so I'll get on it now.

(For later, I've found it confusing that the Developer's FAQ makes no
mention of those notes, nor the other way around - I'll submit some
suggested changes to make everything clearer for those on their first
patch....and yes, I've tried hard to RTFM)

Can I just clarify whether the end of June freeze does or does not apply
to documentation? Clearly, I have a significant amount of documentation
to write also, which presumably will need review also.

What are the plans for review? I've not really heard much as yet...

Best Regards, Simon Riggs


Re: PITR Error Message assistance

From
Tom Lane
Date:
Simon Riggs <simon@2ndquadrant.com> writes:
> Can I just clarify whether the end of June freeze does or does not apply
> to documentation?

Absolutely not.  Usually we're still working on the docs during beta.
Note also that this is *feature* freeze not code freeze.  Bug fixes and
loose ends are not a problem for later.

> What are the plans for review?

Right at the moment I'm hip-deep in nested transactions ... I hope to
commit that today and then start looking at your stuff.

            regards, tom lane