* andrey@stripped <andrey@stripped> [08/02/12 13:45]:
> ChangeSet@stripped, 2008-02-12 12:33:06+02:00, andrey@stripped +1 -0
> Fix for bug#22738 Events: After stop and start disabled events could reside in the
> queue
>
> Disabled events weren't removed from the memory queue after the scheduler has been
> re-enabled. After recalculation of next execution time of an event, it might get
> disabled.
The patch is OK to push, but have you considered the fact that
the event may be ON COMPLETION NOT PRESERVE?
In that case the events will be still present in the system table
until the next start of the server.
Is this an acceptable behaviour? Perhaps it should be reported
separately?
See also one comment below.
> sql/event_queue.cc@stripped, 2008-02-12 12:33:03+02:00, andrey@stripped
> +27 -2
> Sort the event queue in a way that the disabled events will always be
> at the end. We will use this for cleaning it, starting from the end.
>
> After recalculating times in the queue, after the scheduler has been enabled
> after disabled state, the queue should be cleaned from DISABLED events.
> The queue is sorted in a way such that the disabled events are at the end.
> Thus, we can start from the end of the queue and remove all DISABLED till we
> find the first with different state.
>
> diff -Nrup a/sql/event_queue.cc b/sql/event_queue.cc
> --- a/sql/event_queue.cc 2007-08-25 11:43:10 +03:00
> +++ b/sql/event_queue.cc 2008-02-12 12:33:03 +02:00
> @@ -60,8 +60,16 @@ extern "C" int event_queue_element_compa
>
> int event_queue_element_compare_q(void *vptr, uchar* a, uchar *b)
> {
> - my_time_t lhs = ((Event_queue_element *)a)->execute_at;
> - my_time_t rhs = ((Event_queue_element *)b)->execute_at;
> + Event_queue_element *left = (Event_queue_element *)a;
> + Event_queue_element *right = (Event_queue_element *)b;
> + my_time_t lhs = left->execute_at;
> + my_time_t rhs = right->execute_at;
> +
> + if (left->status == Event_queue_element::DISABLED)
> + return right->status != Event_queue_element::DISABLED;
> +
> + if (right->status == Event_queue_element::DISABLED)
> + return 1;
>
> return (lhs < rhs ? -1 : (lhs > rhs ? 1 : 0));
> }
> @@ -434,6 +442,23 @@ Event_queue::recalculate_activation_time
> ((Event_queue_element*)queue_element(&queue,
> i))->update_timing_fields(thd);
> }
> queue_fix(&queue);
> + /*
> + The disabled elements are moved to the end during the `fix`.
> + Start from the end and remove all of the elements which are disabled.
> + When we find the first non-disabled one we break, as we have removed all.
> + We don't call queue_removed() as this will start moving elements around which
> + is not needed as all disabled will be removed and the queue is already ordered.
> + */
> + for (i= queue.elements; i > 0; i--)
> + {
> + Event_queue_element *element = (Event_queue_element*)queue_element(&queue, i
> - 1);
> + if (element->status != Event_queue_element::DISABLED)
> + break;
> + delete element;
Please do not make assumptions about the structure of the queue
and use queue_remove here instead. Let's fix potential performance
issues when they arise.
Additionally, I would consider changing the sort order to put the
disabled elements to the *beginning* of the queue, always, so that
even if a disabled element is not removed in
recalculate_activation_time, we get to it first thing when we ask
for the next element to execute, and dispose of right away.
I think this approach is more error-proof.
Let's discuss if you think differently.
> + }
> + /* `i` will hold the number of elements in the queue */
> + queue.elements = i;
> + /* queue_remove will shift the data*/
I don't see where you call queue_remove here, perhaps a comment
left from the original version of the code?
> UNLOCK_QUEUE_DATA();
>
> DBUG_VOID_RETURN;
The patch is OK to push after addressing the review comments.
--
-- Konstantin Osipov Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com The best DATABASE COMPANY in the GALAXY
