review of bug37737

[Michael Widenius]

Hi!

Here is a review of the bug37737.diff file
37737: Maria: ma_test_loghandler_multiflush-t fails

=== modified file 'storage/maria/ma_loghandler.c'
--- storage/maria/ma_loghandler.c	2008-07-09 09:02:27 +0000
+++ storage/maria/ma_loghandler.c	2008-07-22 23:13:27 +0000

<cut>

+/*
+  @brief Waits previous buffer flush finish
+
+  @param buffer          buffer for check
+
+  @retval 0 previous buffer flushed and this thread have to flush this one
+  @retval 1 previous buffer flushed and this buffer flushed by other thread too
+*/
+
+my_bool translog_prev_buffer_flush_wait(struct st_translog_buffer *buffer)
+{
+  TRANSLOG_ADDRESS offset= buffer->offset;
+  TRANSLOG_FILE *file= buffer->file;
+  uint8 ver= buffer->ver;
+  DBUG_ENTER("translog_prev_buffer_flush_wait");
+  DBUG_PRINT("enter", ("buffer: 0x%lx  #%u  offset: (%lu,0x%lx)  "
+                       "prev sent: (%lu,0x%lx) prev offset: (%lu,0x%lx)",
+                       (ulong) buffer, (uint) buffer->buffer_no,
+                       LSN_IN_PARTS(buffer->offset),
+                       LSN_IN_PARTS(buffer->prev_sent_to_disk),
+                       LSN_IN_PARTS(buffer->prev_buffer_offset)));
+  translog_buffer_lock_assert_owner(buffer);
+  /*
+    if prev_sent_to_disk == LSN_IMPOSSIBLE then
+    prev_buffer_offset should be LSN_IMPOSSIBLE
+    because it means that this buffer was never used
+  */
+  DBUG_ASSERT((buffer->prev_sent_to_disk == LSN_IMPOSSIBLE &&
+               buffer->prev_buffer_offset == LSN_IMPOSSIBLE) ||
+              buffer->prev_sent_to_disk != LSN_IMPOSSIBLE);

A shorter version is:

 DBUG_ASSERT((buffer->prev_sent_to_disk == buffer->prev_buffer_offset ||
             buffer->prev_sent_to_disk != LSN_IMPOSSIBLE);

(Not critical, just maybe intersting way to rewrite it)

I don't however see how this criteria is enforced.
In translog_buffer_next() you do:

   new_buffer->prev_buffer_offset=
     log_descriptor.buffers[old_buffer_no].offset;

But you don't set prev_sent_to_disk in the same function. Is there
something that gurantees that prev_sent_to_disk isn't LSN_IMPOSSIBLE
when translog_buffer_next() is called ?

+  if (buffer->prev_buffer_offset != buffer->prev_sent_to_disk)
+  {
+    do {
+      pthread_cond_wait(&buffer->prev_sent_to_disk_cond, &buffer->mutex);
+      if (buffer->file != file || buffer->offset != offset ||
+          buffer->ver != ver)

Isn't it best to test 'ver' first as this should be different in
almost all cases if the buffer was flushed?
(buffer->file is likely to not change for most flushes)

Please add a comment why we are waiting for buffer->file to change
when we are waiting for the previous buffer to be flushed, not this buffer.

+      {
+        translog_buffer_unlock(buffer);
+        DBUG_RETURN(1); /* some the thread flushed the buffer already */
+      }
+    } while(buffer->prev_buffer_offset != buffer->prev_sent_to_disk);
+  }
+  DBUG_RETURN(0);
+}
+
 
 /*
   Flush given buffer
@@ -2460,39 +2522,8 @@ static my_bool translog_buffer_flush(str
   if (buffer->file != file || buffer->offset != offset || buffer->ver != ver)
     DBUG_RETURN(0); /* some the thread flushed the buffer already */

Same thing here; Better to test 'ver' first ?
 
<cut>

@@ -2553,10 +2584,34 @@ static my_bool translog_buffer_flush(str
   file->is_sync= 0;
 
   if (LSN_OFFSET(buffer->last_lsn) != 0)    /* if buffer->last_lsn is set */
-    translog_set_sent_to_disk(buffer->last_lsn,
-                              buffer->next_buffer_offset);
+  {
+    if (translog_prev_buffer_flush_wait(buffer))
+      DBUG_RETURN(0); /* some the thread flushed the buffer already */
+    translog_set_sent_to_disk(buffer);
+  }
   else
     translog_set_only_in_buffers(buffer->next_buffer_offset);

Why don't we flush the previous buffer if last_lsn is not set ?
I can understand why we don't have to flush the current buffer if lsn
is not set (we don't have anything complete to flush).

+
+  /* say to next buffer that we are finished */
+  {
+    struct st_translog_buffer *next_buffer=

Move variable to start of function so that you can remove the extra
indentation level.

+      log_descriptor.buffers + ((buffer->buffer_no + 1) % TRANSLOG_BUFFERS_NO);
+    if (likely(translog_status == TRANSLOG_OK)){

Move { to separate line

+      translog_buffer_lock(next_buffer);
+      next_buffer->prev_sent_to_disk= buffer->offset;
+      translog_buffer_unlock(next_buffer);
+      pthread_cond_broadcast(&next_buffer->prev_sent_to_disk_cond);

<cut>

Understood about 90 % of the code, but looks good from what I can see.
Please add comments to make the above things clear in the code.

Regards,
Monty