MDEV-20865 extra2_fields refactored to Extra2_info class

read_extra2() is now Extra2_info::read()
Additional assertions for checking size consistency.

Extra2_info::write() is used by further MDEV-20865 development.

4 files changed, 191 insertions, 97 deletions

diff --git a/sql/datadict.cc b/sql/datadict.cc
index e85478a710c..decb947fb97 100644
--- a/sql/datadict.cc
+++ b/sql/datadict.cc
@@ -271,3 +271,138 @@ bool dd_recreate_table(THD *thd, const char *db, const char *table_name)
   DBUG_RETURN(ha_create_table(thd, path_buf, db, table_name, &create_info, 0, 0));
 }
 
+
+bool Extra2_info::read(const uchar *frm_image, size_t frm_size)
+{
+  read_size= uint2korr(frm_image + 4);
+
+  if (frm_size < FRM_HEADER_SIZE + read_size)
+    return true;
+
+  const uchar *pos= frm_image + 64;
+
+  DBUG_ENTER("Extra2_info::read");
+
+  if (*pos == '/')   // old frm had '/' there
+    DBUG_RETURN(false);
+
+  const uchar *e2end= pos + read_size;
+  while (pos + 3 <= e2end)
+  {
+    extra2_frm_value_type type= (extra2_frm_value_type)*pos++;
+    size_t length= extra2_read_len(&pos, e2end);
+    if (!length)
+      DBUG_RETURN(true);
+
+    bool fail= false;
+    switch (type) {
+      case EXTRA2_TABLEDEF_VERSION:
+        if (version.str) // see init_from_sql_statement_string()
+        {
+          if (length != version.length)
+            DBUG_RETURN(true);
+        }
+        else
+        {
+          version.str= pos;
+          version.length= length;
+        }
+        break;
+      case EXTRA2_ENGINE_TABLEOPTS:
+        fail= read_once(&options, pos, length);
+        break;
+      case EXTRA2_DEFAULT_PART_ENGINE:
+        engine.set((const char*)pos, length);
+        break;
+      case EXTRA2_GIS:
+        fail= read_once(&gis, pos, length);
+        break;
+      case EXTRA2_PERIOD_FOR_SYSTEM_TIME:
+        fail= read_once(&system_period, pos, length)
+          || length != 2 * frm_fieldno_size;
+        break;
+      case EXTRA2_FIELD_FLAGS:
+        fail= read_once(&field_flags, pos, length);
+        break;
+      case EXTRA2_APPLICATION_TIME_PERIOD:
+        fail= read_once(&application_period, pos, length);
+        break;
+      case EXTRA2_FIELD_DATA_TYPE_INFO:
+        fail= read_once(&field_data_type_info, pos, length);
+        break;
+      case EXTRA2_PERIOD_WITHOUT_OVERLAPS:
+        fail= read_once(&without_overlaps, pos, length);
+        break;
+      case EXTRA2_INDEX_FLAGS:
+        fail= read_once(&index_flags, pos, length);
+        break;
+      default:
+        /* abort frm parsing if it's an unknown but important extra2 value */
+        if (type >= EXTRA2_ENGINE_IMPORTANT)
+          DBUG_RETURN(true);
+    }
+    if (fail)
+      DBUG_RETURN(true);
+
+    pos+= length;
+  }
+  if (pos != e2end)
+    DBUG_RETURN(true);
+
+  DBUG_ASSERT(store_size() == read_size);
+  DBUG_RETURN(false);
+}
+
+
+/*
+  TODO: now this is used in by MDEV-20865 but this can be also used by
+  build_frm_image() (see TODO comment there).
+*/
+uchar *
+Extra2_info::write(uchar *frm_image, size_t frm_size)
+{
+  // FIXME: what to do with frm_size here (and in read())?
+  uchar *pos;
+  /* write the extra2 segment */
+  pos = frm_image + FRM_HEADER_SIZE;
+  compile_time_assert(EXTRA2_TABLEDEF_VERSION != '/');
+
+  if (version.str)
+    pos= extra2_write(pos, EXTRA2_TABLEDEF_VERSION, version);
+
+  if (engine.str)
+    pos= extra2_write(pos, EXTRA2_DEFAULT_PART_ENGINE, engine);
+
+  if (options.str)
+    pos= extra2_write(pos, EXTRA2_ENGINE_TABLEOPTS, options);
+
+  if (gis.str)
+    pos= extra2_write(pos, EXTRA2_GIS, gis);
+
+  if (field_data_type_info.str)
+    pos= extra2_write(pos, EXTRA2_FIELD_DATA_TYPE_INFO, field_data_type_info);
+
+  if (index_flags.str)
+    pos= extra2_write(pos, EXTRA2_INDEX_FLAGS, index_flags);
+
+  if (system_period.str)
+    pos= extra2_write(pos, EXTRA2_PERIOD_FOR_SYSTEM_TIME, system_period);
+
+  if (application_period.str)
+    pos= extra2_write(pos, EXTRA2_APPLICATION_TIME_PERIOD, application_period);
+
+  if (without_overlaps.str)
+    pos= extra2_write(pos, EXTRA2_PERIOD_WITHOUT_OVERLAPS, without_overlaps);
+
+  if (field_flags.str)
+    pos= extra2_write(pos, EXTRA2_FIELD_FLAGS, field_flags);
+
+  write_size= pos - frm_image - FRM_HEADER_SIZE;
+  DBUG_ASSERT(write_size == store_size());
+  DBUG_ASSERT(write_size <= 0xffff - FRM_HEADER_SIZE - 4);
+
+#if 0
+  int4store(pos, filepos); // end of the extra2 segment
+#endif
+  return pos;
+}
diff --git a/sql/datadict.h b/sql/datadict.h
index 47d43913dcd..9c1c151af1e 100644
--- a/sql/datadict.h
+++ b/sql/datadict.h
@@ -153,7 +153,7 @@ uchar *
 extra2_write_field_properties(uchar *pos, List<Create_field> &create_fields);
 
 
