From a7919a11fcb3a52246158aba3193d0128baff344 Mon Sep 17 00:00:00 2001 From: unknown Date: Sat, 18 Sep 2004 20:33:39 +0200 Subject: bug#2831 - --extenral-locking does not fully work with --myisam-recover. Changed the semantics of open_count so that it is decremented at every unlock (if it was incremented due to data changes). So it indicates a crash, if it is non-zero after a lock. The table will then be repaired. myisam/mi_close.c: bug#2831 - --extenral-locking does not fully work with --myisam-recover. To avoid flushing the open_count at every unlock, we need to do so at close at least. myisam/mi_locking.c: bug#2831 - --extenral-locking does not fully work with --myisam-recover. open_count is now decremented at unlock (from a writelock) with mi_unlock_open_count(). After every new lock the state is read from the index file and the open_count checked. If not zero, another server must have crashed, so the table is marked as crashed. In certain situations the decremented open_count mut be flushed to the index file. I tried to keep these extra flushes as seldom as possible. sql/ha_myisam.cc: bug#2831 - --extenral-locking does not fully work with --myisam-recover. Added code to repair the table, if it is marked crashed after successful locking and the --myisam-recover option is set. sql/sql_table.cc: This does not really belong to the bugfix for #2831. But it was detected during fixing the external locking. --- myisam/mi_locking.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 79 insertions(+), 4 deletions(-) (limited to 'myisam/mi_locking.c') diff --git a/myisam/mi_locking.c b/myisam/mi_locking.c index f3bfa8deb90..8a140a8b6fb 100644 --- a/myisam/mi_locking.c +++ b/myisam/mi_locking.c @@ -26,6 +26,9 @@ #include #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) @@ -35,7 +38,12 @@ int mi_lock_database(MI_INFO *info, int lock_type) MYISAM_SHARE *share=info->s; uint flag; DBUG_ENTER("mi_lock_database"); - DBUG_PRINT("info",("lock_type: %d", lock_type)); + 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'", + share->global_changed, share->state.open_count, + share->index_file_name)); if (share->options & HA_OPTION_READ_ONLY_DATA || info->lock_type == lock_type) @@ -54,7 +62,6 @@ int mi_lock_database(MI_INFO *info, int lock_type) { switch (lock_type) { case F_UNLCK: - DBUG_PRINT("info", ("old lock: %d", info->lock_type)); if (info->lock_type == F_RDLCK) count= --share->r_locks; else @@ -83,7 +90,8 @@ 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_state_info_write(share->kfile, &share->state, 1)) + if (mi_unlock_open_count(info, FALSE) || + mi_state_info_write(share->kfile, &share->state, 1)) error=my_errno; share->changed=0; if (myisam_flush) @@ -98,6 +106,19 @@ 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) @@ -122,10 +143,15 @@ int mi_lock_database(MI_INFO *info, int lock_type) case F_RDLCK: if (info->lock_type == F_WRLCK) { /* 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 (my_lock(share->kfile,lock_type,0L,F_TO_EOF, + if (mi_unlock_open_count(info, ! my_disable_locking) || + my_lock(share->kfile,lock_type,0L,F_TO_EOF, MYF(MY_SEEK_NOT_DONE))) { error=my_errno; @@ -153,6 +179,14 @@ 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++; @@ -198,6 +232,14 @@ 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); + } } } } @@ -459,3 +501,36 @@ 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); +} + -- cgit v1.2.1 From 0d76cb7ea4a20570342d51136a6da598fb553800 Mon Sep 17 00:00:00 2001 From: unknown Date: Wed, 6 Oct 2004 01:24:21 +0300 Subject: 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_locking.c | 117 +++++++++++++++------------------------------------- 1 file changed, 34 insertions(+), 83 deletions(-) (limited to 'myisam/mi_locking.c') 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 #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); -} - -- cgit v1.2.1 From fe46310640b081114f1d9f9be0e3f88f8fd7d926 Mon Sep 17 00:00:00 2001 From: unknown Date: Wed, 6 Oct 2004 17:20:39 +0300 Subject: Code cleanups while doing review of pushed code myisam/mi_locking.c: More comments sql/mysql_priv.h: Change mode to uint (as it's a bitmap) sql/sql_handler.cc: Change mode to uint (as it's a bitmap) Fixed DBUG_PRINT to use same format as other MySQL code --- myisam/mi_locking.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) (limited to 'myisam/mi_locking.c') diff --git a/myisam/mi_locking.c b/myisam/mi_locking.c index 1efd203dc5f..d5212cdee47 100644 --- a/myisam/mi_locking.c +++ b/myisam/mi_locking.c @@ -21,19 +21,6 @@ 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 @@ -426,7 +413,24 @@ int _mi_test_if_changed(register MI_INFO *info) } /* _mi_test_if_changed */ -/* Put a mark in the .MYI file that someone is updating the table */ +/* + Put a mark in the .MYI file that someone is updating the table + + + DOCUMENTATION + + state.open_count in the .MYI file is used the following way: + - For the first change of the .MYI file in this process open_count is + incremented by mi_mark_file_change(). (We have a write lock on the file + when this happens) + - 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). +*/ + int _mi_mark_file_changed(MI_INFO *info) { -- cgit v1.2.1