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.

src/server/database/Database/Field.cpp

@@ -30,18 +30,11 @@ Field::~Field()
     CleanUp();
 }
 
-void Field::SetByteValue(const void* newValue, const size_t newSize, enum_field_types newType, uint32 length)
+void Field::SetByteValue(void* newValue, enum_field_types newType, uint32 length)
 {
-    if (data.value)
-        CleanUp();
-
     // This value stores raw bytes that have to be explicitly cast later
-    if (newValue)
-    {
-        data.value = new char[newSize];
-        memcpy(data.value, newValue, newSize);
-        data.length = length;
-    }
+    data.value = newValue;
+    data.length = length;
     data.type = newType;
     data.raw = true;
 }

src/server/database/Database/Field.h

@@ -29,6 +29,8 @@ class Field
     friend class PreparedResultSet;
 
     public:
+        Field();
+        ~Field();
 
         bool GetBool() const // Wrapper, actually gets integer
         {
@@ -43,7 +45,8 @@ class Field
             #ifdef TRINITY_DEBUG
             if (!IsType(MYSQL_TYPE_TINY))
             {
-                TC_LOG_WARN("sql.sql", "Warning: GetUInt8() on non-tinyint field. Using type: %s.", FieldTypeToString(data.type));
+                TC_LOG_WARN("sql.sql", "Warning: GetUInt8() on non-tinyint field %s.%s (%s.%s) at index %u. Using type: %s.",
+                    meta.TableAlias, meta.Alias, meta.TableName, meta.Name, meta.Index, meta.Type);
                 return 0;
             }
             #endif
@@ -61,7 +64,8 @@ class Field
             #ifdef TRINITY_DEBUG
             if (!IsType(MYSQL_TYPE_TINY))
             {
-                TC_LOG_WARN("sql.sql", "Warning: GetInt8() on non-tinyint field. Using type: %s.", FieldTypeToString(data.type));
+                TC_LOG_WARN("sql.sql", "Warning: GetInt8() on non-tinyint field %s.%s (%s.%s) at index %u. Using type: %s.",
+                    meta.TableAlias, meta.Alias, meta.TableName, meta.Name, meta.Index, meta.Type);
                 return 0;
             }
             #endif
@@ -79,7 +83,8 @@ class Field
             #ifdef TRINITY_DEBUG
             if (!IsType(MYSQL_TYPE_SHORT) && !IsType(MYSQL_TYPE_YEAR))
             {
-                TC_LOG_WARN("sql.sql", "Warning: GetUInt16() on non-smallint field. Using type: %s.", FieldTypeToString(data.type));
+                TC_LOG_WARN("sql.sql", "Warning: GetUInt16() on non-smallint field %s.%s (%s.%s) at index %u. Using type: %s.",
+                    meta.TableAlias, meta.Alias, meta.TableName, meta.Name, meta.Index, meta.Type);
                 return 0;
             }
             #endif
@@ -97,7 +102,8 @@ class Field
             #ifdef TRINITY_DEBUG
             if (!IsType(MYSQL_TYPE_SHORT) && !IsType(MYSQL_TYPE_YEAR))
             {
-                TC_LOG_WARN("sql.sql", "Warning: GetInt16() on non-smallint field. Using type: %s.", FieldTypeToString(data.type));
+                TC_LOG_WARN("sql.sql", "Warning: GetInt16() on non-smallint field %s.%s (%s.%s) at index %u. Using type: %s.",
+                    meta.TableAlias, meta.Alias, meta.TableName, meta.Name, meta.Index, meta.Type);
                 return 0;
             }
             #endif
@@ -115,7 +121,8 @@ class Field
             #ifdef TRINITY_DEBUG
             if (!IsType(MYSQL_TYPE_INT24) && !IsType(MYSQL_TYPE_LONG))
             {
-                TC_LOG_WARN("sql.sql", "Warning: GetUInt32() on non-(medium)int field. Using type: %s.", FieldTypeToString(data.type));
+                TC_LOG_WARN("sql.sql", "Warning: GetUInt32() on non-(medium)int field %s.%s (%s.%s) at index %u. Using type: %s.",
+                    meta.TableAlias, meta.Alias, meta.TableName, meta.Name, meta.Index, meta.Type);
                 return 0;
             }
             #endif
@@ -133,7 +140,8 @@ class Field
             #ifdef TRINITY_DEBUG
             if (!IsType(MYSQL_TYPE_INT24) && !IsType(MYSQL_TYPE_LONG))
             {
-                TC_LOG_WARN("sql.sql", "Warning: GetInt32() on non-(medium)int field. Using type: %s.", FieldTypeToString(data.type));
+                TC_LOG_WARN("sql.sql", "Warning: GetInt32() on non-(medium)int field %s.%s (%s.%s) at index %u. Using type: %s.",
+                    meta.TableAlias, meta.Alias, meta.TableName, meta.Name, meta.Index, meta.Type);
                 return 0;
             }
             #endif
@@ -151,7 +159,8 @@ class Field
             #ifdef TRINITY_DEBUG
             if (!IsType(MYSQL_TYPE_LONGLONG) && !IsType(MYSQL_TYPE_BIT))
             {
-                TC_LOG_WARN("sql.sql", "Warning: GetUInt64() on non-bigint field. Using type: %s.", FieldTypeToString(data.type));
+                TC_LOG_WARN("sql.sql", "Warning: GetUInt64() on non-bigint field %s.%s (%s.%s) at index %u. Using type: %s.",
+                    meta.TableAlias, meta.Alias, meta.TableName, meta.Name, meta.Index, meta.Type);
                 return 0;
             }
             #endif
@@ -169,7 +178,8 @@ class Field
             #ifdef TRINITY_DEBUG
             if (!IsType(MYSQL_TYPE_LONGLONG) && !IsType(MYSQL_TYPE_BIT))
             {
-                TC_LOG_WARN("sql.sql", "Warning: GetInt64() on non-bigint field. Using type: %s.", FieldTypeToString(data.type));
+                TC_LOG_WARN("sql.sql", "Warning: GetInt64() on non-bigint field %s.%s (%s.%s) at index %u. Using type: %s.",
+                    meta.TableAlias, meta.Alias, meta.TableName, meta.Name, meta.Index, meta.Type);
                 return 0;
             }
             #endif
@@ -187,7 +197,8 @@ class Field
             #ifdef TRINITY_DEBUG
             if (!IsType(MYSQL_TYPE_FLOAT))
             {
-                TC_LOG_WARN("sql.sql", "Warning: GetFloat() on non-float field. Using type: %s.", FieldTypeToString(data.type));
+                TC_LOG_WARN("sql.sql", "Warning: GetFloat() on non-float field %s.%s (%s.%s) at index %u. Using type: %s.",
+                    meta.TableAlias, meta.Alias, meta.TableName, meta.Name, meta.Index, meta.Type);
                 return 0.0f;
             }
             #endif
@@ -205,7 +216,8 @@ class Field
             #ifdef TRINITY_DEBUG
             if (!IsType(MYSQL_TYPE_DOUBLE))
             {
-                TC_LOG_WARN("sql.sql", "Warning: GetDouble() on non-double field. Using type: %s.", FieldTypeToString(data.type));
+                TC_LOG_WARN("sql.sql", "Warning: GetDouble() on non-double field %s.%s (%s.%s) at index %u. Using type: %s.",
+                    meta.TableAlias, meta.Alias, meta.TableName, meta.Name, meta.Index, meta.Type);
                 return 0.0f;
             }
             #endif
@@ -223,7 +235,8 @@ class Field
             #ifdef TRINITY_DEBUG
             if (IsNumeric())
             {
-                TC_LOG_WARN("sql.sql", "Error: GetCString() on numeric field. Using type: %s.", FieldTypeToString(data.type));
+                TC_LOG_WARN("sql.sql", "Error: GetCString() on numeric field %s.%s (%s.%s) at index %u. Using type: %s.",
+                    meta.TableAlias, meta.Alias, meta.TableName, meta.Name, meta.Index, meta.Type);
                 return NULL;
             }
             #endif
@@ -235,16 +248,11 @@ class Field
             if (!data.value)
                 return "";
 
-            if (data.raw)
-            {
-                char const* string = GetCString();
-                if (!string)
-                    return "";
+            char const* string = GetCString();
+            if (!string)
+                return "";
 
-                return std::string(string, data.length);
-            }
-
-            return std::string((char*)data.value, data.length);
+            return std::string(string, data.length);
         }
 
         std::vector<uint8> GetBinary() const
