From: Date: November 12 2007 10:07am Subject: Re: bk commit into 4.1 tree (holyfoot:1.2688) BUG#31305 List-Archive: http://lists.mysql.com/commits/37562 Message-Id: <003b01c8250b$66863ac0$5815a8c0@BISONXP> MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset="ISO-8859-15"; reply-type=original Content-Transfer-Encoding: 8bit Hello, Ingo. > Please change MAX_HEADER_LEGNTH to MI_MAX_DYN_BLOCK_HEADER in the > comment. Please place a dot after the last sentence. .... > Please place a dot after the last sentence. .... > Please remove at least one of the empty lines. these were fixed. > Why do you use ABS() here and in other REPEAT() functions? Just copied it from the original bugcase. Removed to not confuse possible reviewers. > If you like to shorten the result file a bit ... --disable_query_log added >> +UPDATE t1 SET b=repeat('a', abs(800)); >> +CHECK TABLE t1 EXTENDED; >So you try an even bigger one. What does this statement show, what the >previous one didn't show? Previous statement was INSERT, and we needed bigger record size for UPDATE to make table full as it uses the old record's space as well. >Wouldn't it be great to show that a smaller record fits after a bigger >one was rejected? Added. > Is this the default in our tests? Won't it be better to save and restore > the old value? No need to store in fact - just 'default' value used. As i didn't modify the code, i assume 'ok to push' from you, though you welcome to take a look at the last patch, Regards HF ----- Original Message ----- From: "Ingo Strüwing" To: Cc: Sent: Sunday, November 11, 2007 7:22 PM Subject: Re: bk commit into 4.1 tree (holyfoot:1.2688) BUG#31305 > Hi Alexey, > > overall a good patch. It could even be better with some comments in the > code and perhaps even the test case. See below for some comments. Some > of them could be added to the code/testcase if you like. > > holyfoot@stripped, 10.11.2007 20:11: > ... >> ChangeSet@stripped, 2007-11-10 23:11:37+04:00, holyfoot@stripped +4 -0 >> Bug #31305 myisam tables crash when they are near capacity. >> >> When we insert a record into MYISAM table which is almost 'full', >> we first write record data in the free space inside a file, and then >> check if we have enough space after the end of the file. >> So if we don't have the space, table will left corrupted. >> Similar error also happens when we updata MYISAM tables. >> >> Fixed by modifying write_dynamic_record and update_dynamic_record >> functions >> to check for free space before writing parts of a record > ... >> diff -Nrup a/BitKeeper/etc/ignore b/BitKeeper/etc/ignore > ... >> +libmysql_r/client_settings.h >> +libmysqld/ha_blackhole.cc > > Unrelated, but acceptable. > >> diff -Nrup a/myisam/mi_dynrec.c b/myisam/mi_dynrec.c > ... >> + /* >> + Check if we have enough room for the new record. >> + First we do simplified check to make usual case faster. >> + Then we do more precise check for the space left. >> + Though it still is not absolutely precise, as >> + we always use MAX_HEADER_LEGNTH while it can be >> + less in the most of the cases >> + */ > > Yes, I agree with the approach. We might refuse to insert a bit too > early, but this is not dramatic, unless the table is very heavily > fragmented. But when the user optimizes the table, the check starts to > work precisely again. > > Please change MAX_HEADER_LEGNTH to MI_MAX_DYN_BLOCK_HEADER in the > comment. Please place a dot after the last sentence. > >> + >> + if (unlikely(info->s->base.max_data_file_length - >> + info->state->data_file_length < >> + reclength + MI_MAX_DYN_BLOCK_HEADER)) > > Yes, this is correct. > >> + { >> + if (info->s->base.max_data_file_length - >> info->state->data_file_length + >> + info->state->empty - info->state->del * MI_MAX_DYN_BLOCK_HEADER >> < >> + reclength + MI_MAX_DYN_BLOCK_HEADER) > > Ok. > > ... >> DBUG_ENTER("update_dynamic_record"); >> >> flag=block_info.second_read=0; >> + /* >> + Check if we have enough room for the record. >> + First we do simplified check to make usual case faster. >> + Then we do more precise check for the space left. >> + Though it still is not absolutely precise, as >> + we always use MI_MAX_DYN_BLOCK_HEADER while it can be >> + less in the most of the cases >> + */ > > Please place a dot after the last sentence. > >> + >> + if (unlikely(info->s->base.max_data_file_length - >> + info->state->data_file_length < reclength)) > > I understand that you do not subtract MI_MAX_DYN_BLOCK_HEADER here > because the old record does use a header already. > > In the first moment I was confused and thought the requirement for new > block headers may add to the space required in the "endspace". But then > I became aware that you use the full new record size for the check (you > do not subtract the old record size). > > Even if the new record is longer and needs to be extended into the > "endspace", enough bytes can be stored in the old blocks so that there > is room for a new block header in the "endspace". > > However, there is one problem left. If the "endspace" is < > MI_MIN_BLOCK_LENGTH, we cannot use that space at all. > > Since blocks are aligned at 4 bytes, and max_data_file_length should > also be aligned a 4, "endspace" < 20 means "endspace" <= 16. > > If the new record is <= 16 bytes long, it will always fit into a type 1 > block (1/01: Full small record, full block, 3 bytes) or a type 3 block > (3/03: Full small record, unused space, 4 bytes). (The block size is > always at least 20 bytes: MI_MIN_BLOCK_LENGTH.) > > So I'd say: good. This should be safe. > >> + { >> + if ((error=_mi_get_block_info(&block_info,info->dfile,filepos)) >> + & (BLOCK_DELETED | BLOCK_ERROR | BLOCK_SYNC_ERROR | >> BLOCK_FATAL_ERROR)) >> + { >> + DBUG_PRINT("error",("Got wrong block info")); >> + if (!(error & BLOCK_FATAL_ERROR)) >> + my_errno=HA_ERR_WRONG_IN_RECORD; >> + goto err; >> + } >> + >> + if (block_info.rec_len < reclength) > > If I understand the above correctly, you read the block header of the > first block of the old record to get at the length of the old record. > Only if the new record is longer, you need to do another check. Good. > >> + { >> + if (info->s->base.max_data_file_length - >> info->state->data_file_length + >> + info->state->empty - info->state->del * >> MI_MAX_DYN_BLOCK_HEADER < >> + reclength - block_info.rec_len + MI_MAX_DYN_BLOCK_HEADER) > > If I understand correctly, you assume that the new record might require > to create a new block in the "endspace". This is correct. > > You subtract the old record length from the new record length and > compensate by adding MI_MAX_DYN_BLOCK_HEADER. Good. > >> + { >> + my_errno=HA_ERR_RECORD_FILE_FULL; >> + goto err; >> + } >> + } >> + block_info.second_read=0; > > Good. This is important here. > > ... >> uint tmp=MY_ALIGN(reclength - length + 3 + >> test(reclength >= 65520L),MI_DYN_ALIGN_SIZE); >> + >> + > > If you feel that we should have an empty line here, that's ok. But > please do never have two empty lines in a row within a function. Please > remove at least one of the empty lines. > > ... >> diff -Nrup a/mysql-test/t/almost_full.test >> b/mysql-test/t/almost_full.test > ... >> +let $1= 303; >> +while ($1) >> +{ >> + INSERT INTO t1 SET b=repeat('a',abs(200)); > > Why do you use ABS() here and in other REPEAT() functions? > >> + dec $1; >> +} > > If you like to shorten the result file a bit, you could also loop over > INSERT...SELECT. Just a hint, I don't insist. > >> + >> +DELETE FROM t1 WHERE a=1 or a=5; > > Here you punch two holes in the table, I guess. > >> + >> +--error 1114 >> +INSERT INTO t1 SET b=repeat('a',abs(600)); >> +CHECK TABLE t1 EXTENDED; > > That record doesn't fit anymore. > >> + >> +--error 1114 >> +UPDATE t1 SET b=repeat('a', abs(800)); >> +CHECK TABLE t1 EXTENDED; > > So you try an even bigger one. What does this statement show, what the > previous one didn't show? > > Wouldn't it be great to show that a smaller record fits after a bigger > one was rejected? > >> + >> +drop table t1; >> +set global myisam_data_pointer_size=4; > > Is this the default in our tests? Won't it be better to save and restore > the old value? > > ... > > Ok to push from me, under the condition that you fix the places I asked > for using the word 'please'. :-) > > Please do also answer my questions. > > I don't demand that you add comments, but I would appreciate it. > > I don't need to take another look, unless you change things not > mentioned in this review. > > Regards > Ingo > -- > Ingo Strüwing, Senior Software Developer > MySQL GmbH, Dachauer Str. 37, D-80335 München > Geschäftsführer: Kaj Arnö - HRB München 162140 >