Re: Patch to implement pg_current_logfile() function - Mailing list pgsql-hackers

On Wed, 6 Apr 2016 23:37:09 -0500
"Karl O. Pinc" <kop@meme.com> wrote:

> On Wed, 6 Apr 2016 22:26:13 -0500
> "Karl O. Pinc" <kop@meme.com> wrote:
> > On Wed, 23 Mar 2016 23:22:26 +0100
> > Gilles Darold <gilles.darold@dalibo.com> wrote:
> >   
> > > Thanks for the reminder, here is the v3 of the patch after a
> > > deeper review and testing. It is now registered to the next
> > > commit fest under the System Administration topic.   

> Ok.  I've built it (but not tested it).  Some comments.

The logic in src/backend/postmaster/syslogger.c
looks good to me.  (The log file is created before you
create the pg_curr_log file, so the worst thing to happen
is that the user gets the old log file, never a non-existent
file.)

In two places in src/backend/postmaster/syslogger.c,
before you call store_current_log_filename()
you have a comment that's more than 80 characters
long.  Please fix.  (But see below for more.)
(http://www.postgresql.org/docs/devel/static/source-format.html)

Given the name store_current_log_filename() the code
comment is not particularly useful.  You might
consider removing the comment.  You might also
consider changing store_currrent_log_filename()
to something like write_pg_log_file().  A little
shorter, and maybe more descriptive.  Maybe enough
so that you can get rid of the comment.

(Are there other functions that create similar files?
Maybe there is a naming convention you can follow?)

(I like comments, but the pg coding style uses fewer
of them than I might use.  Hence my notes above.)

Also, your patch seems to be using spaces, not tabs.
You want tabs.
See the formatting url above.  (I forget whether
the docs use spaces or tabs.  Check the code.)

Other thoughts:

You're going to have to do something for MS Windows
EOL conventions like in logfile_open() of
src/backend/postmaster/syslogger.  You can't just
use a "\n".

The initial pg_log_file file is created by the postmaster.
Subsequent work is done by a logging subprocess.
Are there any permission implications?

src/backend/postmaster/syslogger.c expects to see
fopen() fail with ENFILE and EMFILE.  What will you
do if you get these?  Can you do the same thing
that the log rotation code does and try to update
pg_log_file the next time something logs?  You'd
have to set a flag somewhere and test (in the regular
logging code) since presumably
the next time something is logged the log rotation
code (where all your code is) would no longer be
executed.  This would leave the user with a "stale"
pg_log_file, but I'm not sure what else to do.

Regards the ereport() log level for when you can't
create pg_log_file at all, WARNING seems a bit severe
since LOG is used when the log stops rotating for some
reason.  But there does not seem to be anything else that
fits so WARNING is ok with me.  (See: include/utils/elog.h)

(I'd use LOG if you have to defer the updating of
pg_log_file.  But logging at all here could be problematic.
You wouldn't want to trigger more even more logging
and some sort of tight little loop.  It might be best
_not_ to log in this case.)

Have you given any thought as to when logfile_rotate()
is called?  Since logfile_rotate() itself logs with ereport(),
it would _seem_ safe to ereport() from within your
store_current_log_filename(), called from within
logfile_rotate().  All the same, it wouldn't hurt to be
sure that calling ereport() from within your code
can't trigger another invocation of logfile_rotate()
leading to recursive awfulness.

The indentation of the ereport(), in the part that
continues over multiple lines, in store_current_log_filename()
seems too much.  I think the continued lines should line up with
the "W" in WARNING.

This is what I see at the moment.  I'll wait for replies
before looking further and writing more.

Regards,

Karl <kop@meme.com>
Free Software:  "You don't pay back, you pay forward."                -- Robert A. Heinlein