Re: [Maria-developers] MDEV-15 Log all SQL errors. in file:///home/hf/wmar/mdev15/
Hi, Holyfoot! Just two comments, see below. Please fix and push ASAP! On Mar 13, holyfoot@askmonty.org wrote:
------------------------------------------------------------ revno: 3320 revision-id: holyfoot@askmonty.org-20120313160142-ljcsacc11zb34uu6 parent: sergii@pisem.net-20120309082045-iyihz9ysqo82qb71 committer: Alexey Botchkov <holyfoot@askmonty.org> branch nick: mdev15 timestamp: Tue 2012-03-13 20:01:42 +0400 message: MDEV-15 Log all SQL errors. The logger service added to provide the rotating log functions. The SQL_ERROR_LOG plugin created to log the SQL errors usint that logger service.
=== added file 'include/mysql/service_logger.h' --- a/include/mysql/service_logger.h 1970-01-01 00:00:00 +0000 +++ b/include/mysql/service_logger.h 2012-03-13 16:01:42 +0000 + +typedef struct logger_service_file_st LOGGER_SERVICE_FILE;
small comment: please, rename to LOGGER_HANDLE or LOGGER_DESCRIPTOR, or, say, MYSQL_LOGGER. The point is to avoid the word "FILE", because, in principle, it may log to syslog or to a table or whatever. (and better to avoid SERVICE too, because 1) this structure is not a service, it's a handle, 2) services are supposed to be transparent and invisible, they pretend to be regular function calls)
+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
Fix this, please. Two last values are - version (as a string), and maturity. For example, "1.0", MariaDB_PLUGIN_MATURITY_ALPHA
+} +maria_declare_plugin_end;
Regards, Sergei
-----Original Message----- From: maria-developers- bounces+wlad=montyprogram.com@lists.launchpad.net [mailto:maria- developers-bounces+wlad=montyprogram.com@lists.launchpad.net] On Behalf Of Sergei Golubchik Sent: Dienstag, 13. März 2012 19:09 To: maria-developers@lists.launchpad.net Subject: Re: [Maria-developers] MDEV-15 Log all SQL errors. in file:///home/hf/wmar/mdev15/
Hi, Holyfoot!
Just two comments, see below. Please fix and push ASAP!
On Mar 13, holyfoot@askmonty.org wrote:
------------------------------------------------------------ revno: 3320 revision-id: holyfoot@askmonty.org-20120313160142-ljcsacc11zb34uu6 parent: sergii@pisem.net-20120309082045-iyihz9ysqo82qb71 committer: Alexey Botchkov <holyfoot@askmonty.org> branch nick: mdev15 timestamp: Tue 2012-03-13 20:01:42 +0400 message: MDEV-15 Log all SQL errors. The logger service added to provide the rotating log
functions.
The SQL_ERROR_LOG plugin created to log the SQL errors usint
Hi, this broke Windows build. Would it be possible to fix it? When looking at the patch, I found some things that IMO could be done better. 1. mysys/my_logger.c extern char *mysql_data_home; extern PSI_mutex_key key_LOCK_logger_service; I think referencing variables that do not belong to mysys in mysys is a layering violation. Can mysys be used without sql or not here. Shall everything that links to mysys link sql as well? I think - If there is a need to store data home and PSI key from sql layer, then this something can be passed via initialization function, rather than used directly. Alternatively: Is the functionality provided by my_logger required in mysys? If not, why it is be there, and can it be moved to where it is used, e.g into sql? 2. LOGGER_HANDLE *logger_open(const char *path, unsigned long size_limit, unsigned int rotations) Is (general case) 32 bit size limit limitation by design ? IF so, why is not "int" used (or int32 if one wants a fancy typename)? Files can be larger than 4GB. Even on 32 bits, and on 64 bit systems with 32 bit longs. Can this be fixed? Thanks, Wlad that
logger service.
=== added file 'include/mysql/service_logger.h' --- a/include/mysql/service_logger.h 1970-01-01 00:00:00 +0000 +++ b/include/mysql/service_logger.h 2012-03-13 16:01:42 +0000 + +typedef struct logger_service_file_st LOGGER_SERVICE_FILE;
small comment: please, rename to LOGGER_HANDLE or LOGGER_DESCRIPTOR, or, say, MYSQL_LOGGER. The point is to avoid the word "FILE", because, in principle, it may log to syslog or to a table or whatever. (and better to avoid SERVICE too, because 1) this structure is not a service, it's a handle, 2) services are supposed to be transparent and invisible, they pretend to be regular function calls)
+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
Fix this, please. Two last values are - version (as a string), and maturity. For example,
"1.0", MariaDB_PLUGIN_MATURITY_ALPHA
+} +maria_declare_plugin_end;
Regards, Sergei
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Hi, Vladislav! On Mar 14, Vladislav Vaintroub wrote:
Hi, this broke Windows build. Would it be possible to fix it?
When looking at the patch, I found some things that IMO could be done better.
1. mysys/my_logger.c
extern char *mysql_data_home; extern PSI_mutex_key key_LOCK_logger_service;
I think referencing variables that do not belong to mysys in mysys is a layering violation. Can mysys be used without sql or not here. Shall everything that links to mysys link sql as well?
I think - If there is a need to store data home and PSI key from sql layer, then this something can be passed via initialization function, rather than used directly. Alternatively: Is the functionality provided by my_logger required in mysys? If not, why it is be there, and can it be moved to where it is used, e.g into sql?
Right. As the logger is in mysys, the psi initialization of key_LOCK_logger_service should be in mysys. As for mysql_data_home - it's not needed at all here, I suspect. Plugins are initialized after the server has chdir-ed into mysql_data_home. So here we can use current directory and relative paths instead.
2. LOGGER_HANDLE *logger_open(const char *path, unsigned long size_limit, unsigned int rotations)
Is (general case) 32 bit size limit limitation by design ? IF so, why is not "int" used (or int32 if one wants a fancy typename)? Files can be larger than 4GB. Even on 32 bits, and on 64 bit systems with 32 bit longs.
Can this be fixed?
ulonglong would be more appropriate, indeed Regards, Sergei
participants (2)
-
Sergei Golubchik -
Vladislav Vaintroub