Skip to content

Commit

Permalink
Bug#33824058 NDB_BUG17624736 FAILS IN PB2 #2
Browse files Browse the repository at this point in the history 
* PROBLEM

The test "ndb.ndb_bug17624736" was constantly failing in
[daily|weekly]-8.0-cluster branches in PB2, whether on `ndb-ps` or
`ndb-default-big` profile test runs. The high-level reason for the
failure was the installation of a duplicate entry in the Data
Dictionary in respect to the `engine`-`se_private_id` pair, even when
the previous table definition should have been dropped.

* LOW-LEVEL EXPLANATION

When data nodes fail and need to reorganize, the MySQL servers
connected start to synchronize the schema definition in their own Data
Dictionary. The `se_private_id` for NDB tables installed in the DD is
the same as the NDB table ID, hereafter refered to as just ID, and
thus a pair `engine`-`se_private_id` is installed in the
`tables.engine`. It is common tables to be updated with different IDs,
such as when an ALTER table or a DROP/CREATE occurs. The previous
table definition, gotten by table full qualified name ("schema.table"
format), is usually sufficient to be dropped and hence the new table
to be installed with the new ID, since it is assumed that no other
table definition is installed with that ID. However, on the
synchronization phase, if the data node failure caused a previous
table definition *of a different table than the one to be installed*
to still exist with the ID to be installed, then that old definition
won't be dropped and thus a duplicate entry warning will be logged on
the THD.

Example:
t1 - id=13,version=1
t2 - id=15,version=1
<failures and synchronization>
t1 = id=9,version=2
t2 = id=13,version=2 (previous def=15, but ndbcluster-13 still exists)

One of the reasons for the error is that on
`Ndb_dd_client::install_table` the name is used to fetch the previous
definition while on `Ndb_dd_client::store_table` the ID is used
instead. Also, `Ndb_dd_client::install_table` should be able to drop
the required table definitions on the DD in order to install the new
one, as dictated by the data nodes. It was just dropping the one found
by the name of the table to be installed.

* SOLUTION

The solution was to add procedures to check if the ID to be installed
is different than the previous, then it must be checked if an old
table definition already exists with that ID. If it does, drop it
also.

Additionally, some renaming (`object_id` to `spi`, refering to
`se_private_id`) and a new struct were employed to make it
simpler to keep the pair (ID-VERSION) together and respectively
install these on the new table's definition SE fields.

Change-Id: Ie671a5fc58646e02c21ef1299309303f33173e95
  • Loading branch information
