From: Frazer Clement
Date: January 27 2011 3:03pm
Subject: Re: bzr commit into mysql-5.1-telco-7.0 branch (jonas:4144) Bug#59756
List-Archive: http://lists.mysql.com/commits/129783
Message-Id: <4D41892D.1010603@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Comments inline.  Looks ok, but as always, more explanation will be 
useful in future.

Jonas Oreland wrote:
> #At file:///home/jonas/src/telco-7.0/ based on revid:jonas@stripped
> 
>  4144 Jonas Oreland	2011-01-27
>       ndb - bug#59756 - Don't put table into global cache until schema trans has been successfully committed. Add interface to get ObjectId back from createTable()
> 
>     modified:
>       mysql-test/suite/ndb/r/ndb_basic.result
>       mysql-test/suite/ndb/t/ndb_basic.test
>       sql/ha_ndbcluster.cc
>       storage/ndb/include/ndbapi/NdbDictionary.hpp
>       storage/ndb/src/ndbapi/NdbDictionary.cpp
>       storage/ndb/src/ndbapi/NdbDictionaryImpl.cpp
>       storage/ndb/src/ndbapi/NdbDictionaryImpl.hpp
> === modified file 'mysql-test/suite/ndb/r/ndb_basic.result'
> --- a/mysql-test/suite/ndb/r/ndb_basic.result	2011-01-27 10:36:47 +0000
> +++ b/mysql-test/suite/ndb/r/ndb_basic.result	2011-01-27 13:23:07 +0000
> @@ -873,4 +873,16 @@ id	parent_id
>  2	2
>  3	3
>  DROP TABLE child, parent;
> +CREATE TABLE t1 (a INT PRIMARY KEY, b TEXT)
> +ENGINE=ndb PARTITION BY KEY(a) PARTITIONS 24;
> +ERROR HY000: Can't create table 'test.t1' (errno: 140)
> +show warnings;
> +Level	Code	Message
> +Error	1296	Got error 1224 'Too many fragments' from NDB
> +Error	1005	Can't create table 'test.t1' (errno: 140)
> +CREATE TABLE t1 (a INT PRIMARY KEY, b TEXT)
> +ENGINE=ndb;
> +show warnings;
> +Level	Code	Message
> +drop table t1;
>  End of 5.1 tests
> 
> === modified file 'mysql-test/suite/ndb/t/ndb_basic.test'
> --- a/mysql-test/suite/ndb/t/ndb_basic.test	2011-01-27 10:36:47 +0000
> +++ b/mysql-test/suite/ndb/t/ndb_basic.test	2011-01-27 13:23:07 +0000
> @@ -785,4 +785,16 @@ SELECT * FROM child ORDER BY id;
>  
>  DROP TABLE child, parent;
>  
> +#
> +# bug#59756
> +#
> +--error 1005
> +CREATE TABLE t1 (a INT PRIMARY KEY, b TEXT)
> +ENGINE=ndb PARTITION BY KEY(a) PARTITIONS 24;
> +show warnings;
> +CREATE TABLE t1 (a INT PRIMARY KEY, b TEXT)
> +ENGINE=ndb;
> +show warnings;
> +drop table t1;
> +
>  --echo End of 5.1 tests
> 
> === modified file 'sql/ha_ndbcluster.cc'
> --- a/sql/ha_ndbcluster.cc	2011-01-27 10:36:47 +0000
> +++ b/sql/ha_ndbcluster.cc	2011-01-27 13:23:07 +0000
> @@ -7674,6 +7674,7 @@ int ha_ndbcluster::create(const char *na
>    bool ndb_sys_table= FALSE;
>    partition_info *part_info;
>    int result= 0;
> +  NdbDictionary::ObjectId objId;
>  
>    DBUG_ENTER("ha_ndbcluster::create");
>    DBUG_PRINT("enter", ("name: %s", name));
> @@ -7700,7 +7701,6 @@ int ha_ndbcluster::create(const char *na
>    
>    Ndb *ndb= get_ndb(thd);
>    NDBDICT *dict= ndb->getDictionary();
> -  Ndb_table_guard ndbtab_g(dict);
>  
>  #ifndef NDB_WITHOUT_TABLESPACE_IN_FRM
>    DBUG_PRINT("info", ("Tablespace %s,%s", form->s->tablespace, create_info->tablespace));
> @@ -7754,6 +7754,7 @@ int ha_ndbcluster::create(const char *na
>  
>    if (is_truncate)
>    {
> +    Ndb_table_guard ndbtab_g(dict);
>      ndbtab_g.init(m_tabname);
>      if (!(m_table= ndbtab_g.get_table()))
>        ERR_RETURN(dict->getNdbError());
> @@ -8055,7 +8056,7 @@ int ha_ndbcluster::create(const char *na
>    }
>  
>    // Create the table in NDB     
> -  if (dict->createTable(tab) != 0) 
> +  if (dict->createTable(tab, &objId) != 0)
>    {
>      const NdbError err= dict->getNdbError();
>      set_ndb_err(thd, err);
> @@ -8063,26 +8064,14 @@ int ha_ndbcluster::create(const char *na
>      goto abort;
>    }
>  
> -  ndbtab_g.init(m_tabname);
> -  // temporary set m_table during create
> -  // reset at return
> -  m_table= ndbtab_g.get_table();
> -  // TODO check also that we have the same frm...
> -  if (!m_table)
> -  {
> -    /* purecov: begin deadcode */
> -    const NdbError err= dict->getNdbError();
> -    set_ndb_err(thd, err);
> -    my_errno= ndb_to_mysql_error(&err);
> -    goto abort;
> -    /* purecov: end */
> -  }
> -
>    DBUG_PRINT("info", ("Table %s/%s created successfully", 
>                        m_dbname, m_tabname));
>  
>    // Create secondary indexes

Add comment :
  // Assigning table's created objectid for index creation to reference

> +  tab.assignObjId(objId);
> +  m_table= &tab;
>    my_errno= create_indexes(thd, ndb, form);
> +  m_table= 0;
>  
>    if (!my_errno)
>    {
> @@ -8118,6 +8107,13 @@ err_return:
>      m_table= 0;
>      ERR_RETURN(dict->getNdbError());
>    }
> +
> +  /**
> +   * createTable/index schema transaction OK
> +   */

Add comment :
   // Load table definition back from kernel

> +  Ndb_table_guard ndbtab_g(dict, m_tabname);
> +  m_table= ndbtab_g.get_table();
> +
>    if (my_errno)
>    {
>      /*
> 
> === modified file 'storage/ndb/include/ndbapi/NdbDictionary.hpp'
> --- a/storage/ndb/include/ndbapi/NdbDictionary.hpp	2010-10-13 09:33:02 +0000
> +++ b/storage/ndb/include/ndbapi/NdbDictionary.hpp	2011-01-27 13:23:07 +0000
> @@ -1041,6 +1041,14 @@ public:
>       *   passing NULL pointer will equal to bitmap with all columns set
>       */
>      int checkColumns(const Uint32* bitmap, unsigned len_in_bytes) const;
> +
> +    /**
> +     * Set tableId,tableVersion on a table...
> +     *   this is a "work-around" since createIndex can't (currently)
> +     *   accept an ObjectId instead of table-object in createIndex
> +     *   this as way way too much stuff is pushed into NdbDictInterface
> +     */

Remove comment about 'way way too much stuff is pushed...'
Mention instead that tableid, tableversion are required for createIndex, 
so this simplifies the createTable(), createIndex() call sequence.

> +    void assignObjId(const ObjectId &);
>  #endif
>  
>      // these 2 are not de-doxygenated
> @@ -2211,6 +2219,14 @@ public:
>      int createTable(const Table &table);
>  
>      /**
> +     * Create defined table given defined Table instance
> +     *   return ObjectId
> +     * @param table Table to create
> +     * @return 0 if successful otherwise -1.
> +     */
> +    int createTable(const Table &table, ObjectId * objid);
> +
> +    /**
>       * Start table optimization given defined table object
>       * @param t Object of table to optimize
>       * @param Pre-allocated OptimizeTableHandle
> 
> === modified file 'storage/ndb/src/ndbapi/NdbDictionary.cpp'
> --- a/storage/ndb/src/ndbapi/NdbDictionary.cpp	2010-09-30 14:27:18 +0000
> +++ b/storage/ndb/src/ndbapi/NdbDictionary.cpp	2011-01-27 13:23:07 +0000
> @@ -532,6 +532,7 @@ NdbDictionary::Table::addColumn(const Co
>    {
>      return -1;
>    }
> +  col->m_column_no = m_impl.m_columns.size() - 1;

Is this a separate bug?

>    return 0;
>  }
>  
> @@ -976,6 +977,14 @@ NdbDictionary::Table::getPartitionId(Uin
>    }
>  }
>  
> +void
> +NdbDictionary::Table::assignObjId(const NdbDictionary::ObjectId& _objId)
> +{
> +  const NdbDictObjectImpl& objId = NdbDictObjectImpl::getImpl(_objId);
> +  m_impl.m_id = objId.m_id;
> +  m_impl.m_version = objId.m_version;
> +}
> +

Quite dangerous if it's misused!

>  /*****************************************************************
>   * Index facade
>   */
> @@ -2064,12 +2073,23 @@ NdbDictionary::Dictionary::~Dictionary()
>  int 
>  NdbDictionary::Dictionary::createTable(const Table & t)
>  {
> +  return createTable(t, 0);
> +}
> +
> +int
> +NdbDictionary::Dictionary::createTable(const Table & t, ObjectId * objId)
> +{
>    int ret;
> +  ObjectId tmp;
> +  if (objId == 0)
> +    objId = &tmp;
> +
>    if (likely(! is_ndb_blob_table(t.getName())))
>    {
>      DO_TRANS(
>        ret,
> -      m_impl.createTable(NdbTableImpl::getImpl(t))
> +      m_impl.createTable(NdbTableImpl::getImpl(t),
> +                         NdbDictObjectImpl::getImpl( *objId))
>      );
>    }
>    else
> 
> === modified file 'storage/ndb/src/ndbapi/NdbDictionaryImpl.cpp'
> --- a/storage/ndb/src/ndbapi/NdbDictionaryImpl.cpp	2011-01-12 08:04:39 +0000
> +++ b/storage/ndb/src/ndbapi/NdbDictionaryImpl.cpp	2011-01-27 13:23:07 +0000
> @@ -2879,7 +2879,7 @@ NdbDictInterface::parseTableInfo(NdbTabl
>   * Create table and alter table
>   */
>  int
> -NdbDictionaryImpl::createTable(NdbTableImpl &t)
> +NdbDictionaryImpl::createTable(NdbTableImpl &t, NdbDictObjectImpl & objid)
>  { 
>    DBUG_ENTER("NdbDictionaryImpl::createTable");
>  
> @@ -2910,6 +2910,8 @@ NdbDictionaryImpl::createTable(NdbTableI
>    Uint32* data = (Uint32*)m_receiver.m_buffer.get_data();
>    t.m_id = data[0];
>    t.m_version = data[1];
> +  objid.m_id = data[0];
> +  objid.m_version = data[1];
>  
>    // update table def from DICT - by-pass cache
>    NdbTableImpl* t2 =
> @@ -3014,7 +3016,8 @@ NdbDictionaryImpl::createBlobTables(cons
>        assert(bc != NULL);
>        bc->setStorageType(d);
>      }
> -    if (createTable(bt) != 0) {
> +    NdbDictionary::ObjectId objId; // ignore objid
> +    if (createTable(bt, NdbDictObjectImpl::getImpl(objId)) != 0) {
>        DBUG_RETURN(-1);
>      }
>    }
> 
> === modified file 'storage/ndb/src/ndbapi/NdbDictionaryImpl.hpp'
> --- a/storage/ndb/src/ndbapi/NdbDictionaryImpl.hpp	2010-11-09 20:40:03 +0000
> +++ b/storage/ndb/src/ndbapi/NdbDictionaryImpl.hpp	2011-01-27 13:23:07 +0000
> @@ -817,7 +817,7 @@ public:
>    bool setTransporter(class Ndb * ndb, class TransporterFacade * tf);
>    bool setTransporter(class TransporterFacade * tf);
>  
> -  int createTable(NdbTableImpl &t);
> +  int createTable(NdbTableImpl &t, NdbDictObjectImpl &);
>    int optimizeTable(const NdbTableImpl &t,
>                      NdbOptimizeTableHandleImpl &h);
>    int optimizeIndex(const NdbIndexImpl &index,
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 


-- 
Frazer Clement, Senior Software Engineer,
MySQL Cluster / Oracle - www.mysql.com
Office: Edinburgh, UK

Are you MySQL certified?  www.mysql.com/certification