@@ -263,10 +271,17 @@ class Field
             return data.value == NULL;
         }
 
-    protected:
-        Field();
-        ~Field();
+        struct Metadata
+        {
+            char const* TableName;
+            char const* TableAlias;
+            char const* Name;
+            char const* Alias;
+            char const* Type;
+            uint32 Index;
+        };
 
+    protected:
         #pragma pack(push, 1)
         struct
         {
@@ -277,12 +292,14 @@ class Field
          } data;
         #pragma pack(pop)
 
-        void SetByteValue(void const* newValue, size_t const newSize, enum_field_types newType, uint32 length);
+        void SetByteValue(void* newValue, enum_field_types newType, uint32 length);
         void SetStructuredValue(char* newValue, enum_field_types newType, uint32 length);
 
         void CleanUp()
         {
-            delete[] ((char*)data.value);
+            // Field does not own the data if fetched with prepared statement
+            if (!data.raw)
+                delete[] ((char*)data.value);
             data.value = NULL;
         }
 
@@ -387,6 +404,19 @@ class Field
                 default:                     return "-Unknown-";
             }
         }
+
+        void SetMetadata(MYSQL_FIELD* field, uint32 fieldIndex)
+        {
+            meta.TableName = field->org_table;
+            meta.TableAlias = field->table;
+            meta.Name = field->org_name;
+            meta.Alias = field->name;
+            meta.Type = FieldTypeToString(field->type);
+            meta.Index = fieldIndex;
+        }
+
+        Metadata meta;
+
         #endif
 };
 

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;
     }
 
