| Bug #41948 | Query_log_event constructor needlessly contorted | ||
|---|---|---|---|
| Submitted: | 8 Jan 2009 8:56 | Modified: | 24 Jun 2009 15:27 |
| Reporter: | Mats Kindahl | Email Updates: | |
| Status: | Closed | Impact on me: | |
| Category: | MySQL Server: Replication | Severity: | S3 (Non-critical) |
| Version: | OS: | Any | |
| Assigned to: | Zhenxing He | CPU Architecture: | Any |
[8 Jan 2009 17:28]
Mark Callaghan
Please fix this. Incorrectly set error codes for binlog events -- error code set after the logged operation was successful because the thread was killed after the event and before binlog event generation -- has caused many replication halts for us in production.
[22 May 2009 7:29]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/74770 2884 He Zhenxing 2009-05-22 BUG#41948 Query_log_event constructor needlessly contorted Make the caller of Query_log_event, Execute_load_log_event constructors and THD::binlog_query to provide the error code instead of having the constructors to figure out the error code.
[26 May 2009 3:34]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/74928 2884 He Zhenxing 2009-05-26 BUG#41948 Query_log_event constructor needlessly contorted Make the caller of Query_log_event, Execute_load_log_event constructors and THD::binlog_query to provide the error code instead of having the constructors to figure out the error code. @ sql/log_event.cc Changed constructors of Query_log_event and Execute_load_log_event to accept the error code argument instead of figuring it out by itself @ sql/log_event.h Changed constructors of Query_log_event and Execute_load_log_event to accept the error code argument
[26 May 2009 3:42]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/74929 2884 He Zhenxing 2009-05-26 BUG#41948 Query_log_event constructor needlessly contorted Make the caller of Query_log_event, Execute_load_log_event constructors and THD::binlog_query to provide the error code instead of having the constructors to figure out the error code. @ sql/log_event.cc Changed constructors of Query_log_event and Execute_load_log_event to accept the error code argument instead of figuring it out by itself @ sql/log_event.h Changed constructors of Query_log_event and Execute_load_log_event to accept the error code argument
[30 May 2009 13:32]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/75307 2914 He Zhenxing 2009-05-30 BUG#41948 Query_log_event constructor needlessly contorted Make the caller of Query_log_event, Execute_load_log_event constructors and THD::binlog_query to provide the error code instead of having the constructors to figure out the error code. @ sql/log_event.cc Changed constructors of Query_log_event and Execute_load_log_event to accept the error code argument instead of figuring it out by itself @ sql/log_event.h Changed constructors of Query_log_event and Execute_load_log_event to accept the error code argument
[30 May 2009 13:54]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/75309 2915 He Zhenxing 2009-05-30 [merge] Merge BUG#41948
[31 May 2009 9:15]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/75328 2915 He Zhenxing 2009-05-31 [merge] Merge BUG#41948
[31 May 2009 13:16]
Zhenxing He
pushed to 5.1-bugteam and 6.0-bugteam
[16 Jun 2009 11:04]
Bugs System
Pushed into 5.1.36 (revid:joro@sun.com-20090616102155-3zhezogudt4uxdyn) (version source revid:zhenxing.he@sun.com-20090531091535-muxt9b4f4vgo353b) (merge vers: 5.1.36) (pib:6)
[17 Jun 2009 19:27]
Bugs System
Pushed into 5.4.4-alpha (revid:alik@sun.com-20090616183122-chjzbaa30qopdra9) (version source revid:zhenxing.he@sun.com-20090531120501-4p5pu2outy6qvls8) (merge vers: 6.0.12-alpha) (pib:11)
[24 Jun 2009 15:27]
Jon Stephens
Documented bugfix in the 5.1.36 and 5.4.4 changelogs as follows:
The Query_log_event used by replication to transfer a query to
the slave has been refactored. Query_log_event also stores and
sends the error code resulting from the execution since it, in
some cases, is necessary to execute the statement on the slave
as well, which should result in the same error code. The
Query_log_event constructor previously worked out for itself the
error code using a complex routine, the result of which was
often set aside within the constructor itself. This was also
involved with at least 2 known bugs relating to invalid errors,
and taken as a clear sign that the constructor was not
well-designed and needed to be re-written.
[12 Aug 2009 22:07]
Paul DuBois
Noted in 5.4.2 changelog because next 5.4 version will be 5.4.2 and not 5.4.4.
[14 Aug 2009 22:57]
Paul DuBois
Ignore previous comment about 5.4.2.
[26 Aug 2009 13:46]
Bugs System
Pushed into 5.1.37-ndb-7.0.8 (revid:jonas@mysql.com-20090826132541-yablppc59e3yb54l) (version source revid:jonas@mysql.com-20090826132541-yablppc59e3yb54l) (merge vers: 5.1.37-ndb-7.0.8) (pib:11)
[26 Aug 2009 13:46]
Bugs System
Pushed into 5.1.37-ndb-6.3.27 (revid:jonas@mysql.com-20090826105955-bkj027t47gfbamnc) (version source revid:jonas@mysql.com-20090826105955-bkj027t47gfbamnc) (merge vers: 5.1.37-ndb-6.3.27) (pib:11)
[26 Aug 2009 13:48]
Bugs System
Pushed into 5.1.37-ndb-6.2.19 (revid:jonas@mysql.com-20090825194404-37rtosk049t9koc4) (version source revid:jonas@mysql.com-20090825194404-37rtosk049t9koc4) (merge vers: 5.1.37-ndb-6.2.19) (pib:11)
[27 Aug 2009 16:33]
Bugs System
Pushed into 5.1.35-ndb-7.1.0 (revid:magnus.blaudd@sun.com-20090827163030-6o3kk6r2oua159hr) (version source revid:jonas@mysql.com-20090826132541-yablppc59e3yb54l) (merge vers: 5.1.37-ndb-7.0.8) (pib:11)
[7 Oct 2009 14:36]
Paul DuBois
The 5.4 fix has been pushed to 5.4.2.
Description: The Query_log_event used by replication to transfer a query to the slave also stores and sends the error code resulting from the execution since it, in some cases, is necessary to execute the statement on the slave as well, which should result in the same error code. The Query_log_event constructor figures out the error code itself using a quite contorted piece of code, and since there are several uses that does not want the error code set, the error code is immediately after set to 0 to indicate no error. The fact that it is a common case to set the error code to something else is a clear sign that the constructor is not well-designed and needs to be re-written. How to repeat: Look in the code, where the following piece of code can be found: Query_log_event::Query_log_event(THD* thd_arg, const char* query_arg, ... if (killed_status_arg == THD::KILLED_NO_VALUE) killed_status_arg= thd_arg->killed; error_code= (killed_status_arg == THD::NOT_KILLED) ? (thd_arg->is_error() ? thd_arg->main_da.sql_errno() : 0) : ((thd_arg->system_thread & SYSTEM_THREAD_DELAYED_INSERT) ? 0 : thd_arg->killed_errno()); Suggested fix: Instead of having the constructor figure out the error code, the caller should provide the error code for the object. This can be passed as a parameter to the constructor. Since the caller usually knows what the error code is, or how to find it, this change will simplify the code, reduce the risk of potential regression bugs (where the constructor code does the wrong thing) and is very likely to also speed up the code significantly.