-struct extra2_fields
+struct Extra2_info
 {
   LEX_CUSTRING version;
   LEX_CUSTRING options;
@@ -165,8 +165,50 @@ struct extra2_fields
   LEX_CUSTRING field_data_type_info;
   LEX_CUSTRING without_overlaps;
   LEX_CUSTRING index_flags;
-  void reset()
-  { bzero((void*)this, sizeof(*this)); }
+
+  uint read_size;
+  size_t write_size;
+
+  Extra2_info()
+  {
+    bzero((void*)this, sizeof(*this));
+  }
+
+  template <class S>
+  size_t store_size(const S &f) const
+  {
+    if (f.length == 0)
+      return 0;
+    DBUG_ASSERT(f.length <= 65535);
+    // 1 byte is for type; 1 or 3 bytes for length
+    return f.length + (f.length <= 255 ? 2 : 4);
+  }
+  size_t store_size() const
+  {
+    return
+      store_size(version) +
+      store_size(options) +
+      store_size(engine) +
+      store_size(gis) +
+      store_size(field_flags) +
+      store_size(system_period) +
+      store_size(application_period) +
+      store_size(field_data_type_info) +
+      store_size(without_overlaps) +
+      store_size(index_flags);
+  }
+
+  bool read_once(LEX_CUSTRING *section, const uchar *pos, size_t len)
+  {
+    if (section->str)
+      return true;
+
+    *section= { pos, len };
+    return false;
+  }
+
+  bool read(const uchar* frm_image, size_t frm_size);
+  uchar * write(uchar* frm_image, size_t frm_size);
 };
 
 
diff --git a/sql/table.cc b/sql/table.cc
index 2c963cbb88d..2f2798c6d2e 100644
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -1557,93 +1557,6 @@ bool TABLE_SHARE::init_period_from_extra2(period_info_t *period,
 }
 
 
