Re: [Maria-developers] Rev 4029: MDEV-4856 SQL_ERROR_LOG shows 1146 errors which didnt appear in mysql client
Hi, Holyfoot! On Jan 31, holyfoot@askmonty.org wrote:
revno: 4029 revision-id: holyfoot@askmonty.org-20140131103303-1rx7ielo83f8iahe parent: holyfoot@askmonty.org-20140124020722-dd5twtwlcc8o1xiy committer: Alexey Botchkov <holyfoot@askmonty.org> branch nick: mdev-4856 timestamp: Fri 2014-01-31 14:33:03 +0400 message: MDEV-4856 SQL_ERROR_LOG shows 1146 errors which didnt appear in mysql client. The fill_schema_table() function used to call get_table_share() for a table name in WHERE then clear the error list. That way plugins receive the superfluous error notification if it happens in it. Also the problem was that error handler didn't prevent the suppressed error message from logging anyway as the logging happens in THD::raise_condition before the handler call. Fixed by adding the No_table_error_handler before the call. raise_condition() also fixed.
=== modified file 'sql/sql_class.cc' --- a/sql/sql_class.cc 2014-01-23 18:21:02 +0000 +++ b/sql/sql_class.cc 2014-01-31 10:33:03 +0000 @@ -1145,7 +1145,6 @@ MYSQL_ERROR* THD::raise_condition(uint s got_warning= 1; break; case MYSQL_ERROR::WARN_LEVEL_ERROR: - mysql_audit_general(this, MYSQL_AUDIT_GENERAL_ERROR, sql_errno, msg); break; default: DBUG_ASSERT(FALSE); @@ -1156,6 +1155,8 @@ MYSQL_ERROR* THD::raise_condition(uint s
if (level == MYSQL_ERROR::WARN_LEVEL_ERROR) { + mysql_audit_general(this, MYSQL_AUDIT_GENERAL_ERROR, sql_errno, msg); +
This is good.
=== modified file 'sql/sql_show.cc' --- a/sql/sql_show.cc 2013-11-19 12:16:25 +0000 +++ b/sql/sql_show.cc 2014-01-31 10:33:03 +0000 @@ -4199,8 +4199,13 @@ static int fill_schema_table_from_frm(TH key_length= create_table_def_key(thd, key, &table_list, 0); hash_value= my_calc_hash(&table_def_cache, (uchar*) key, key_length); mysql_mutex_lock(&LOCK_open); - share= get_table_share(thd, &table_list, key, - key_length, OPEN_VIEW, ¬_used, hash_value); + { + No_such_table_error_handler no_such_table_handler; + thd->push_internal_handler(&no_such_table_handler); + share= get_table_share(thd, &table_list, key, + key_length, OPEN_VIEW, ¬_used, hash_value); + thd->pop_internal_handler(); + }
But I don't understand that. Do you mean, old code did *not* suppress errors here? How could selects from I_S tables work without it? Regards, Sergei
But I don't understand that. Do you mean, old code did *not* suppress errors here? How could selects from I_S tables work without it?
Here's how it works today: get_all_tables() Trigger_error_handler err_handler; thd->push_internal_handler(&err_handler); fill_schema_table_from_frm get_table_share .... /* here we get the error of file not found */ my_message_sql THD::raise_condition /* here we test the err_handler */ /* but it doesn't react on that kind of errors */ mysql_audit_notify() stmt_da->set_error_status warning_info->push_warning / / .... /*error is returned back along the call stack */ / thd->clear_error(); /*which erases all the fileopen errors*/ / So basically the error is launched and then erased. Which doesn't help with the plugin notifications. To fix that i added one more error handler. Best regards. HF 03.02.2014 20:42, Sergei Golubchik wrote:
Hi, Holyfoot!
On Jan 31, holyfoot@askmonty.org wrote:
revno: 4029 revision-id: holyfoot@askmonty.org-20140131103303-1rx7ielo83f8iahe parent: holyfoot@askmonty.org-20140124020722-dd5twtwlcc8o1xiy committer: Alexey Botchkov <holyfoot@askmonty.org> branch nick: mdev-4856 timestamp: Fri 2014-01-31 14:33:03 +0400 message: MDEV-4856 SQL_ERROR_LOG shows 1146 errors which didnt appear in mysql client. The fill_schema_table() function used to call get_table_share() for a table name in WHERE then clear the error list. That way plugins receive the superfluous error notification if it happens in it. Also the problem was that error handler didn't prevent the suppressed error message from logging anyway as the logging happens in THD::raise_condition before the handler call. Fixed by adding the No_table_error_handler before the call. raise_condition() also fixed. === modified file 'sql/sql_class.cc' --- a/sql/sql_class.cc 2014-01-23 18:21:02 +0000 +++ b/sql/sql_class.cc 2014-01-31 10:33:03 +0000 @@ -1145,7 +1145,6 @@ MYSQL_ERROR* THD::raise_condition(uint s got_warning= 1; break; case MYSQL_ERROR::WARN_LEVEL_ERROR: - mysql_audit_general(this, MYSQL_AUDIT_GENERAL_ERROR, sql_errno, msg); break; default: DBUG_ASSERT(FALSE); @@ -1156,6 +1155,8 @@ MYSQL_ERROR* THD::raise_condition(uint s
if (level == MYSQL_ERROR::WARN_LEVEL_ERROR) { + mysql_audit_general(this, MYSQL_AUDIT_GENERAL_ERROR, sql_errno, msg); + This is good.
=== modified file 'sql/sql_show.cc' --- a/sql/sql_show.cc 2013-11-19 12:16:25 +0000 +++ b/sql/sql_show.cc 2014-01-31 10:33:03 +0000 @@ -4199,8 +4199,13 @@ static int fill_schema_table_from_frm(TH key_length= create_table_def_key(thd, key, &table_list, 0); hash_value= my_calc_hash(&table_def_cache, (uchar*) key, key_length); mysql_mutex_lock(&LOCK_open); - share= get_table_share(thd, &table_list, key, - key_length, OPEN_VIEW, ¬_used, hash_value); + { + No_such_table_error_handler no_such_table_handler; + thd->push_internal_handler(&no_such_table_handler); + share= get_table_share(thd, &table_list, key, + key_length, OPEN_VIEW, ¬_used, hash_value); + thd->pop_internal_handler(); + } But I don't understand that. Do you mean, old code did *not* suppress errors here? How could selects from I_S tables work without it?
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, Alexey! On Feb 14, Alexey Botchkov wrote:
But I don't understand that. Do you mean, old code did *not* suppress errors here? How could selects from I_S tables work without it?
Here's how it works today:
get_all_tables() Trigger_error_handler err_handler; thd->push_internal_handler(&err_handler); fill_schema_table_from_frm get_table_share .... /* here we get the error of file not found */ my_message_sql THD::raise_condition /* here we test the err_handler */ /* but it doesn't react on that kind of errors */ mysql_audit_notify() stmt_da->set_error_status warning_info->push_warning / / .... /*error is returned back along the call stack */ / thd->clear_error(); /*which erases all the fileopen errors*/ /
So basically the error is launched and then erased. Which doesn't help with the plugin notifications. To fix that i added one more error handler.
It's worse than that :) After thd->clear_error() the error flag is cleared, indeed. But all issued errors are still present in the warning list. I've looked into this about found this little gem: do_fill_table() function. It actually copies Warning_info list and removes warnings from there! Could you, please remove that piece of fine creativity and thd->clear_error() too and use the error handler instead? Looking at the logic of do_fill_table(), all you need to do is to filter out all errors, but keep all warnings. Like class Warnings_only_error_handler : public Internal_error_handler { public: bool handle_condition(THD *thd, uint sql_errno, const char* sqlstate, MYSQL_ERROR::enum_warning_level level, const char* msg, MYSQL_ERROR ** cond_hdl) { return level == MYSQL_ERROR::WARN_LEVEL_ERROR; } }; Regards, Sergei
Hi, Sergei.
Could you, please remove that piece of fine creativity and thd->clear_error() too and use the error handler instead?
There's one puzzle. We need to fill the INFORMATION_SCHEMA.TABLES.TABLE_COMMENT field. If a table exists but corrupt, that field presently is filled with the error message. Like here (tx.MYD and tx.MYI files were removed, only tx.frm left): mysql> SELECT TABLE_SCHEMA, TABLE_NAME, TABLE_TYPE, ENGINE, ROW_FORMAT, TABLE_ROWS, DATA_LENGTH, TABLE_CO MMENT FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_NAME = 'tx'; +--------------+------------+------------+--------+------------+------------+-------------+-------------- --------------------+ | TABLE_SCHEMA | TABLE_NAME | TABLE_TYPE | ENGINE | ROW_FORMAT | TABLE_ROWS | DATA_LENGTH | TABLE_COMMENT | +--------------+------------+------------+--------+------------+------------+-------------+-------------- --------------------+ | test | tx | BASE TABLE | NULL | NULL | NULL | NULL | Can't find file: 'tx' (errno: 2) | +--------------+------------+------------+--------+------------+------------+-------------+----------------------------------+ 1 row in set, 5 warnings (0.00 sec) This field is set here: sql_show.cc:4899 (get_schema_tables_record()) if (res || info_error) { /* If an error was encountered, push a warning, set the TABLE COMMENT column with the error text, and clear the error so that the operation can continue. */ const char *error= thd->is_error() ? thd->stmt_da->message() : ""; table->field[20]->store(error, strlen(error), cs); if (thd->is_error()) { push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN, thd->stmt_da->sql_errno(), thd->stmt_da->message()); thd->clear_error(); } } As we're supposed to block that error with the Error_handler, the error message gets lost there and cannot be recovered later in the get_schema_tables_record. So we either have stop showing the error message in that field, or we have to store it somewhere. The Error_handler looks like a good place to store that error message. But i can't figure out how to send it to the get_schema_tables_record() nicely. Another place is the TABLE_LIST * parameter. Seems to be convenient for eveything. Though the TABLE_LIST structure will get even more polluted. Sergei, what is your opinion here? Best regards. HF 17.02.2014 20:30, Sergei Golubchik wrote: Hi, Alexey! On Feb 14, Alexey Botchkov wrote:
But I don't understand that. Do you mean, old code did *not* suppress errors here? How could selects from I_S tables work without it? Here's how it works today:
get_all_tables() Trigger_error_handler err_handler; thd->push_internal_handler(&err_handler); fill_schema_table_from_frm get_table_share .... /* here we get the error of file not found */ my_message_sql THD::raise_condition /* here we test the err_handler */ /* but it doesn't react on that kind of errors */ mysql_audit_notify() stmt_da->set_error_status warning_info->push_warning / / .... /*error is returned back along the call stack */ / thd->clear_error(); /*which erases all the fileopen errors*/ /
So basically the error is launched and then erased. Which doesn't help with the plugin notifications. To fix that i added one more error handler. It's worse than that :) After thd->clear_error() the error flag is cleared, indeed. But all issued errors are still present in the warning list. I've looked into this about found this little gem: do_fill_table() function.
It actually copies Warning_info list and removes warnings from there!
Could you, please remove that piece of fine creativity and thd->clear_error() too and use the error handler instead?
Looking at the logic of do_fill_table(), all you need to do is to filter out all errors, but keep all warnings. Like
class Warnings_only_error_handler : public Internal_error_handler { public: bool handle_condition(THD *thd, uint sql_errno, const char* sqlstate, MYSQL_ERROR::enum_warning_level level, const char* msg, MYSQL_ERROR ** cond_hdl) { return level == MYSQL_ERROR::WARN_LEVEL_ERROR; } };
Regards, Sergei
Hi, Alexey! On Feb 19, Alexey Botchkov wrote:
Hi, Sergei.
Could you, please remove that piece of fine creativity and thd->clear_error() too and use the error handler instead?
There's one puzzle. This field is set here: sql_show.cc:4899 (get_schema_tables_record()) if (res || info_error) { /* If an error was encountered, push a warning, set the TABLE COMMENT column with the error text, and clear the error so that the operation can continue. */ const char *error= thd->is_error() ? thd->stmt_da->message() : ""; table->field[20]->store(error, strlen(error), cs);
if (thd->is_error()) { push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN, thd->stmt_da->sql_errno(), thd->stmt_da->message()); thd->clear_error(); } }
As we're supposed to block that error with the Error_handler, the error message gets lost there and cannot be recovered later in the get_schema_tables_record. So we either have stop showing the error message in that field, or we have to store it somewhere.
The Error_handler looks like a good place to store that error message. But i can't figure out how to send it to the get_schema_tables_record() nicely. Another place is the TABLE_LIST * parameter. Seems to be convenient for everything. Though the TABLE_LIST structure will get even more polluted.
Error handler looks perfect - it could have a char[] buffer and remember there the first error message for SHOW TABLES. But I agree that it's difficult to reach from get_schema_tables_record(). One option would be to use THD::get_internal_handler() it'll allow you to access the current error handler. The problem with this solution - there's no easy way to verify that you've got an object of the correct class. If someone has pushed another error handler after you, this will crash. Another solution could to have a char* pointer in THD or TABLE_LIST and set it to point to this buffer. This will work even if there's another handler on top of yours. But it'll need a pointer in THD or TABLE_LIST. I'd probably use the THD::get_internal_handler() approach and a test case for it (it'll make sure the calling convention is maintained). Seems silly to add another member to THD or TABLE_LIST that are used literally everywhere only to show an open table error in I_S.TABLES. Regards, Sergei
Hi. As you didn't react on my last patch about it, i decided to remind: http://lists.askmonty.org/pipermail/commits/2014-February/005931.html Best regards. HF 19.02.2014 20:43, Sergei Golubchik wrote:
Hi, Alexey!
Hi, Sergei.
Could you, please remove that piece of fine creativity and thd->clear_error() too and use the error handler instead? There's one puzzle. This field is set here: sql_show.cc:4899 (get_schema_tables_record()) if (res || info_error) { /* If an error was encountered, push a warning, set the TABLE COMMENT column with the error text, and clear the error so that the operation can continue. */ const char *error= thd->is_error() ? thd->stmt_da->message() : ""; table->field[20]->store(error, strlen(error), cs);
if (thd->is_error()) { push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN, thd->stmt_da->sql_errno(), thd->stmt_da->message()); thd->clear_error(); } }
As we're supposed to block that error with the Error_handler, the error message gets lost there and cannot be recovered later in the get_schema_tables_record. So we either have stop showing the error message in that field, or we have to store it somewhere.
The Error_handler looks like a good place to store that error message. But i can't figure out how to send it to the get_schema_tables_record() nicely. Another place is the TABLE_LIST * parameter. Seems to be convenient for everything. Though the TABLE_LIST structure will get even more polluted. Error handler looks perfect - it could have a char[] buffer and remember
On Feb 19, Alexey Botchkov wrote: there the first error message for SHOW TABLES.
But I agree that it's difficult to reach from get_schema_tables_record(). One option would be to use
THD::get_internal_handler()
it'll allow you to access the current error handler. The problem with this solution - there's no easy way to verify that you've got an object of the correct class. If someone has pushed another error handler after you, this will crash.
Another solution could to have a char* pointer in THD or TABLE_LIST and set it to point to this buffer. This will work even if there's another handler on top of yours. But it'll need a pointer in THD or TABLE_LIST.
I'd probably use the THD::get_internal_handler() approach and a test case for it (it'll make sure the calling convention is maintained). Seems silly to add another member to THD or TABLE_LIST that are used literally everywhere only to show an open table error in I_S.TABLES.
Regards, Sergei
participants (2)
-
Alexey Botchkov -
Sergei Golubchik