Hi Sergei, On 12/27/2017 08:33 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Dec 26, Alexander Barkov wrote:
Hi Sergei,
can you please review a patch for MDEV-14603?
I agree with the fix.
But I don't like that there are many things to backup/restore (Statement, arena, free_list, and now change_list), they're all saved/restored in different places - it's easy to miss something when making changes.
Would it be possible to move all that saving/restoring into dedicated helpers and use them in all three places (prepare, execute, execute immediate)?
Thanks for a good idea. Done. Please find attached. By the way, it seems this approach with a helper class is fixing one more potential problem. Details: Before the patch mysql_sql_stmt_prepare() did not backup/restore change_list, but also it did not backup/restore free_list. Only mysql_sql_stmt_execute() and mysql_sql_stmt_execute_immediate() preserved/restored free_list. I think this was wrong. I could not invent a test case to get some unexpected behavior because of free_list not preserved/restored in mysql_sql_stmt_prepare(). But logically it looked not correct for me. In a query like this: PREPARE stmt FROM CONCAT('SELECT 1'); Item_func_concat and Item_string('SELECT 1') were deleted with this call stack: #0 Item::delete_self #1 Query_arena::free_items #2 THD::cleanup_after_query #3 Prepared_statement::cleanup_stmt #4 Prepared_statement::prepare ("SELECT 1") So it was an impression that those items belonged to the "SELECT" query. But in fact they belong to the "PREPARE" query. After this patch Item_func_concat and Item_string are deleted with this stack: #0 Item::delete_self #1 0x0000555555abf330 in Query_arena::free_items #2 0x0000555555aba9ab in THD::cleanup_after_query #3 0x0000555555b104fe in mysql_parse ( rawbuf=0x7fff580131e8 "PREPARE stmt FROM CONCAT('SELECT 1')",... Which seems to be more correct. Thanks!