@dtcpatricio
dtcpatricio committed May 31, 2022
1 parent 9b827eb commit 8588e62
Show file tree
Hide file tree
Showing 9 changed files with 261 additions and 174 deletions.
82 changes: 41 additions & 41 deletions storage/ndb/plugin/ha_ndbcluster.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1886,9 +1886,8 @@ int ha_ndbcluster::get_metadata(Ndb *ndb, const char *dbname,
assert(m_table == nullptr);
assert(m_trans_table_stats == nullptr);

int object_id, object_version;
if (!ndb_dd_table_get_object_id_and_version(table_def, object_id,
object_version)) {
Ndb_dd_handle dd_handle = ndb_dd_table_get_spi_and_version(table_def);
if (!dd_handle.valid()) {
DBUG_PRINT("error", ("Could not extract object_id and object_version "
"from table definition"));
return 1;
Expand All @@ -1900,31 +1899,32 @@ int ha_ndbcluster::get_metadata(Ndb *ndb, const char *dbname,
ERR_RETURN(ndbtab_g.getNdbError());
}

// Check that the id and version from DD
// matches the id and version of the NDB table
const int ndb_object_id = tab->getObjectId();
const int ndb_object_version = tab->getObjectVersion();
if (ndb_object_id != object_id || ndb_object_version != object_version) {
DBUG_PRINT("error", ("Table id or version mismatch"));
DBUG_PRINT("error", ("NDB table id: %u, version: %u", ndb_object_id,
ndb_object_version));
DBUG_PRINT("error",
("DD table id: %u, version: %u", object_id, object_version));

ndb_log_verbose(10,
"Table id or version mismatch for table '%s.%s', "
"[%d, %d] != [%d, %d]",
dbname, tabname, object_id, object_version, ndb_object_id,
ndb_object_version);
{
// Check that the id and version from DD
// matches the id and version of the NDB table
Ndb_dd_handle curr_handle{tab->getObjectId(), tab->getObjectVersion()};
if (curr_handle != dd_handle) {
DBUG_PRINT("error", ("Table id or version mismatch"));
DBUG_PRINT("error", ("NDB table id: %llu, version: %d", curr_handle.spi,
curr_handle.version));
DBUG_PRINT("error", ("DD table id: %llu, version: %d", dd_handle.spi,
dd_handle.version));

ndb_log_verbose(10,
"Table id or version mismatch for table '%s.%s', "
"[%llu, %d] != [%llu, %d]",
dbname, tabname, dd_handle.spi, dd_handle.version,
curr_handle.spi, curr_handle.version);

ndbtab_g.invalidate();
ndbtab_g.invalidate();

// When returning HA_ERR_TABLE_DEF_CHANGED from handler::open()
// the caller is intended to call ha_discover() in order to let
// the engine install the correct table definition in the
// data dictionary, then the open() will be retried and presumably
// the table definition will be correct
return HA_ERR_TABLE_DEF_CHANGED;
// When returning HA_ERR_TABLE_DEF_CHANGED from handler::open()
// the caller is intended to call ha_discover() in order to let
// the engine install the correct table definition in the
// data dictionary, then the open() will be retried and presumably
// the table definition will be correct
return HA_ERR_TABLE_DEF_CHANGED;
}
}

if (DBUG_EVALUATE_IF("ndb_get_metadata_fail", true, false)) {
Expand Down Expand Up @@ -9561,8 +9561,9 @@ int ha_ndbcluster::create(const char *path [[maybe_unused]],

// Update table definition with the table id and version of the NDB table
const NdbDictionary::Table *const ndbtab = ndbtab_g.get_table();
ndb_dd_table_set_object_id_and_version(table_def, ndbtab->getObjectId(),
ndbtab->getObjectVersion());
const Ndb_dd_handle dd_handle{ndbtab->getObjectId(),
ndbtab->getObjectVersion()};
ndb_dd_table_set_spi_and_version(table_def, dd_handle);

return create.succeeded();
}
Expand Down Expand Up @@ -10140,8 +10141,8 @@ int ha_ndbcluster::create(const char *path [[maybe_unused]],

// Update table definition with the table id and version of the newly
// created table, the caller will then save this information in the DD
ndb_dd_table_set_object_id_and_version(table_def, tab.getObjectId(),
tab.getObjectVersion());
ndb_dd_table_set_spi_and_version(table_def, tab.getObjectId(),
tab.getObjectVersion());

// Create secondary indexes
if (create_indexes(thd, table, &tab) != 0) {
Expand Down Expand Up @@ -10721,8 +10722,8 @@ int rename_table_impl(THD *thd, Ndb *ndb,
// The version should have been changed by the rename
assert(ndbtab->getObjectVersion() != ndb_table_version);

ndb_dd_table_set_object_id_and_version(to_table_def, ndb_table_id,
ndbtab->getObjectVersion());
ndb_dd_table_set_spi_and_version(to_table_def, ndb_table_id,
ndbtab->getObjectVersion());

// Log the rename in the Ndb_DDL_transaction_ctx object
ddl_ctx =
Expand Down Expand Up @@ -10889,16 +10890,15 @@ static bool check_table_id_and_version(const dd::Table *table_def,
const NdbDictionary::Table *ndbtab) {
DBUG_TRACE;

int object_id, object_version;
if (!ndb_dd_table_get_object_id_and_version(table_def, object_id,
object_version)) {
Ndb_dd_handle dd_handle = ndb_dd_table_get_spi_and_version(table_def);
if (!dd_handle.valid()) {
return false;
}

// Check that the id and version from DD
// matches the id and version of the NDB table
if (ndbtab->getObjectId() != object_id ||
ndbtab->getObjectVersion() != object_version) {
Ndb_dd_handle curr_handle{ndbtab->getObjectId(), ndbtab->getObjectVersion()};
if (curr_handle != dd_handle) {
return false;
}

Expand Down Expand Up @@ -16624,8 +16624,8 @@ bool ha_ndbcluster::commit_inplace_alter_table(
// The version should have been changed by the alter
assert((Uint32)ndbtab->getObjectVersion() != table_version);

ndb_dd_table_set_object_id_and_version(new_table_def, table_id,
ndbtab->getObjectVersion());
ndb_dd_table_set_spi_and_version(new_table_def, ndbtab->getObjectId(),
ndbtab->getObjectVersion());

// Also check and correct the partition count if required.
const bool check_partition_count_result =
Expand Down Expand Up @@ -17560,8 +17560,8 @@ bool ha_ndbcluster::upgrade_table(THD *thd, const char *db_name,
}

// Set object id and version
ndb_dd_table_set_object_id_and_version(dd_table, ndbtab->getObjectId(),
ndbtab->getObjectVersion());
ndb_dd_table_set_spi_and_version(dd_table, ndbtab->getObjectId(),
ndbtab->getObjectVersion());

/*
Detect and set row format for the table. This is done here
Expand Down
Loading

0 comments on commit 8588e62

Please sign in to comment.