Core/DBLayer: Optimized prepared statement query results by eliminating unneeded buffer copies

* Improved error logs for using incorrect Field getters to also include table name, field name and field index.

1 files changed, 64 insertions, 50 deletions

diff --git a/src/server/database/Database/QueryResult.cpp b/src/server/database/Database/QueryResult.cpp
index 61a1a682705..66f1eb40eee 100644
--- a/src/server/database/Database/QueryResult.cpp
+++ b/src/server/database/Database/QueryResult.cpp
@@ -26,7 +26,10 @@ _result(result),
 _fields(fields)
 {
     _currentRow = new Field[_fieldCount];
-    ASSERT(_currentRow);
+#ifdef TRINITY_DEBUG
+    for (uint32 i = 0; i < _fieldCount; i++)
+        _currentRow[i].SetMetadata(&_fields[i], i);
+#endif
 }
 
 PreparedResultSet::PreparedResultSet(MYSQL_STMT* stmt, MYSQL_RES *result, uint64 rowCount, uint32 fieldCount) :
@@ -35,11 +38,11 @@ m_rowPosition(0),
 m_fieldCount(fieldCount),
 m_rBind(NULL),
 m_stmt(stmt),
-m_res(result),
+m_metadataResult(result),
 m_isNull(NULL),
 m_length(NULL)
 {
-    if (!m_res)
+    if (!m_metadataResult)
         return;
 
     if (m_stmt->bind_result_done)
@@ -66,50 +69,44 @@ m_length(NULL)
         return;
     }
 
+    m_rowCount = mysql_stmt_num_rows(m_stmt);
+
     //- This is where we prepare the buffer based on metadata
-    uint32 i = 0;
-    MYSQL_FIELD* field = mysql_fetch_field(m_res);
-    while (field)
+    MYSQL_FIELD* field = mysql_fetch_fields(m_metadataResult);
+    for (uint32 i = 0; i < m_fieldCount; ++i)
     {
-        size_t size = Field::SizeForType(field);
+        size_t size = Field::SizeForType(&field[i]);
 
-        m_rBind[i].buffer_type = field->type;
-        m_rBind[i].buffer = malloc(size);
-        memset(m_rBind[i].buffer, 0, size);
+        m_rBind[i].buffer_type = field[i].type;
+        m_rBind[i].buffer = new char[size * m_rowCount];
         m_rBind[i].buffer_length = size;
         m_rBind[i].length = &m_length[i];
         m_rBind[i].is_null = &m_isNull[i];
         m_rBind[i].error = NULL;
-        m_rBind[i].is_unsigned = field->flags & UNSIGNED_FLAG;
-
-        ++i;
-        field = mysql_fetch_field(m_res);
+        m_rBind[i].is_unsigned = field[i].flags & UNSIGNED_FLAG;
     }
 
     //- This is where we bind the bind the buffer to the statement
     if (mysql_stmt_bind_result(m_stmt, m_rBind))
     {
         TC_LOG_WARN("sql.sql", "%s:mysql_stmt_bind_result, cannot bind result from MySQL server. Error: %s", __FUNCTION__, mysql_stmt_error(m_stmt));
-        delete[] m_rBind;
+        mysql_stmt_free_result(m_stmt);
+        CleanUp();
         delete[] m_isNull;
         delete[] m_length;
         return;
     }
 
