Re: [Maria-developers] [Commits] 97d212a: MDEV-10545: Server crashed in my_copy_fix_mb on querying I_S and P_S tables
Hi Svoj!
Thanks for the review.
On Fri, Sep 2, 2016 at 1:41 AM, Sergey Vojtovich
Hi Nirbhay,
Ok to push, one question below.
revision-id: 97d212a34ff4e0558126bc393bbef97036611d83 (mariadb-10.1.17-4-g97d212a) parent(s): a322651b8aa702e58d473edfae26606f10a089fb author: Nirbhay Choubey committer: Nirbhay Choubey timestamp: 2016-09-01 12:46:56 -0400 message:
MDEV-10545: Server crashed in my_copy_fix_mb on querying I_S and P_S
On Thu, Sep 01, 2016 at 12:46:58PM -0400, Nirbhay Choubey wrote: tables
Once THDs have been added to the global "threads" list, they must modify query_string only after acquiring per- thread LOCK_thd_data mutex.
--- sql/log_event.cc | 2 +- sql/sql_acl.cc | 8 ++++---- sql/sql_parse.cc | 7 +++---- 3 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/sql/log_event.cc b/sql/log_event.cc index afa58af..66e7c60 100644 --- a/sql/log_event.cc +++ b/sql/log_event.cc @@ -6022,7 +6022,7 @@ int Load_log_event::do_apply_event(NET* net,
rpl_group_info *rgi,
new_db.str= (char *) rpl_filter->get_rewrite_db(db, &new_db.length); thd->set_db(new_db.str, new_db.length); DBUG_ASSERT(thd->query() == 0); - thd->reset_query_inner(); // Should not be needed + if (thd->query()) thd->reset_query(); // Should not be needed Why if? Original code didn't have it.
Now that we call reset_query(), the check has been added to avoid it and an extra mutex. And from the above assert, I think it will be ever better have this check within unlikely(). Best, Nirbhay
Regards, Sergey _______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
Hi Nirbhay, On Fri, Sep 02, 2016 at 09:00:27AM -0400, Nirbhay Choubey wrote:
diff --git a/sql/log_event.cc b/sql/log_event.cc index afa58af..66e7c60 100644 --- a/sql/log_event.cc +++ b/sql/log_event.cc @@ -6022,7 +6022,7 @@ int Load_log_event::do_apply_event(NET* net, rpl_group_info *rgi, new_db.str= (char *) rpl_filter->get_rewrite_db(db, &new_db.length); thd->set_db(new_db.str, new_db.length); DBUG_ASSERT(thd->query() == 0); - thd->reset_query_inner(); // Should not be needed + if (thd->query()) thd->reset_query(); // Should not be needed Why if? Original code didn't have it.
Now that we call reset_query(), the check has been added to avoid it and an extra mutex. And from the above assert, I think it will be ever better have this check within unlikely(). Taking into account this assert I'd say there's no point in reset_query at all.
Regards, Sergey
Hi Svoj,
On Fri, Sep 2, 2016 at 9:10 AM, Sergey Vojtovich
Hi Nirbhay,
On Fri, Sep 02, 2016 at 09:00:27AM -0400, Nirbhay Choubey wrote:
diff --git a/sql/log_event.cc b/sql/log_event.cc index afa58af..66e7c60 100644 --- a/sql/log_event.cc +++ b/sql/log_event.cc @@ -6022,7 +6022,7 @@ int Load_log_event::do_apply_event(NET* net, rpl_group_info *rgi, new_db.str= (char *) rpl_filter->get_rewrite_db(db, &new_db.length); thd->set_db(new_db.str, new_db.length); DBUG_ASSERT(thd->query() == 0); - thd->reset_query_inner(); // Should not be needed + if (thd->query()) thd->reset_query(); // Should not be needed Why if? Original code didn't have it.
Now that we call reset_query(), the check has been added to avoid it and an extra mutex. And from the above assert, I think it will be ever better have this check within unlikely(). Taking into account this assert I'd say there's no point in reset_query at all.
Ok, I will remove it. - Nirbhay
_______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
participants (2)
-
Nirbhay Choubey -
Sergey Vojtovich