Hi, Alexey! On Mar 09, Alexey Botchkov wrote:

3316 Alexey Botchkov 2012-03-09 MDEV-15 Log all SQL errors. Added the logger service that provides us with the rotating logs. The plugin SQL_ERROR_LOG added. It logs the errors using the 'logger service' for the rotating log files. the example record from the log: 2012-03-09 15:07:29 root[root] @ localhost [] ERROR 1146: Table 'test.xyz' doesn't exist : select * from test.xyz

Pretty much ok. See comments below. Six of them (that use imperative) I ask you to do. The rest are suggestions, opinions, or questions. And one question (static declaration of logs) I will discuss with you on irc.

=== added file 'include/mysql/service_logger.h' --- include/mysql/service_logger.h 1970-01-01 00:00:00 +0000 +++ include/mysql/service_logger.h 2012-03-07 10:29:14 +0000 @@ -0,0 +1,97 @@ +/* Copyright (C) 2012 Monty Program Ab + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ + +#ifndef MYSQL_SERVICE_LOGGER_INCLUDED +#define MYSQL_SERVICE_LOGGER_INCLUDED + +#ifndef MYSQL_ABI_CHECK +#include <stdarg.h> +#endif + +/** + @file + logger service + + Log file with rotation implementation. + + This service implements logging with possible rotation + of the log files. Interface intentionally tries to be similar to FILE* + related functions. + + So that one can open the log with logger_open(), specifying + the limit on the logfile size and the rotations number. + + Then it's possible to write messages to the log with + logger_printf or logger_vprintf functions. + + As the size of the logfile grows over the specified limit, + it is renamed to 'logfile.1'. The former 'logfile.1' becomes + 'logfile.2', etc. The file 'logfile.rotations' is removed. + That's how the rotation works. + + The rotation can be forced with the logger_rotate() call. + + Finally the log should be closed with logger_close(). + +@notes: + Implementation checks the size of the log file before it starts new + printf into it. So the size of the file gets over the limit when it rotates. + + The access is secured with the mutex, so the log is threadsafe. +*/ + + +#ifdef __cplusplus +extern "C" { +#endif + +typedef struct logger_service_file_st LOGGER_SERVICE_FILE; + +extern struct logger_service_st { + LOGGER_SERVICE_FILE* (*open)(const char *path, + unsigned long size_limit, + unsigned int rotations); + int (*close)(LOGGER_SERVICE_FILE *log); + int (*vprintf)(LOGGER_SERVICE_FILE *log, const char *fmt, va_list argptr); + int (*printf)(LOGGER_SERVICE_FILE *log, const char *fmt, ...); + int (*rotate)(LOGGER_SERVICE_FILE *log); +} *logger_service; + +#ifdef MYSQL_DYNAMIC_PLUGIN + +#define logger_open logger_service->open

you can write this with parentheses #define logger_open(P,L,R) logger_service->open(P,L,R)

+#define logger_close(log) (logger_service->close(log)) +#define logger_rotate(log) (logger_service->rotate(log)) +#define logger_vprintf(log, fmt, argptr) (logger_service->\ + vprintf(log, fmt, argptr)) +#define logger_printf logger_service->printf +#else + + LOGGER_SERVICE_FILE *logger_open(const char *path, + unsigned long size_limit, + unsigned int rotations); + int logger_close(LOGGER_SERVICE_FILE *log); + int logger_vprintf(LOGGER_SERVICE_FILE *log, const char *fmt, va_list argptr); + int logger_rotate(LOGGER_SERVICE_FILE *log); + int logger_printf(LOGGER_SERVICE_FILE *log, const char *fmt, ...); +#endif + + +#ifdef __cplusplus +} +#endif + +#endif /*MYSQL_SERVICE_LOGGER_INCLUDED*/ + === added file 'mysys/my_logger.c' --- mysys/my_logger.c 1970-01-01 00:00:00 +0000 +++ mysys/my_logger.c 2012-03-09 13:03:47 +0000 @@ -0,0 +1,167 @@ +/* Copyright (C) 2012 Monty Program Ab + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ + + +#include "my_global.h" +#include <my_sys.h> +#include <mysql/service_logger.h> + +extern MYSQL_PLUGIN_IMPORT char *mysql_data_home;

why MYSQL_PLUGIN_IMPORT? Because the linker cannot find the symbol otherwise? that is, MYSQL_PLUGIN_IMPORT affects how the name is mangled?

