Re: logging enhancements, minor code cleanup

On Sat, Aug 09, 2003 at 07:22:56PM -0400, Andrew Dunstan wrote:
> The attached patch does 3 things:
>
> . logs connection ends if log_connections = true

Should this be a separate GUC variable? Personally, I'd rather not have
my log files bloated with info on both the beginning *and* the end of
each connection.

> . provides a new option "log_line_format" which allows addition of extra
> information on a log line before the keyword.

This patch should also update the documentation.

Some minor stylistic comments follow...

> Index: backend/tcop/postgres.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/backend/tcop/postgres.c,v
> retrieving revision 1.357
> diff -c -w -r1.357 postgres.c
> *** backend/tcop/postgres.c 6 Aug 2003 17:46:45 -0000 1.357
> --- backend/tcop/postgres.c 9 Aug 2003 21:17:13 -0000
> + gettimeofday(&end,NULL);
> + if (end.tv_usec < port->session_start.tv_usec)
> + {
> + end.tv_sec--;
> + end.tv_usec += 1000000;
> + }
> + end.tv_sec -= port->session_start.tv_sec;

Can you add an assertion that end.tv_sec >= port->session_start.tv_sec before
this line, please?

> Index: backend/utils/error/elog.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/error/elog.c,v
> retrieving revision 1.119
> diff -c -w -r1.119 elog.c
> *** backend/utils/error/elog.c 8 Aug 2003 21:42:11 -0000 1.119
> --- backend/utils/error/elog.c 9 Aug 2003 21:17:14 -0000
> ***************
> *** 1019,1024 ****
> --- 1021,1089 ----
> }
> #endif /* HAVE_SYSLOG */
>
> + /*
> + * Formatted extra info for log if option is set and we have a data,
> + * or empty string otherwise.
> + */

This comment is a little awkward: "we have a data"?

> + static const char *
> + log_line_extra(void)
> + {
> + /* space for option string + 2 identifiers */
> + /* Note: if more identifers are built in this will have to increase */

"Identifier" is incorrectly spelled.

> + static char *result = NULL;

Why is this static? IMHO it would be much cleaner to just return a palloc'ed
string.

> + int flen = strlen(Log_line_format_str);
> + int i,j;
> + int tlen = 2*NAMEDATALEN + flen +3 ;

Can we choose some more helpful variable names than "flen" and
"tlen", please? Also, you can move the declaration of i & j to
the scope of the if statement.

> + {
> + char * dbname = MyProcPort->database_name;
> + char * username = MyProcPort->user_name;
> + if (dbname == NULL)
> + dbname="";
> + if (username == NULL)
> + username = "";

Could either of these ever be NULL?

> diff -c -w -r1.87 postgresql.conf.sample
> *** backend/utils/misc/postgresql.conf.sample 29 Jul 2003 00:03:18 -0000 1.87
> --- backend/utils/misc/postgresql.conf.sample 9 Aug 2003 21:17:15 -0000
> ***************
> *** 173,178 ****
> --- 173,179 ----
> #log_connections = false
> #log_duration = false
> #log_pid = false
> + #log_line_format = '<%U~%D>' # %U=username %D=databasename %%=%

Shouldn't the commented-out line have the default value? (which is "", right?)

-Neil