-static
-bool read_extra2_section_once(const uchar *extra2, size_t len, LEX_CUSTRING *section)
-{
-  if (section->str)
-    return true;
-  *section= {extra2, len};
-  return false;
-}
-
-static
-bool read_extra2(const uchar *frm_image, size_t len, extra2_fields *fields)
-{
-  const uchar *extra2= frm_image + 64;
-
-  DBUG_ENTER("read_extra2");
-
-  fields->reset();
-
-  if (*extra2 != '/')   // old frm had '/' there
-  {
-    const uchar *e2end= extra2 + len;
-    while (extra2 + 3 <= e2end)
-    {
-      extra2_frm_value_type type= (extra2_frm_value_type)*extra2++;
-      size_t length= extra2_read_len(&extra2, e2end);
-      if (!length)
-        DBUG_RETURN(true);
-
-      bool fail= false;
-      switch (type) {
-        case EXTRA2_TABLEDEF_VERSION:
-          if (fields->version.str) // see init_from_sql_statement_string()
-          {
-            if (length != fields->version.length)
-              DBUG_RETURN(true);
-          }
-          else
-          {
-            fields->version.str= extra2;
-            fields->version.length= length;
-          }
-          break;
-        case EXTRA2_ENGINE_TABLEOPTS:
-          fail= read_extra2_section_once(extra2, length, &fields->options);
-          break;
-        case EXTRA2_DEFAULT_PART_ENGINE:
-          fields->engine.set((const char*)extra2, length);
-          break;
-        case EXTRA2_GIS:
-          fail= read_extra2_section_once(extra2, length, &fields->gis);
-          break;
-        case EXTRA2_PERIOD_FOR_SYSTEM_TIME:
-          fail= read_extra2_section_once(extra2, length, &fields->system_period)
-                  || length != 2 * frm_fieldno_size;
-          break;
-        case EXTRA2_FIELD_FLAGS:
-          fail= read_extra2_section_once(extra2, length, &fields->field_flags);
-          break;
-        case EXTRA2_APPLICATION_TIME_PERIOD:
-          fail= read_extra2_section_once(extra2, length, &fields->application_period);
-          break;
-        case EXTRA2_PERIOD_WITHOUT_OVERLAPS:
-          fail= read_extra2_section_once(extra2, length, &fields->without_overlaps);
-          break;
-        case EXTRA2_FIELD_DATA_TYPE_INFO:
-          fail= read_extra2_section_once(extra2, length, &fields->field_data_type_info);
-          break;
-        case EXTRA2_INDEX_FLAGS:
-          fail= read_extra2_section_once(extra2, length, &fields->index_flags);
-          break;
-        default:
-          /* abort frm parsing if it's an unknown but important extra2 value */
-          if (type >= EXTRA2_ENGINE_IMPORTANT)
-            DBUG_RETURN(true);
-      }
-      if (fail)
-        DBUG_RETURN(true);
-
-      extra2+= length;
-    }
-    if (extra2 != e2end)
-      DBUG_RETURN(true);
-  }
-  DBUG_RETURN(false);
-}
-
-
 class Field_data_type_info_array
 {
 public:
@@ -1785,7 +1698,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
   MEM_ROOT *old_root= thd->mem_root;
   Virtual_column_info **table_check_constraints;
   bool *interval_unescaped= NULL;
-  extra2_fields extra2;
+  Extra2_info extra2;
   bool extra_index_flags_present= FALSE;
   DBUG_ENTER("TABLE_SHARE::init_from_binary_frm_image");
 
@@ -1821,10 +1734,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
   new_frm_ver= (frm_image[2] - FRM_VER);
   field_pack_length= new_frm_ver < 2 ? 11 : 17;
 
-  /* Length of the MariaDB extra2 segment in the form file. */
-  len = uint2korr(frm_image+4);
-
-  if (read_extra2(frm_image, len, &extra2))
+  if (extra2.read(frm_image, frm_length))
     goto err;
 
   tabledef_version.length= extra2.version.length;
@@ -1845,8 +1755,8 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
   }
 #endif
 
-  if (frm_length < FRM_HEADER_SIZE + len ||
-      !(pos= uint4korr(frm_image + FRM_HEADER_SIZE + len)))
+  if (frm_length < FRM_HEADER_SIZE + extra2.read_size ||
+      !(pos= uint4korr(frm_image + FRM_HEADER_SIZE + extra2.read_size)))
     goto err;
 
   forminfo= frm_image + pos;
diff --git a/sql/unireg.cc b/sql/unireg.cc
index 7ae42ce34f5..e5e3b068891 100644
--- a/sql/unireg.cc
+++ b/sql/unireg.cc
@@ -345,6 +345,13 @@ LEX_CUSTRING build_frm_image(THD *thd, const LEX_CSTRING &table,
 
   prepare_frm_header(thd, reclength, frm_header, create_info, keys, key_info);
 
+  /*
+    TODO: here we write extra2 directly from create structures. There is also
+    Extra2_info::write() which writes extra2 from its data (previously read by
+    Extra2_info::read()). Extra2_info should be able to import create structures
+    so the below code will not be needed.
+  */
+
   /* one byte for a type, one or three for a length */
   size_t extra2_size= 1 + extra2_str_size(create_info->tabledef_version.length);
   if (options_len)