+extern PSI_mutex_key key_LOCK_logger_service; + +typedef struct logger_service_file_st { + FILE *file; + char path[FN_REFLEN]; + long size_limit; + unsigned int rotations; + unsigned int index_len; + size_t path_len; + mysql_mutex_t lock; +} LSFS; + + +static unsigned int n_dig(unsigned int i) +{ + return (i == 0) ? 0 : ((i < 10) ? 1 : ((i < 100) ? 2 : 3)); +} + + +LOGGER_SERVICE_FILE *logger_open(const char *path, + unsigned long size_limit, + unsigned int rotations) +{ + LOGGER_SERVICE_FILE new_log, *l_perm; + + /* I don't think we need more rotations, */ + /* but if it's so, the rotation procedure should be adapted to it. */

please write multi-line comments as we do elsewhere in the code

+ if (rotations > 999) + return 0; + + new_log.rotations= rotations; + new_log.size_limit= size_limit; + new_log.index_len= n_dig(rotations);

I don't think you need to cache the value of n_dig(rotations) it's only used once, in logname() function.

+ new_log.path_len= strlen(fn_format(new_log.path, path, + mysql_data_home, "", 4));

replace 4 with a named constant, they exist in 5.5

+ + if (new_log.path_len+new_log.index_len+1 > FN_REFLEN || + !(new_log.file= fopen(new_log.path, "a+"))) + { + /* File path too long || Check errno for the cause */

set errno=ENAMETOOLONG here. That is, split this if() in two, and set errno appropriately in the first one.

+ return 0; + } + + setbuf(new_log.file, 0); + fseek(new_log.file, 0, SEEK_END); + if (!(l_perm= (LOGGER_SERVICE_FILE *) + my_malloc(sizeof(LOGGER_SERVICE_FILE), MYF(0))))

close the file here

+ return 0; /* End of memory */ + *l_perm= new_log; + mysql_mutex_init(key_LOCK_logger_service, &l_perm->lock, MY_MUTEX_INIT_FAST); + return l_perm; +} + +int logger_close(LOGGER_SERVICE_FILE *log) +{ + int result; + mysql_mutex_destroy(&log->lock); + result= fclose(log->file); + my_free(log); + return result; +} + + +static char *logname(LOGGER_SERVICE_FILE *log, char *buf, unsigned int n_log) +{ + unsigned int n_zeroes; + size_t path_len= log->path_len; + buf[path_len++]= '.'; + for (n_zeroes= log->index_len - n_dig(n_log); + n_zeroes > 0; n_zeroes--, path_len++) + buf[path_len]= '0'; + + int10_to_str(n_log, buf+path_len, 10); + return buf;

as I wrote earlier, I'd simply used sprintf("%0*d") here. Like this: sprintf(buf + path_len, ".%0*u", log->index_len, n_log); but ok, as you like.

+} + + +static int do_rotate(LOGGER_SERVICE_FILE *log) +{ + char namebuf[FN_REFLEN]; + struct stat stbuf; + int result; + unsigned int i; + char *buf_old, *buf_new, *tmp; + + memcpy(namebuf, log->path, log->path_len); + + buf_new= logname(log, namebuf, log->rotations); + buf_old= log->path; + for (i=log->rotations-1; i>0; i--) + { + logname(log, buf_old, i); + if (!stat(buf_old, &stbuf) &&

why stat? iirc, access(name, F_OK) is supposed to be used for checking if a file exists. We aren't interested in the values from stbuf here, are we?

+ (result= my_rename(buf_old, buf_new, MYF(0)))) + return result; + tmp= buf_old; + buf_old= buf_new; + buf_new= tmp; + } + if ((result= fclose(log->file))) + return result; + namebuf[log->path_len]= 0; + result= my_rename(namebuf, logname(log, log->path, 1), MYF(0)); + log->file= fopen(namebuf, "a+"); + return log->file==NULL || result; +} + + +int logger_vprintf(LOGGER_SERVICE_FILE *log, const char* fmt, va_list ap) +{ + int result; + mysql_mutex_lock(&log->lock); + if (ftell(log->file) >= log->size_limit && + do_rotate(log)) + { + result= -1; + goto exit; /* Log rotation needed but failed */ + } + + result= vfprintf(log->file, fmt, ap);

use my_vfprintf

+exit: + mysql_mutex_unlock(&log->lock); + return result; +} + + +int logger_rotate(LOGGER_SERVICE_FILE *log) +{ + int result; + mysql_mutex_lock(&log->lock); + result= do_rotate(log); + mysql_mutex_unlock(&log->lock); + return result; +} + + +int logger_printf(LOGGER_SERVICE_FILE *log, const char *fmt, ...) +{ + int result; + va_list args; + va_start(args,fmt); + result= logger_vprintf(log, fmt, args); + va_end(args); + return result; +} + === added file 'plugin/sql_errlog/sql_errlog.c' --- plugin/sql_errlog/sql_errlog.c 1970-01-01 00:00:00 +0000 +++ plugin/sql_errlog/sql_errlog.c 2012-03-09 10:24:35 +0000 @@ -0,0 +1,147 @@ +/* Copyright (C) 2012 Monty Program Ab. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ + +#include <mysql/plugin_audit.h> +#include <stdio.h> +#include <time.h> + +/* + rate 0 means the logging was disabled. +*/ + +static char *filename; +static unsigned int rate; +static unsigned int size_limit; +static unsigned int rotations; +static char rotate; + +static unsigned int count; +LOGGER_SERVICE_FILE *logfile; + +static void rotate_log(MYSQL_THD thd, struct st_mysql_sys_var *var, + void *var_ptr, const void *save); + +static MYSQL_SYSVAR_UINT(rate, rate, PLUGIN_VAR_RQCMDARG, + "Sampling rate. If set to 0(zero), the logging disabled.", NULL, NULL,

typo: the logging *is* disabled

+ 1, 0, 1000000, 1); + +static MYSQL_SYSVAR_UINT(size_limit, size_limit, + PLUGIN_VAR_READONLY, "Log file size limit", NULL, NULL, + 1000000, 100, 1024*1024L*1024L, 1); + +static MYSQL_SYSVAR_UINT(rotations, rotations, + PLUGIN_VAR_READONLY, "Number of rotations before log is removed.", + NULL, NULL, 9, 1, 999, 1); + +static MYSQL_SYSVAR_BOOL(rotate, rotate, + PLUGIN_VAR_NOSYSVAR, "Force log rotation", NULL, rotate_log,

What do you mean PLUGIN_VAR_NOSYSVAR? It only works from a command line? It'd be more useful to force a log rotation from SQL. if users wouldn't need it, we woldn't have FLUSH LOGS :)

+ 0); + +static MYSQL_SYSVAR_STR(filename, filename, + PLUGIN_VAR_READONLY | PLUGIN_VAR_RQCMDARG, + "The file to log sql errors to", NULL, NULL, + "sql_errors.log"); + +static struct st_mysql_sys_var* vars[] = { + MYSQL_SYSVAR(rate), + MYSQL_SYSVAR(size_limit), + MYSQL_SYSVAR(rotations), + MYSQL_SYSVAR(rotate), + MYSQL_SYSVAR(filename), + NULL +}; + + +static void log_sql_errors(MYSQL_THD thd __attribute__((unused)), + unsigned int event_class __attribute__((unused)), + const void *ev) +{ + const struct mysql_event_general *event = + (const struct mysql_event_general*)ev; + if (rate && + event->event_subclass == MYSQL_AUDIT_GENERAL_ERROR) + { + if (++count >= rate) + { + struct tm t; + time_t event_time = event->general_time; + + count = 0; + localtime_r(&event_time, &t); + logger_printf(logfile, "%04d-%02d-%02d %2d:%02d:%02d " + "%s ERROR %d: %s : %s\n", + t.tm_year + 1900, t.tm_mon + 1, + t.tm_mday, t.tm_hour, t.tm_min, t.tm_sec, + event->general_user, event->general_error_code, + event->general_command, event->general_query); + } + } +} + + +static int sql_error_log_init(void *p __attribute__((unused))) +{ + logfile= logger_open(filename, size_limit, rotations); + if (logfile == NULL) { + fprintf(stderr, "Could not create file '%s'\n", + filename); + return 1; + } + count = 0; + return 0; +} + + +static int sql_error_log_deinit(void *p __attribute__((unused))) +{ + logger_close(logfile); + return 0; +} + + +static void rotate_log(MYSQL_THD thd __attribute__((unused)), + struct st_mysql_sys_var *var __attribute__((unused)), + void *var_ptr __attribute__((unused)), + const void *save __attribute__((unused))) +{ + (void) logger_rotate(logfile); +} + + +static struct st_mysql_audit descriptor = +{ + MYSQL_AUDIT_INTERFACE_VERSION, + NULL, + log_sql_errors, + { MYSQL_AUDIT_GENERAL_CLASSMASK } +}; + +maria_declare_plugin(sql_errlog) +{ + MYSQL_AUDIT_PLUGIN, + &descriptor, + "SQL_ERROR_LOG", + "Alexey Botchkov", + "Log SQL level errors to a file with rotation", + PLUGIN_LICENSE_GPL, + sql_error_log_init, + sql_error_log_deinit, + 0x0100, + NULL, + vars, + NULL, + 0 +} +maria_declare_plugin_end;

Regards, Sergei