MySQL Bugs: #68493: unnecessary dict_update_statistics during concurrent updates

Bug #68493	unnecessary dict_update_statistics during concurrent updates
Submitted:	26 Feb 2013 8:14	Modified:	8 Apr 2013 14:42
Reporter:	xiaobin lin (OCA)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S3 (Non-critical)
Version:	5.5+	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	dict_update_statistics

Description:
in 5.5.29, in the logic of row_update_statistics_if_needed, calling dict_update_statistics based on the value of stat_modified_counter. But if there are more than one threads comes into here, it may leads to unnecessary server load.

How to repeat:
In a test , I find there are 11 treads calling dict_update_statistics  at the same time (from pstack result, attached bellow)

Suggested fix:
I think after one thread in dict_update_statistics get the x_lock , the condition can be checked again, if not satified now (means another thread has just done), return directly. As patch bellow

The pstack result mentioned above

Attachment: pstack.rt (application/octet-stream, text), 144.42 KiB.

Based on 5.5.29

Attachment: patch.dict_update_statistics.5529.diff (application/octet-stream, text), 722 bytes.

Based on 5.5.29 (sorry for mistaked attache it in the flle tab) (*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: patch.dict_update_statistics.5529.diff (application/octet-stream, text), 722 bytes.

Hello and thank you for filling this bug report and writing a patch for it!

The patch has the problem that it would cancel the update of the
statistics even if dict_update_statistics() is not called from
row_update_statistics_if_needed().

Based on comment from Vasil, this means that in the current form, this contribution will be rejected. Some rework needs to happen before it is pushable.

Hi,Vasil, yes, thank you for pointing it. I used to want to avoid the times of "lock", but it seems mis-understanding. The new version looks simpler.

based on 5.5.29 (*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: patch.dict_update_statistics.5529.diff (application/octet-stream, text), 1.85 KiB.

fixup patch

Attachment: bug16400412-03.diff (application/octet-stream, text), 8.64 KiB.

Hello xiaobin lin,

Thank you for submitting a second patch for this bug! It does not actually solve the problem because multiple threads could enter dict_update_statistics() after the unlock you have added.

The following patch is going to be pushed to the official 5.5 tree: http://bugs.mysql.com/file.php?id=19845

Hi, Dimov ,

  Thank you for feedback. In fact, my second patch can solve the problem. There is a double check "need_check" when locking, so the threads that do not match "DICT_TABLE_CHANGED_TOO_MUCH" will not enter dict_update_statistics(). The patch you attached can solve too, though more pieces of code need to be modified :)
 Both are OK. Thank you.

xiaobin lin,

I think that there is the following problem with your second patch, which is irrelevant now since a different patch has already been committed to the tree, but anyway:

953 inline ibool check_need_update_statistics(dict_table_t*   table, ulint counter)
954 {
955         return (counter > 2000000000
956                 || ((ib_int64_t)counter > 16 + table->stat_n_rows / 16));
957 }
...
970         counter = table->stat_modified_counter;
971 
972         table->stat_modified_counter = counter + 1;
973 
974         /* Calculate new statistics if 1 / 16 of table has been modified
975         since the last time a statistics batch was run, or if
976         stat_modified_counter > 2 000 000 000 (to avoid wrap-around).
977         We calculate statistics at most every 16th round, since we may have
978         a counter table which is very small and updated very often. */
979 
980         need_check = check_need_update_statistics(table, counter);
981 
982         if (need_check)
983         {
984           /*re-check under lock, to avoid unnecessary calling*/
985           dict_table_stats_lock(table, RW_X_LATCH);
986           need_check = check_need_update_statistics(table, table->stat_modified_counter);
987           dict_table_stats_unlock(table, RW_X_LATCH);
988 
989           if (need_check)
990           {
991             dict_update_statistics(table, FALSE /* update even if stats
992                                                    are initialized */);
993           }
994         }

Imagine two threads: T1 and T2, both executing concurrently this piece of code starting around line 970.

On line 980 both threads will assign TRUE to their 'need_check' variable
Lets assume T1 manages to first grab the latch at line 985
T2 waits for the latch to be released
T1 calls check_need_update_statistics() again which evaluates to TRUE, releases the latch and enters dict_update_statistics()
T2 is waked up, acquires the latch at line 985, calls check_need_update_statistics() again which evaluates to TRUE (!), releases the latch and enters dict_update_statistics()

you see - both threads entered dict_update_statistics().

In order for that patch to solve the problem you have to do something between lines 985 and 987, so that further calls to check_need_update_statistics() from _other_ threads would evaluate to FALSE.

Anyway - this is irrelevant now since a proper patch has been committed, just wanted to clarify.

Thank you!

Hi Xiaobin,

As Vasil writes, he has written another patch that will fix the problem. Thank you for your patch which was used as an inspiration for the patch by Vasil.

Thanks again,
Staale

Added changelog entry to 5.5.32, 5.6.12, 5.7.2:

"Multiple concurrent calls to "dict_update_statistics()" would result in unnecessary server load."