From 12ea1d7540efd3d3ec86beb6f782b26936d9277e Mon Sep 17 00:00:00 2001 From: unknown <monty@mysql.com> Date: Wed, 6 Oct 2004 01:24:21 +0300 Subject: [PATCH] Reverted patch for new usage of open_count as it caused more problems than it solved Cleaned up patch for checking locks for multi-table updates myisam/mi_close.c: Reverted patch for new usage of open_counts myisam/mi_locking.c: Reverted patch for new usage of open_counts sql/ha_myisam.cc: Reverted patch for new usage of open_counts sql/handler.cc: Removed compiler warning sql/sql_acl.cc: Removed compiler warning sql/sql_table.cc: No need to unlock after failed call to external_lock() sql/sql_update.cc: Cleaned up (and made it more secure) patch for checking locks for multi-table updates --- myisam/mi_close.c | 9 +++- myisam/mi_locking.c | 117 +++++++++++++------------------------------- sql/ha_myisam.cc | 26 ++-------- sql/handler.cc | 2 +- sql/sql_acl.cc | 2 +- sql/sql_table.cc | 6 +-- sql/sql_update.cc | 110 +++++++++++++++++++++++++---------------- 7 files changed, 115 insertions(+), 157 deletions(-) diff --git a/myisam/mi_close.c b/myisam/mi_close.c index 47308a5e9eb..2712f0ca283 100644 --- a/myisam/mi_close.c +++ b/myisam/mi_close.c @@ -70,8 +70,13 @@ int mi_close(register MI_INFO *info) error=my_errno; if (share->kfile >= 0) { - /* We must always flush the state with the current open_count. */ - if (share->mode != O_RDONLY) + /* + If we are crashed, we can safely flush the current state as it will + not change the crashed state. + We can NOT write the state in other cases as other threads + may be using the file at this point + */ + if (share->mode != O_RDONLY && mi_is_crashed(info)) mi_state_info_write(share->kfile, &share->state, 1); if (my_close(share->kfile,MYF(0))) error = my_errno; diff --git a/myisam/mi_locking.c b/myisam/mi_locking.c index 8a140a8b6fb..1efd203dc5f 100644 --- a/myisam/mi_locking.c +++ b/myisam/mi_locking.c @@ -19,16 +19,26 @@ reads info from a isam-table. Must be first request before doing any furter calls to any isamfunktion. Is used to allow many process use the same isamdatabase. - */ +*/ + +/* + state.open_count in the .MYI file is used the following way: + - For the first change of the file in this process it's incremented with + mi_mark_file_change(). (We have a write lock on the file in this case) + - In mi_close() it's decremented by _mi_decrement_open_count() if it + was incremented in the same process. + + This mean that if we are the only process using the file, the open_count + tells us if the MYISAM file wasn't properly closed. (This is true if + my_disable_locking is set). +*/ + #include "myisamdef.h" #ifdef __WIN__ #include <errno.h> #endif -static int mi_unlock_open_count(MI_INFO *info, my_bool write_info); - - /* lock table by F_UNLCK, F_RDLCK or F_WRLCK */ int mi_lock_database(MI_INFO *info, int lock_type) @@ -38,17 +48,17 @@ int mi_lock_database(MI_INFO *info, int lock_type) MYISAM_SHARE *share=info->s; uint flag; DBUG_ENTER("mi_lock_database"); - DBUG_PRINT("enter",("mi_lock_database: lock_type %d, old lock %d" - ", r_locks %u, w_locks %u", lock_type, - info->lock_type, share->r_locks, share->w_locks)); - DBUG_PRINT("enter",("mi_lock_database: gl._changed %d, open_count %u '%s'", + DBUG_PRINT("enter",("lock_type: %d old lock %d r_locks: %u w_locks: %u " + "global_changed: %d open_count: %u name: '%s'", + lock_type, info->lock_type, share->r_locks, + share->w_locks, share->global_changed, share->state.open_count, share->index_file_name)); if (share->options & HA_OPTION_READ_ONLY_DATA || info->lock_type == lock_type) DBUG_RETURN(0); - if (lock_type == F_EXTRA_LCK) + if (lock_type == F_EXTRA_LCK) /* Used by TMP tables */ { ++share->w_locks; ++share->tot_locks; @@ -90,8 +100,7 @@ int mi_lock_database(MI_INFO *info, int lock_type) share->state.process= share->last_process=share->this_process; share->state.unique= info->last_unique= info->this_unique; share->state.update_count= info->last_loop= ++info->this_loop; - if (mi_unlock_open_count(info, FALSE) || - mi_state_info_write(share->kfile, &share->state, 1)) + if (mi_state_info_write(share->kfile, &share->state, 1)) error=my_errno; share->changed=0; if (myisam_flush) @@ -106,19 +115,6 @@ int mi_lock_database(MI_INFO *info, int lock_type) if (error) mi_mark_crashed(info); } - else - { - /* - There are chances that _mi_mark_file_changed() has been called, - while share->changed remained FALSE. Consequently, we need to - clear the open_count even when share->changed is FALSE. Note, - that mi_unlock_open_count() will only clear the open_count when - it is set and only write the status to file, if it changes it - and we are running --with-external-locking. - */ - if (mi_unlock_open_count(info, ! my_disable_locking)) - error= my_errno; - } if (info->lock_type != F_EXTRA_LCK) { if (share->r_locks) @@ -142,16 +138,17 @@ int mi_lock_database(MI_INFO *info, int lock_type) break; case F_RDLCK: if (info->lock_type == F_WRLCK) - { /* Change RW to READONLY */ + { /* + Change RW to READONLY + mysqld does not turn write locks to read locks, so we're never here in mysqld. */ if (share->w_locks == 1) { flag=1; - if (mi_unlock_open_count(info, ! my_disable_locking) || - my_lock(share->kfile,lock_type,0L,F_TO_EOF, + if (my_lock(share->kfile,lock_type,0L,F_TO_EOF, MYF(MY_SEEK_NOT_DONE))) { error=my_errno; @@ -179,14 +176,6 @@ int mi_lock_database(MI_INFO *info, int lock_type) my_errno=error; break; } - if (share->state.open_count) - { - DBUG_PRINT("error",("RD: Table has not been correctly unlocked" - ": open_count %d '%s'", - share->state.open_count, - share->index_file_name)); - mi_mark_crashed(info); - } } VOID(_mi_test_if_changed(info)); share->r_locks++; @@ -232,14 +221,6 @@ int mi_lock_database(MI_INFO *info, int lock_type) my_errno=error; break; } - if (share->state.open_count) - { - DBUG_PRINT("error",("WR: Table has not been correctly unlocked" - ": open_count %d '%s'", - share->state.open_count, - share->index_file_name)); - mi_mark_crashed(info); - } } } } @@ -375,9 +356,10 @@ int _mi_readinfo(register MI_INFO *info, int lock_type, int check_keybuffer) } /* _mi_readinfo */ - /* Every isam-function that uppdates the isam-database must! end */ - /* with this request */ - /* ARGSUSED */ +/* + Every isam-function that uppdates the isam-database MUST end with this + request +*/ int _mi_writeinfo(register MI_INFO *info, uint operation) { @@ -450,6 +432,8 @@ int _mi_mark_file_changed(MI_INFO *info) { char buff[3]; register MYISAM_SHARE *share=info->s; + DBUG_ENTER("_mi_mark_file_changed"); + if (!(share->state.changed & STATE_CHANGED) || ! share->global_changed) { share->state.changed|=(STATE_CHANGED | STATE_NOT_ANALYZED | @@ -463,12 +447,12 @@ int _mi_mark_file_changed(MI_INFO *info) { mi_int2store(buff,share->state.open_count); buff[2]=1; /* Mark that it's changed */ - return (my_pwrite(share->kfile,buff,sizeof(buff), - sizeof(share->state.header), - MYF(MY_NABP))); + DBUG_RETURN(my_pwrite(share->kfile,buff,sizeof(buff), + sizeof(share->state.header), + MYF(MY_NABP))); } } - return 0; + DBUG_RETURN(0); } @@ -501,36 +485,3 @@ int _mi_decrement_open_count(MI_INFO *info) } return test(lock_error || write_error); } - -/* - Decrement open_count in preparation for unlock. - - SYNOPSIS - mi_unlock_open_count() - info Pointer to the MI_INFO structure. - write_info If info must be written when changed. - - RETURN - 0 OK -*/ - -static int mi_unlock_open_count(MI_INFO *info, my_bool write_info) -{ - int rc= 0; - MYISAM_SHARE *share=info->s; - - DBUG_ENTER("mi_unlock_open_count"); - DBUG_PRINT("enter",("mi_unlock_open_count: gl._changed %d open_count %d '%s'", - share->global_changed, share->state.open_count, - share->index_file_name)); - if (share->global_changed) - { - share->global_changed= 0; - if (share->state.open_count) - share->state.open_count--; - if (write_info) - rc= _mi_writeinfo(info, WRITEINFO_UPDATE_KEYFILE); - } - DBUG_RETURN(rc); -} - diff --git a/sql/ha_myisam.cc b/sql/ha_myisam.cc index 2b7b8f436b1..3bfee7cdd79 100644 --- a/sql/ha_myisam.cc +++ b/sql/ha_myisam.cc @@ -998,32 +998,14 @@ int ha_myisam::delete_table(const char *name) return mi_delete_table(name); } + int ha_myisam::external_lock(THD *thd, int lock_type) { - int rc; - - while ((! (rc= mi_lock_database(file, !table->tmp_table ? - lock_type : ((lock_type == F_UNLCK) ? - F_UNLCK : F_EXTRA_LCK)))) && - mi_is_crashed(file) && (myisam_recover_options != HA_RECOVER_NONE)) - { - /* - check_and_repair() implicitly write locks the table, unless a - LOCK TABLES is in effect. It should be safer to always write lock here. - The implicit lock by check_and_repair() will then be a no-op. - check_and_repair() does not restore the original lock, but unlocks the - table. So we have to get the requested lock type again. And then to - check, if the table has been crashed again meanwhile by another server. - If anything fails, we break. - */ - if (((lock_type != F_WRLCK) && (rc= mi_lock_database(file, F_WRLCK))) || - (rc= check_and_repair(thd))) - break; - } - return rc; + return mi_lock_database(file, !table->tmp_table ? + lock_type : ((lock_type == F_UNLCK) ? + F_UNLCK : F_EXTRA_LCK)); } - THR_LOCK_DATA **ha_myisam::store_lock(THD *thd, THR_LOCK_DATA **to, enum thr_lock_type lock_type) diff --git a/sql/handler.cc b/sql/handler.cc index 9eb129fab45..65078a485c5 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -348,7 +348,7 @@ int ha_commit_trans(THD *thd, THD_TRANS* trans) if (trans == &thd->transaction.all && my_b_tell(&thd->transaction.trans_log)) { - if (error= wait_if_global_read_lock(thd, 0, 0)) + if ((error= wait_if_global_read_lock(thd, 0, 0))) { /* Note that ROLLBACK [TO SAVEPOINT] does not have this test; it's diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 58d0fe9a7fa..6e782f81ae5 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -469,7 +469,7 @@ static ulong get_sort(uint count,...) uint chars= 0; uint wild_pos= 0; /* first wildcard position */ - if (start= str) + if ((start= str)) { for (; *str ; str++) { diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 0dd5c65bf79..6561af9c841 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -2209,11 +2209,7 @@ copy_data_between_tables(TABLE *from,TABLE *to, DBUG_RETURN(-1); /* purecov: inspected */ if (to->file->external_lock(thd, F_WRLCK)) - { - /* We must always unlock, even when lock failed. */ - (void) to->file->external_lock(thd, F_UNLCK); DBUG_RETURN(-1); - } to->file->extra(HA_EXTRA_WRITE_CACHE); from->file->info(HA_STATUS_VARIABLE); to->file->deactivate_non_unique_index(from->file->records); @@ -2313,11 +2309,11 @@ copy_data_between_tables(TABLE *from,TABLE *to, error=1; if (ha_commit(thd)) error=1; + err: free_io_cache(from); *copied= found_count; *deleted=delete_count; - /* we must always unlock the table on return. */ if (to->file->external_lock(thd,F_UNLCK)) error=1; DBUG_RETURN(error > 0 ? -1 : 0); diff --git a/sql/sql_update.cc b/sql/sql_update.cc index a17742df03b..cdcc90e8651 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -386,6 +386,24 @@ int mysql_update(THD *thd, Update multiple tables from join ***************************************************************************/ +/* + Get table map for list of Item_field +*/ + +static table_map get_table_map(List<Item> *items) +{ + List_iterator_fast<Item> item_it(*items); + Item_field *item; + table_map map= 0; + + while ((item= (Item_field *) item_it++)) + map|= item->used_tables(); + DBUG_PRINT("info",("table_map: 0x%08x", map)); + return map; +} + + + /* Setup multi-update handling and call SELECT to do the join */ @@ -401,59 +419,45 @@ int mysql_multi_update(THD *thd, int res; multi_update *result; TABLE_LIST *tl; - const bool locked= !(thd->locked_tables); + const bool using_lock_tables= thd->locked_tables != 0; DBUG_ENTER("mysql_multi_update"); + thd->select_limit= HA_POS_ERROR; + for (;;) { - table_map update_map= 0; - int tnr= 0; + table_map update_map; + int tnr; if ((res= open_tables(thd, table_list))) DBUG_RETURN(res); - /* - Only need to call lock_tables if (thd->locked_tables == NULL) - */ - if (locked && ((res= lock_tables(thd, table_list)))) + /* Only need to call lock_tables if we are not using LOCK TABLES */ + if (!using_lock_tables && ((res= lock_tables(thd, table_list)))) DBUG_RETURN(res); - thd->select_limit=HA_POS_ERROR; - /* Ensure that we have update privilege for all tables and columns in the SET part While we are here, initialize the table->map field. */ - for (tl= table_list ; tl ; tl=tl->next) + for (tl= table_list,tnr=0 ; tl ; tl=tl->next) { TABLE *table= tl->table; table->grant.want_privilege= (UPDATE_ACL & ~table->grant.privilege); table->map= (table_map) 1 << (tnr++); } - if (!setup_fields(thd, table_list, *fields, 1, 0, 0)) - { - List_iterator_fast<Item> field_it(*fields); - Item_field *item; - - while ((item= (Item_field *) field_it++)) - update_map|= item->used_tables(); - - DBUG_PRINT("info",("update_map=0x%08x", update_map)); - } - else + if (setup_fields(thd, table_list, *fields, 1, 0, 0)) DBUG_RETURN(-1); - /* - Unlock the tables in preparation for relocking - */ - if (locked) + update_map= get_table_map(fields); + + /* Unlock the tables in preparation for relocking */ + if (!using_lock_tables) { - pthread_mutex_lock(&LOCK_open); mysql_unlock_tables(thd, thd->lock); thd->lock= 0; - pthread_mutex_unlock(&LOCK_open); } /* @@ -474,26 +478,48 @@ int mysql_multi_update(THD *thd, tl->lock_type= TL_READ; tl->updating= 0; } - if (locked) + if (!using_lock_tables) tl->table->reginfo.lock_type= tl->lock_type; } + /* Relock the tables with the correct modes */ + res= lock_tables(thd,table_list); + if (using_lock_tables) + { + if (res) + DBUG_RETURN(res); + break; // Don't have to do setup_field() + } + /* - Relock the tables + We must setup fields again as the file may have been reopened + during lock_tables */ - if (!(res=lock_tables(thd,table_list))) - break; - - if (!locked) - DBUG_RETURN(res); - List_iterator_fast<Item> field_it(*fields); - Item_field *item; + { + List_iterator_fast<Item> field_it(*fields); + Item_field *item; - while ((item= (Item_field *) field_it++)) - /* item->cleanup(); XXX Use this instead in MySQL 4.1+ */ - item->field= item->result_field= 0; + while ((item= (Item_field *) field_it++)) +#if MYSQL_VERSION < 40100 + item->field= item->result_field= 0; +#else + item->cleanup(); +#endif + } + if (setup_fields(thd, table_list, *fields, 1, 0, 0)) + DBUG_RETURN(-1); + /* + If lock succeded and the table map didn't change since the above lock + we can continue. + */ + if (!res && update_map == get_table_map(fields)) + break; + /* + There was some very unexpected changes in the table definition between + open tables and lock tables. Close tables and try again. + */ close_thread_tables(thd); } @@ -548,7 +574,7 @@ int multi_update::prepare(List<Item> ¬_used_values) { TABLE_LIST *table_ref; SQL_LIST update; - table_map tables_to_update= 0; + table_map tables_to_update; Item_field *item; List_iterator_fast<Item> field_it(*fields); List_iterator_fast<Item> value_it(*values); @@ -559,8 +585,7 @@ int multi_update::prepare(List<Item> ¬_used_values) thd->cuted_fields=0L; thd->proc_info="updating main table"; - while ((item= (Item_field *) field_it++)) - tables_to_update|= item->used_tables(); + tables_to_update= get_table_map(fields); if (!tables_to_update) { @@ -624,7 +649,6 @@ int multi_update::prepare(List<Item> ¬_used_values) /* Split fields into fields_for_table[] and values_by_table[] */ - field_it.rewind(); while ((item= (Item_field *) field_it++)) { Item *value= value_it++; -- 2.30.9