-    m_rowCount = mysql_stmt_num_rows(m_stmt);
-
-    m_rows.resize(uint32(m_rowCount));
+    m_rows.resize(uint32(m_rowCount) * m_fieldCount);
     while (_NextRow())
     {
-        m_rows[uint32(m_rowPosition)] = new Field[m_fieldCount];
-        for (uint64 fIndex = 0; fIndex < m_fieldCount; ++fIndex)
+        for (uint32 fIndex = 0; fIndex < m_fieldCount; ++fIndex)
         {
+            unsigned long buffer_length = m_rBind[fIndex].buffer_length;
+            unsigned long fetched_length = *m_rBind[fIndex].length;
             if (!*m_rBind[fIndex].is_null)
-                m_rows[uint32(m_rowPosition)][fIndex].SetByteValue(m_rBind[fIndex].buffer,
-                                                            m_rBind[fIndex].buffer_length,
-                                                            m_rBind[fIndex].buffer_type,
-                                                           *m_rBind[fIndex].length);
-            else
+            {
+                void* buffer = m_stmt->bind[fIndex].buffer;
                 switch (m_rBind[fIndex].buffer_type)
                 {
                     case MYSQL_TYPE_TINY_BLOB:
@@ -118,24 +115,42 @@ m_length(NULL)
                     case MYSQL_TYPE_BLOB:
                     case MYSQL_TYPE_STRING:
                     case MYSQL_TYPE_VAR_STRING:
-                    m_rows[uint32(m_rowPosition)][fIndex].SetByteValue("",
-                                                            m_rBind[fIndex].buffer_length,
-                                                            m_rBind[fIndex].buffer_type,
-                                                           *m_rBind[fIndex].length);
-                    break;
-                    default:
-                    m_rows[uint32(m_rowPosition)][fIndex].SetByteValue(nullptr,
-                                                            m_rBind[fIndex].buffer_length,
-                                                            m_rBind[fIndex].buffer_type,
-                                                           *m_rBind[fIndex].length);
+                        // warning - the string will not be null-terminated if there is no space for it in the buffer
+                        // when mysql_stmt_fetch returned MYSQL_DATA_TRUNCATED
+                        // we cannot blindly null-terminate the data either as it may be retrieved as binary blob and not specifically a string
+                        // in this case using Field::GetCString will result in garbage
+                        // TODO: remove Field::GetCString and use boost::string_ref (currently proposed for TS as string_view, maybe in C++17)
+                        if (fetched_length < buffer_length)
+                            *((char*)buffer + fetched_length) = '\0';
+                        break;
                 }
+
+                m_rows[uint32(m_rowPosition) * m_fieldCount + fIndex].SetByteValue(
+                    buffer,
+                    m_rBind[fIndex].buffer_type,
+                    fetched_length);
+
+                // move buffer pointer to next part
+                m_stmt->bind[fIndex].buffer = (char*)buffer + buffer_length;
+            }
+            else
+            {
+                m_rows[uint32(m_rowPosition) * m_fieldCount + fIndex].SetByteValue(
+                    nullptr,
+                    m_rBind[fIndex].buffer_type,
+                    *m_rBind[fIndex].length);
+            }
+
+#ifdef TRINITY_DEBUG
+            m_rows[uint32(m_rowPosition) * m_fieldCount + fIndex].SetMetadata(&field[fIndex], fIndex);
+#endif
         }
         m_rowPosition++;
     }
     m_rowPosition = 0;
 
     /// All data is buffered, let go of mysql c api structures
-    CleanUp();
+    mysql_stmt_free_result(m_stmt);
 }
 
 ResultSet::~ResultSet()
@@ -145,8 +160,7 @@ ResultSet::~ResultSet()
 
 PreparedResultSet::~PreparedResultSet()
 {
-    for (uint32 i = 0; i < uint32(m_rowCount); ++i)
-        delete[] m_rows[i];
+    CleanUp();
 }
 
 bool ResultSet::NextRow()
@@ -215,18 +229,18 @@ void ResultSet::CleanUp()
 
 void PreparedResultSet::CleanUp()
 {
-    /// More of the in our code allocated sources are deallocated by the poorly documented mysql c api
-    if (m_res)
-        mysql_free_result(m_res);
+    if (m_metadataResult)
+        mysql_free_result(m_metadataResult);
 
-    FreeBindBuffer();
-    mysql_stmt_free_result(m_stmt);
-
-    delete[] m_rBind;
-}
+    if (m_rBind)
+    {
+        for (uint32 i = 0; i < m_fieldCount; ++i)
+        {
+            delete[]((char*)m_rBind[i].buffer);
+            m_rBind[i].buffer = nullptr;
+        }
 
-void PreparedResultSet::FreeBindBuffer()
-{
-    for (uint32 i = 0; i < m_fieldCount; ++i)
-        free (m_rBind[i].buffer);
+        delete[] m_rBind;
+        m_rBind = nullptr;
+    }
 }