-    //- This is where we prepare the buffer based on metadata
-    uint32 i = 0;
-    MYSQL_FIELD* field = mysql_fetch_field(m_res);
-    while (field)
-    {
-        size_t size = Field::SizeForType(field);
+    m_rowCount = mysql_stmt_num_rows(m_stmt);
 
-        m_rBind[i].buffer_type = field->type;
-        m_rBind[i].buffer = malloc(size);
-        memset(m_rBind[i].buffer, 0, size);
+    //- This is where we prepare the buffer based on metadata
+    MYSQL_FIELD* field = mysql_fetch_fields(m_metadataResult);
+    for (uint32 i = 0; i < m_fieldCount; ++i)
+    {
+        size_t size = Field::SizeForType(&field[i]);
+
+        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);
+    if (m_rBind)
+    {
+        for (uint32 i = 0; i < m_fieldCount; ++i)
+        {
+            delete[]((char*)m_rBind[i].buffer);
+            m_rBind[i].buffer = nullptr;
+        }
 
-    delete[] m_rBind;
-}
-
-void PreparedResultSet::FreeBindBuffer()
-{
-    for (uint32 i = 0; i < m_fieldCount; ++i)
-        free (m_rBind[i].buffer);
+        delete[] m_rBind;
+        m_rBind = nullptr;
+    }
 }

src/server/database/Database/QueryResult.h

@@ -73,18 +73,18 @@ class PreparedResultSet
         Field* Fetch() const
         {
             ASSERT(m_rowPosition < m_rowCount);
-            return m_rows[uint32(m_rowPosition)];
+            return const_cast<Field*>(&m_rows[uint32(m_rowPosition) * m_fieldCount]);
         }
 
-        const Field & operator [] (uint32 index) const
+        Field const& operator[](uint32 index) const
         {
             ASSERT(m_rowPosition < m_rowCount);
             ASSERT(index < m_fieldCount);
-            return m_rows[uint32(m_rowPosition)][index];
+            return m_rows[uint32(m_rowPosition) * m_fieldCount + index];
         }
 
     protected:
-        std::vector<Field*> m_rows;
+        std::vector<Field> m_rows;
         uint64 m_rowCount;
         uint64 m_rowPosition;
         uint32 m_fieldCount;
@@ -92,12 +92,11 @@ class PreparedResultSet
     private:
         MYSQL_BIND* m_rBind;
         MYSQL_STMT* m_stmt;
-        MYSQL_RES* m_res;
+        MYSQL_RES* m_metadataResult;    ///< Field metadata, returned by mysql_stmt_result_metadata
 
         my_bool* m_isNull;
         unsigned long* m_length;
 
-        void FreeBindBuffer();
         void CleanUp();
         bool _NextRow();