Merge lp:~hingo/drizzle/drizzle-execute-result_set-off-by-one into lp:~drizzle-trunk/drizzle/development

[Henrik Ingo]

Execute API has off by one error, there is always an empty row at the
end of each result set. This fixes it.

Sorry I have no test for this, I don't know who or what code uses this
in the server except for json_server, which has no tests currently.

Ref: http://www.flamingspork.com/blog/2011/04/21/http-json-alsosql-interface-to-drizzle/comment-page-1/#comment-102142

[David Shrewsbury (dshrews)]

Hi Henrik,

The slave plugin makes extensive use of the Execute API. We should verify that the replication tests still pass with these changes. Running it through param-build should check that.

-Dave

[Henrik Ingo (hingo)]

Thanks Shrews for enlightening me!

Of course, it would be even better to have a test somewhere that fails without this patch. But I'm glad to hear this area is covered by some tests, at least we should find out soon if I broke something or not.

[Henrik Ingo (hingo)]

Just to document it for our children and whatnot, Shrews pointed me to the code in slave plugin that is affected by this:
queue_consumer.cc:198

****************************************************************

  uint32_t found_rows= 0;
  while (result_set.next())
  {
    string msg= result_set.getString(0);
    string com_id= result_set.getString(1);
    string orig_server_uuid= result_set.getString(2);
    string orig_commit_id= result_set.getString(3);

    if ((msg == "") || (found_rows == 1))
      break;

    /* No columns should be NULL */
    assert(result_set.isNull(0) == false);
    assert(result_set.isNull(1) == false);
    assert(result_set.isNull(2) == false);
    assert(result_set.isNull(3) == false);

    google::protobuf::TextFormat::ParseFromString(msg, &transaction);

    commit_id= com_id;
    originating_server_uuid= orig_server_uuid;
    originating_commit_id= boost::lexical_cast<uint64_t>(orig_commit_id);
    found_rows++;
  }

  if (found_rows == 0)
    return false;

  return true;
****************************************************************

The code will continue to work properly because of:

  while (result_set.next())

The code currently works because it checks to omit the empty row at:
    if ((msg == "") || (found_rows == 1))
      break;

(It seems to me this is there specifically to work around this bug and could now be removed.)

[Mark Atwood (fallenpegasus)]

Turns out it does fail in a test

http://jenkins.drizzle.org/view/Drizzle-build/job/drizzle-build-ubuntu10.10-32bit/1151/console

json_server.basic [ fail ]
--- ../plugin/json_server/tests/r/basic.result 2011-12-28 05:06:28.000000000 +0300
+++ /home/hudson/hudson/workspace/drizzle-build-ubuntu10.10-32bit/drizzle7-2011.11.29.2480/_build/tests/var/log/basic.reject 2011-12-28 05:29:35.000000000 +0300
@@ -3,9 +3,6 @@
 http_post("http://localhost:PORT/0.1/sql", 'select * from t1;')
 {
    "query" : "select * from t1;",
- "result_set" : [
- [ "", "" ]
- ],
    "sqlstate" : "00000"
 }

@@ -15,8 +12,7 @@
 {
    "query" : "select * from t1;",
    "result_set" : [
- [ "1", "from MySQL protocol" ],
- [ "", "" ]
+ [ "1", "from MySQL protocol" ]
    ],
    "sqlstate" : "00000"
 }

drizzletest: Result content mismatch

[Henrik Ingo (hingo)]

More than anything else I'm surprised and confused to realize json_server has a tests directory and I've not seen it until now :-) Apologies for proposing to merge before running make test myself, I don't usually do that, in this case my brain apparently had just really decided that there is nothing to test, which is never true...

Seems you realized already that this test is wrong and the empty line needs to be removed from result set.

=== modified file 'drizzled/plugin/client/cached.h'
--- drizzled/plugin/client/cached.h	2011-07-25 04:35:53 +0000
+++ drizzled/plugin/client/cached.h	2011-12-23 10:04:12 +0000
@@ -58,7 +58,8 @@
       max_column++;
     }
     _result_set->setColumnCount(max_column);
-    _result_set->createRow();
+    // Moved to checkRowBegin()
+    //_result_set->createRow();
   }
 
   virtual void sendError(drizzled::error_t error_code, const char *error_message)
@@ -66,14 +67,19 @@
     _result_set->pushException(sql::Exception(error_message, error_code));
   }
 
-  virtual void checkRowEnd()
+  virtual void checkRowBegin()
   {
-    if (++column % max_column == 0)
+    if (currentColumn() == 0)
     {
       _result_set->createRow();
     }
   }
 
+virtual void checkRowEnd()
+  {
+    column++;
+  }
+
   using Client::store;
 
   virtual void store(Field *from)
@@ -90,42 +96,49 @@
 
   virtual void store()
   {
+    checkRowBegin();
     _result_set->setColumnNull(currentColumn());
     checkRowEnd();
   }
 
   virtual void store(int32_t from)
   {
+    checkRowBegin();
     _result_set->setColumn(currentColumn(), boost::lexical_cast<std::string>(from));
     checkRowEnd();
   }
 
   virtual void store(uint32_t from)
   {
+    checkRowBegin();
     _result_set->setColumn(currentColumn(), boost::lexical_cast<std::string>(from));
     checkRowEnd();
   }
 
   virtual void store(int64_t from)
   {
+    checkRowBegin();
     _result_set->setColumn(currentColumn(), boost::lexical_cast<std::string>(from));
     checkRowEnd();
   }
 
   virtual void store(uint64_t from)
   {
+    checkRowBegin();
     _result_set->setColumn(currentColumn(), boost::lexical_cast<std::string>(from));
     checkRowEnd();
   }
 
   virtual void store(double from, uint32_t decimals, String *buffer)
   {
+    checkRowBegin();
     buffer->set_real(from, decimals, &my_charset_bin);
     return store(buffer->ptr(), buffer->length());
   }
 
   virtual void store(const char *from, size_t length)
   {
+    checkRowBegin();
     _result_set->setColumn(currentColumn(), std::string(from, length));
     checkRowEnd();
   }