- developers - lists.mariadb.org

Re: [Maria-developers] a4b8e0c30d1: MDEV-18800 Server crash in instant_alter_column_possible or Assertion...
by Sergei Golubchik 13 Mar '19

13 Mar '19

Hi, Sachin! ok to push On Mar 12, Sachin Setiya wrote: > revision-id: a4b8e0c30d1 (mariadb-10.4.3-60-ga4b8e0c30d1) > parent(s): d8e4463d5dc > author: sachin <sachin.setiya(a)mariadb.com> > committer: sachin <sachin.setiya(a)mariadb.com> > timestamp: 2019-03-12 19:29:57 +0530 > message: > > MDEV-18800 Server crash in instant_alter_column_possible or Assertion... > `!pk->has_virtual()' failed in instant_alter_column_possible upon adding key > > Hash key can't be primary key. > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 2d86035b7fd..a9c4c71331a 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -4179,6 +4179,7 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info, > } > if (is_hash_field_needed || > (key_info->algorithm == HA_KEY_ALG_HASH && > + key->type != Key::PRIMARY && > key_info->flags & HA_NOSAME && > !(file->ha_table_flags() & HA_CAN_HASH_KEYS ) && > file->ha_table_flags() & HA_CAN_VIRTUAL_COLUMNS)) > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] e2f8f968785: MDEV-18790 Server crash in fields_in_hash_keyinfo after unsuccessful...
by Sergei Golubchik 13 Mar '19

13 Mar '19

Hi, Sachin! Ok to push, just one minor detail, see below On Mar 12, Sachin Setiya wrote: > revision-id: e2f8f968785 (mariadb-10.4.3-57-ge2f8f968785) > parent(s): 6d68a3464e9 > author: sachin <sachin.setiya(a)mariadb.com> > committer: sachin <sachin.setiya(a)mariadb.com> > timestamp: 2019-03-12 19:29:57 +0530 > message: > > MDEV-18790 Server crash in fields_in_hash_keyinfo after unsuccessful... > attempt to drop BLOB with long index > > Restore long table->key_info, So that in the case of unsuccessful table > alter table->key_info should have a correct value. In the case of successful > table alter old table is flushed so that is why we don't see this error in > the case of successful alter. > > diff --git a/mysql-test/main/long_unique_bugs.result b/mysql-test/main/long_unique_bugs.result > index 5de7566c8a3..1544565d95e 100644 > --- a/mysql-test/main/long_unique_bugs.result > +++ b/mysql-test/main/long_unique_bugs.result > @@ -57,3 +57,9 @@ create table t (b blob, unique(b)) engine=myisam; > insert into t values ('foo'); > replace into t values ('foo'); > drop table t; > +CREATE TABLE t1 (f INT, x BLOB, UNIQUE (x)); > +INSERT INTO t1 VALUES (1,'foo'); > +ALTER TABLE t1 DROP x, ALGORITHM=INPLACE; > +Got one of the listed errors > +UPDATE t1 SET x = 'bar'; > +DROP TABLE t1; > diff --git a/mysql-test/main/long_unique_bugs.test b/mysql-test/main/long_unique_bugs.test > index 0246ea7962a..14c002f998c 100644 > --- a/mysql-test/main/long_unique_bugs.test > +++ b/mysql-test/main/long_unique_bugs.test > @@ -72,3 +72,13 @@ create table t (b blob, unique(b)) engine=myisam; > insert into t values ('foo'); > replace into t values ('foo'); > drop table t; > + > +# > +# MDEV-18790 Server crash in fields_in_hash_keyinfo after unsuccessful attempt to drop BLOB with long index > +# > +CREATE TABLE t1 (f INT, x BLOB, UNIQUE (x)); > +INSERT INTO t1 VALUES (1,'foo'); > +--error ER_ALTER_OPERATION_NOT_SUPPORTED,ER_ALTER_OPERATION_NOT_SUPPORTED_REASON this makes the test to accept any of the listed error messages and the result file will have "Got one of the listed errors". It's useful when a test can indeed see different errors in a certain place (Elena has probably run the test with different storage engines, and has seen different errors for MyISAM and InnoDB). But this final committed test case is very deterministic, and as far as I can see there can be only one possible error code here. It's generally better to have just this one error code in the `--error` directive and the correct complete error message text in the result file. > +ALTER TABLE t1 DROP x, ALGORITHM=INPLACE; > +UPDATE t1 SET x = 'bar'; > +DROP TABLE t1; Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 190a29d06: Make Lock Tree's lock waits use externally provided mutexes/conditions
by jocelyn fournier 12 Mar '19

12 Mar '19

Hi Sergei! Just wondering why keeping the toku_ prefix after renaming toku_* to toku_external_* since it’s also used by other engine like RockDB? BR, Jocelyn Fournier > Le 12 mars 2019 à 09:52, Sergei Petrunia <psergey(a)askmonty.org> a écrit : > > revision-id: 190a29d06f4e79d2df4cb513944ac34bd133caa0 (v5.8-1024-g190a29d06) > parent(s): 7a8c777d4880cde8fd96135f6cc216223371a9f4 > author: Sergei Petrunia > committer: Sergei Petrunia > timestamp: 2019-03-12 11:52:17 +0300 > message: > > Make Lock Tree's lock waits use externally provided mutexes/conditions > > In MyRocks, this allows: > - SHOW PROCESSLIST to show "waiting for row lock" for waiting threads > - KILL statement to abort the lock wait in the same way as it does it with > point locking (without use of handlerton::kill_connection() method) > > - Remove previously added TransactionDB::KillLockWait() method as it is > no longer needed. > > --- > include/rocksdb/utilities/transaction_db.h | 1 - > .../transactions/pessimistic_transaction_db.cc | 12 +++- > .../transactions/pessimistic_transaction_db.h | 4 -- > .../range_locking/locktree/lock_request.cc | 35 ++++++---- > .../range_locking/locktree/lock_request.h | 9 ++- > .../range_locking/locktree/locktree.cc | 12 ++-- > .../transactions/range_locking/locktree/locktree.h | 13 ++-- > .../transactions/range_locking/locktree/manager.cc | 17 +++-- > .../portability/toku_external_pthread.h | 81 ++++++++++++++++++++++ > utilities/transactions/transaction_lock_mgr.cc | 15 ++-- > utilities/transactions/transaction_lock_mgr.h | 6 +- > 11 files changed, 155 insertions(+), 50 deletions(-) > > diff --git a/include/rocksdb/utilities/transaction_db.h b/include/rocksdb/utilities/transaction_db.h > index 2a98a8d3d..fb3c6a88c 100644 > --- a/include/rocksdb/utilities/transaction_db.h > +++ b/include/rocksdb/utilities/transaction_db.h > @@ -291,7 +291,6 @@ class TransactionDB : public StackableDB { > bool use_range_locking; > virtual RangeLockMgrControl* get_range_lock_manager() { return nullptr; } > > - virtual void KillLockWait(TransactionID txnid){}; > protected: > // To Create an TransactionDB, call Open() > // The ownership of db is transferred to the base StackableDB > diff --git a/utilities/transactions/pessimistic_transaction_db.cc b/utilities/transactions/pessimistic_transaction_db.cc > index d44f329e5..308187e13 100644 > --- a/utilities/transactions/pessimistic_transaction_db.cc > +++ b/utilities/transactions/pessimistic_transaction_db.cc > @@ -41,7 +41,11 @@ PessimisticTransactionDB::PessimisticTransactionDB( > ? txn_db_options_.custom_mutex_factory > : std::shared_ptr<TransactionDBMutexFactory>( > new TransactionDBMutexFactoryImpl())), > - range_lock_mgr_(this) { > + range_lock_mgr_(this, > + txn_db_options_.custom_mutex_factory? > + txn_db_options_.custom_mutex_factory : > + std::shared_ptr<TransactionDBMutexFactory>( > + new TransactionDBMutexFactoryImpl())) { > assert(db_impl_ != nullptr); > info_log_ = db_impl_->GetDBOptions().info_log; > } > @@ -73,7 +77,11 @@ PessimisticTransactionDB::PessimisticTransactionDB( > ? txn_db_options_.custom_mutex_factory > : std::shared_ptr<TransactionDBMutexFactory>( > new TransactionDBMutexFactoryImpl())), > - range_lock_mgr_(this) { > + range_lock_mgr_(this, > + txn_db_options_.custom_mutex_factory > + ? txn_db_options_.custom_mutex_factory > + : std::shared_ptr<TransactionDBMutexFactory>( > + new TransactionDBMutexFactoryImpl())) { > assert(db_impl_ != nullptr); > } > > diff --git a/utilities/transactions/pessimistic_transaction_db.h b/utilities/transactions/pessimistic_transaction_db.h > index 07ed3fed2..388ed4099 100644 > --- a/utilities/transactions/pessimistic_transaction_db.h > +++ b/utilities/transactions/pessimistic_transaction_db.h > @@ -127,10 +127,6 @@ class PessimisticTransactionDB : public TransactionDB { > virtual void UpdateCFComparatorMap(const std::vector<ColumnFamilyHandle*>&) {} > virtual void UpdateCFComparatorMap(ColumnFamilyHandle*) {} > > - void KillLockWait(TransactionID txnid) override { > - if (use_range_locking) > - range_lock_mgr_.KillLockWait(txnid); > - } > protected: > DBImpl* db_impl_; > std::shared_ptr<Logger> info_log_; > diff --git a/utilities/transactions/range_locking/locktree/lock_request.cc b/utilities/transactions/range_locking/locktree/lock_request.cc > index 0b96b2051..84cfdbf62 100644 > --- a/utilities/transactions/range_locking/locktree/lock_request.cc > +++ b/utilities/transactions/range_locking/locktree/lock_request.cc > @@ -59,7 +59,7 @@ Copyright (c) 2006, 2015, Percona and/or its affiliates. All rights reserved. > namespace toku { > > // initialize a lock request's internals > -void lock_request::create(void) { > +void lock_request::create(toku_external_mutex_factory_t mutex_factory) { > m_txnid = TXNID_NONE; > m_conflicting_txnid = TXNID_NONE; > m_start_time = 0; > @@ -75,7 +75,9 @@ void lock_request::create(void) { > m_state = state::UNINITIALIZED; > m_info = nullptr; > > - toku_cond_init(*lock_request_m_wait_cond_key, &m_wait_cond, nullptr); > + //psergey-todo: this condition is for interruptible wait > + // note: moved to here from lock_request::create: > + toku_external_cond_init(mutex_factory, &m_wait_cond); > > m_start_test_callback = nullptr; > m_start_before_pending_test_callback = nullptr; > @@ -89,13 +91,14 @@ void lock_request::destroy(void) { > m_state = state::DESTROYED; > toku_destroy_dbt(&m_left_key_copy); > toku_destroy_dbt(&m_right_key_copy); > - toku_cond_destroy(&m_wait_cond); > + toku_external_cond_destroy(&m_wait_cond); > } > > // set the lock request parameters. this API allows a lock request to be reused. > void lock_request::set(locktree *lt, TXNID txnid, const DBT *left_key, const DBT *right_key, lock_request::type lock_type, bool big_txn, void *extra) { > invariant(m_state != state::PENDING); > m_lt = lt; > + > m_txnid = txnid; > m_left_key = left_key; > m_right_key = right_key; > @@ -190,13 +193,13 @@ int lock_request::start(void) { > m_conflicting_txnid = conflicts.get(0); > if (m_start_before_pending_test_callback) > m_start_before_pending_test_callback(); > - toku_mutex_lock(&m_info->mutex); > + toku_external_mutex_lock(&m_info->mutex); > insert_into_lock_requests(); > if (deadlock_exists(conflicts)) { > remove_from_lock_requests(); > r = DB_LOCK_DEADLOCK; > } > - toku_mutex_unlock(&m_info->mutex); > + toku_external_mutex_unlock(&m_info->mutex); > if (m_start_test_callback) > m_start_test_callback(); // test callback > } > @@ -221,7 +224,7 @@ int lock_request::wait(uint64_t wait_time_ms, uint64_t killed_time_ms, int (*kil > uint64_t t_start = t_now; > uint64_t t_end = t_start + wait_time_ms * 1000; > > - toku_mutex_lock(&m_info->mutex); > + toku_external_mutex_lock(&m_info->mutex); > > // check again, this time locking out other retry calls > if (m_state == state::PENDING) { > @@ -252,10 +255,14 @@ int lock_request::wait(uint64_t wait_time_ms, uint64_t killed_time_ms, int (*kil > if (t_wait > t_end) > t_wait = t_end; > } > + /* > + PORT: don't need to compute "time when the wait should end" anymore. > struct timespec ts = {0, 0}; > ts.tv_sec = t_wait / 1000000; > ts.tv_nsec = (t_wait % 1000000) * 1000; > - int r = toku_cond_timedwait(&m_wait_cond, &m_info->mutex, &ts); > + */ > + int r = toku_external_cond_timedwait(&m_wait_cond, &m_info->mutex, > + int32_t(wait_time_ms)*1000); > invariant(r == 0 || r == ETIMEDOUT); > > t_now = toku_current_time_microsec(); > @@ -279,7 +286,7 @@ int lock_request::wait(uint64_t wait_time_ms, uint64_t killed_time_ms, int (*kil > m_info->counters.long_wait_count += 1; > m_info->counters.long_wait_time += duration; > } > - toku_mutex_unlock(&m_info->mutex); > + toku_external_mutex_unlock(&m_info->mutex); > > invariant(m_state == state::COMPLETE); > return m_complete_r; > @@ -332,7 +339,7 @@ int lock_request::retry(GrowableArray<TXNID> *conflicts_collector) { > complete(r); > if (m_retry_test_callback) > m_retry_test_callback(); // test callback > - toku_cond_broadcast(&m_wait_cond); > + toku_external_cond_broadcast(&m_wait_cond); > } else { > m_conflicting_txnid = conflicts.get(0); > add_conflicts_to_waits(&conflicts, conflicts_collector); > @@ -393,7 +400,7 @@ void lock_request::retry_all_lock_requests( > } > > void lock_request::retry_all_lock_requests_info(lt_lock_request_info *info, GrowableArray<TXNID> *collector) { > - toku_mutex_lock(&info->mutex); > + toku_external_mutex_lock(&info->mutex); > // retry all of the pending lock requests. > for (size_t i = 0; i < info->pending_lock_requests.size();) { > lock_request *request; > @@ -412,7 +419,7 @@ void lock_request::retry_all_lock_requests_info(lt_lock_request_info *info, Grow > > // future threads should only retry lock requests if some still exist > info->should_retry_lock_requests = info->pending_lock_requests.size() > 0; > - toku_mutex_unlock(&info->mutex); > + toku_external_mutex_unlock(&info->mutex); > } > > void lock_request::add_conflicts_to_waits(txnid_set *conflicts, > @@ -444,12 +451,12 @@ void *lock_request::get_extra(void) const { > void lock_request::kill_waiter(void) { > remove_from_lock_requests(); > complete(DB_LOCK_NOTGRANTED); > - toku_cond_broadcast(&m_wait_cond); > + toku_external_cond_broadcast(&m_wait_cond); > } > > void lock_request::kill_waiter(locktree *lt, void *extra) { > lt_lock_request_info *info = lt->get_lock_request_info(); > - toku_mutex_lock(&info->mutex); > + toku_external_mutex_lock(&info->mutex); > for (size_t i = 0; i < info->pending_lock_requests.size(); i++) { > lock_request *request; > int r = info->pending_lock_requests.fetch(i, &request); > @@ -458,7 +465,7 @@ void lock_request::kill_waiter(locktree *lt, void *extra) { > break; > } > } > - toku_mutex_unlock(&info->mutex); > + toku_external_mutex_unlock(&info->mutex); > } > > // find another lock request by txnid. must hold the mutex. > diff --git a/utilities/transactions/range_locking/locktree/lock_request.h b/utilities/transactions/range_locking/locktree/lock_request.h > index 8b9529344..b17b639c8 100644 > --- a/utilities/transactions/range_locking/locktree/lock_request.h > +++ b/utilities/transactions/range_locking/locktree/lock_request.h > @@ -84,7 +84,7 @@ public: > }; > > // effect: Initializes a lock request. > - void create(void); > + void create(toku_external_mutex_factory_t mutex_factory); > > // effect: Destroys a lock request. > void destroy(void); > @@ -171,7 +171,7 @@ public: > int m_complete_r; > state m_state; > > - toku_cond_t m_wait_cond; > + toku_external_cond_t m_wait_cond; > > bool m_big_txn; > > @@ -227,6 +227,9 @@ public: > > friend class lock_request_unit_test; > }; > -ENSURE_POD(lock_request); > +// PORT: lock_request is not a POD anymore due to use of toku_external_cond_t > +// This is ok as the PODness is not really required: lock_request objects are > +// not moved in memory or anything. > +//ENSURE_POD(lock_request); > > } /* namespace toku */ > diff --git a/utilities/transactions/range_locking/locktree/locktree.cc b/utilities/transactions/range_locking/locktree/locktree.cc > index 4ecfb2d26..9b530c7b0 100644 > --- a/utilities/transactions/range_locking/locktree/locktree.cc > +++ b/utilities/transactions/range_locking/locktree/locktree.cc > @@ -74,7 +74,9 @@ namespace toku { > // but does nothing based on the value of the reference count - it is > // up to the user of the locktree to destroy it when it sees fit. > > -void locktree::create(locktree_manager *mgr, DICTIONARY_ID dict_id, const comparator &cmp) { > +void locktree::create(locktree_manager *mgr, DICTIONARY_ID dict_id, > + const comparator &cmp, > + toku_external_mutex_factory_t mutex_factory) { > m_mgr = mgr; > m_dict_id = dict_id; > > @@ -91,14 +93,14 @@ void locktree::create(locktree_manager *mgr, DICTIONARY_ID dict_id, const compar > m_sto_end_early_count = 0; > m_sto_end_early_time = 0; > > - m_lock_request_info.init(); > + m_lock_request_info.init(mutex_factory); > } > > -void lt_lock_request_info::init(void) { > +void lt_lock_request_info::init(toku_external_mutex_factory_t mutex_factory) { > pending_lock_requests.create(); > pending_is_empty = true; > ZERO_STRUCT(mutex); > - toku_mutex_init(*locktree_request_info_mutex_key, &mutex, nullptr); > + toku_external_mutex_init(mutex_factory, &mutex); > retry_want = retry_done = 0; > ZERO_STRUCT(counters); > ZERO_STRUCT(retry_mutex); > @@ -124,7 +126,7 @@ void locktree::destroy(void) { > > void lt_lock_request_info::destroy(void) { > pending_lock_requests.destroy(); > - toku_mutex_destroy(&mutex); > + toku_external_mutex_destroy(&mutex); > toku_mutex_destroy(&retry_mutex); > toku_cond_destroy(&retry_cv); > } > diff --git a/utilities/transactions/range_locking/locktree/locktree.h b/utilities/transactions/range_locking/locktree/locktree.h > index 4f0530855..5ff4f7449 100644 > --- a/utilities/transactions/range_locking/locktree/locktree.h > +++ b/utilities/transactions/range_locking/locktree/locktree.h > @@ -55,6 +55,7 @@ Copyright (c) 2006, 2015, Percona and/or its affiliates. All rights reserved. > > #include <db.h> > #include <toku_pthread.h> > +#include <toku_external_pthread.h> > #include <toku_time.h> > > #include <ft/comparator.h> > @@ -102,7 +103,7 @@ namespace toku { > struct lt_lock_request_info { > omt<lock_request *> pending_lock_requests; > std::atomic_bool pending_is_empty; > - toku_mutex_t mutex; > + toku_external_mutex_t mutex; > bool should_retry_lock_requests; > lt_counters counters; > std::atomic_ullong retry_want; > @@ -111,7 +112,7 @@ namespace toku { > toku_cond_t retry_cv; > bool running_retry; > > - void init(void); > + void init(toku_external_mutex_factory_t mutex_factory); > void destroy(void); > }; > > @@ -127,7 +128,8 @@ namespace toku { > void create(lt_create_cb create_cb, > lt_destroy_cb destroy_cb, > lt_escalate_cb escalate_cb, > - void *extra); > + void *extra, > + toku_external_mutex_factory_t mutex_factory_arg); > > void destroy(void); > > @@ -212,6 +214,8 @@ namespace toku { > > omt<locktree *> m_locktree_map; > > + toku_external_mutex_factory_t mutex_factory; > + > // the manager's mutex protects the locktree map > toku_mutex_t m_mutex; > > @@ -275,7 +279,8 @@ namespace toku { > class locktree { > public: > // effect: Creates a locktree > - void create(locktree_manager *mgr, DICTIONARY_ID dict_id, const comparator &cmp); > + void create(locktree_manager *mgr, DICTIONARY_ID dict_id, const comparator &cmp, > + toku_external_mutex_factory_t mutex_factory); > > void destroy(void); > > diff --git a/utilities/transactions/range_locking/locktree/manager.cc b/utilities/transactions/range_locking/locktree/manager.cc > index c31158bb8..47a782565 100644 > --- a/utilities/transactions/range_locking/locktree/manager.cc > +++ b/utilities/transactions/range_locking/locktree/manager.cc > @@ -60,7 +60,12 @@ Copyright (c) 2006, 2015, Percona and/or its affiliates. All rights reserved. > > namespace toku { > > -void locktree_manager::create(lt_create_cb create_cb, lt_destroy_cb destroy_cb, lt_escalate_cb escalate_cb, void *escalate_extra) { > +void locktree_manager::create(lt_create_cb create_cb, > + lt_destroy_cb destroy_cb, > + lt_escalate_cb escalate_cb, > + void *escalate_extra, > + toku_external_mutex_factory_t mutex_factory_arg) { > + mutex_factory= mutex_factory_arg; > m_max_lock_memory = DEFAULT_MAX_LOCK_MEMORY; > m_current_lock_memory = 0; > > @@ -151,7 +156,7 @@ locktree *locktree_manager::get_lt(DICTIONARY_ID dict_id, > locktree *lt = locktree_map_find(dict_id); > if (lt == nullptr) { > XCALLOC(lt); > - lt->create(this, dict_id, cmp); > + lt->create(this, dict_id, cmp, mutex_factory); > > // new locktree created - call the on_create callback > // and put it in the locktree map > @@ -320,7 +325,7 @@ int locktree_manager::iterate_pending_lock_requests(lock_request_iterate_callbac > invariant_zero(r); > > struct lt_lock_request_info *info = lt->get_lock_request_info(); > - toku_mutex_lock(&info->mutex); > + toku_external_mutex_lock(&info->mutex); > > size_t num_requests = info->pending_lock_requests.size(); > for (size_t k = 0; k < num_requests && r == 0; k++) { > @@ -332,7 +337,7 @@ int locktree_manager::iterate_pending_lock_requests(lock_request_iterate_callbac > req->get_conflicting_txnid(), req->get_start_time(), extra); > } > > - toku_mutex_unlock(&info->mutex); > + toku_external_mutex_unlock(&info->mutex); > } > mutex_unlock(); > return r; > @@ -472,10 +477,10 @@ void locktree_manager::get_status(LTM_STATUS statp) { > locktree *lt; > int r = m_locktree_map.fetch(i, &lt); > invariant_zero(r); > - if (toku_mutex_trylock(&lt->m_lock_request_info.mutex) == 0) { > + if (toku_external_mutex_trylock(&lt->m_lock_request_info.mutex) == 0) { > lock_requests_pending += lt->m_lock_request_info.pending_lock_requests.size(); > lt_counters.add(lt->get_lock_request_info()->counters); > - toku_mutex_unlock(&lt->m_lock_request_info.mutex); > + toku_external_mutex_unlock(&lt->m_lock_request_info.mutex); > } > sto_num_eligible += lt->sto_txnid_is_valid_unsafe() ? 1 : 0; > sto_end_early_count += lt->m_sto_end_early_count; > diff --git a/utilities/transactions/range_locking/portability/toku_external_pthread.h b/utilities/transactions/range_locking/portability/toku_external_pthread.h > new file mode 100644 > index 000000000..2482ec507 > --- /dev/null > +++ b/utilities/transactions/range_locking/portability/toku_external_pthread.h > @@ -0,0 +1,81 @@ > +/* > + A wrapper around rocksdb::TransactionDBMutexFactory-provided condition and mutex > + that provides toku_pthread_*-like interface. The functions are named > + > + toku_external_{mutex|cond}_XXX > + > + Lock Tree uses this mutex and condition for interruptible (long) lock waits. > + > + (It also still uses toku_pthread_XXX calls for mutexes/conditions for > + shorter waits on internal objects) > +*/ > + > +#pragma once > + > +#include <pthread.h> > +#include <time.h> > +#include <stdint.h> > + > +#include "toku_portability.h" > + > +#include "rocksdb/utilities/transaction_db.h" > +#include "rocksdb/utilities/transaction_db_mutex.h" > + > +using rocksdb::TransactionDBMutex; > +using rocksdb::TransactionDBCondVar; > + > +typedef std::shared_ptr<rocksdb::TransactionDBMutexFactory> toku_external_mutex_factory_t; > + > +typedef std::shared_ptr<TransactionDBMutex> toku_external_mutex_t; > +typedef std::shared_ptr<TransactionDBCondVar> toku_external_cond_t; > + > +static inline > +void toku_external_cond_init(toku_external_mutex_factory_t mutex_factory, > + toku_external_cond_t *cond) { > + *cond= mutex_factory->AllocateCondVar(); > +} > + > +inline void toku_external_cond_destroy(toku_external_cond_t *cond) { > + cond->reset(); // this will destroy the managed object > +} > + > +inline void toku_external_cond_signal(toku_external_cond_t *cond) { > + (*cond)->Notify(); > +} > + > +inline void toku_external_cond_broadcast(toku_external_cond_t *cond) { > + (*cond)->NotifyAll(); > +} > + > +inline int toku_external_cond_timedwait(toku_external_cond_t *cond, > + toku_external_mutex_t *mutex, > + int32_t timeout_microsec) { > + auto res= (*cond)->WaitFor(*mutex, timeout_microsec); > + if (res.ok()) > + return 0; > + else > + return ETIMEDOUT; > +} > + > +inline void toku_external_mutex_init(toku_external_mutex_factory_t factory, > + toku_external_mutex_t *mutex) { > + *mutex = factory->AllocateMutex(); > +} > + > +inline void toku_external_mutex_lock(toku_external_mutex_t *mutex) { > + (*mutex)->Lock(); > +} > + > +inline int toku_external_mutex_trylock(toku_external_mutex_t *mutex) { > + (*mutex)->Lock(); > + return 0; > +} > + > +inline void toku_external_mutex_unlock(toku_external_mutex_t *mutex) { > + (*mutex)->UnLock(); > +} > + > +inline void toku_external_mutex_destroy(toku_external_mutex_t *mutex) { > + mutex->reset(); // this will destroy the managed object > +} > + > diff --git a/utilities/transactions/transaction_lock_mgr.cc b/utilities/transactions/transaction_lock_mgr.cc > index 56f3295fd..79164a962 100644 > --- a/utilities/transactions/transaction_lock_mgr.cc > +++ b/utilities/transactions/transaction_lock_mgr.cc > @@ -790,11 +790,6 @@ public: > bool releasing_locks_; > }; > > - > -void RangeLockMgr::KillLockWait(TransactionID txnid) { > - ltm.kill_waiter((void*)txnid); > -} > - > // Get a range lock on [start_key; end_key] range > Status RangeLockMgr::TryRangeLock(PessimisticTransaction* txn, > uint32_t column_family_id, > @@ -802,7 +797,7 @@ Status RangeLockMgr::TryRangeLock(PessimisticTransaction* txn, > const rocksdb::Slice &end_key, > bool exclusive) { > toku::lock_request request; > - request.create(); > + request.create(mutex_factory_); > DBT start_key_dbt, end_key_dbt; > > toku_fill_dbt(&start_key_dbt, start_key.data(), start_key.size()); > @@ -810,6 +805,8 @@ Status RangeLockMgr::TryRangeLock(PessimisticTransaction* txn, > > // Use the txnid as "extra" in the lock_request. Then, KillLockWait() > // will be able to use kill_waiter(txn_id) to kill the wait if needed > + // TODO: KillLockWait is gone and we are no longer using > + // locktree::kill_waiter call. Do we need this anymore? > TransactionID wait_txn_id = txn->GetID(); > > request.set(lt, (TXNID)txn, &start_key_dbt, &end_key_dbt, > @@ -1004,8 +1001,10 @@ int RangeLockMgr::compare_dbt_endpoints(__toku_db*, void *arg, > } > > > -RangeLockMgr::RangeLockMgr(TransactionDB* txn_db) : my_txn_db(txn_db) { > - ltm.create(on_create, on_destroy, on_escalate, NULL); > +RangeLockMgr::RangeLockMgr(TransactionDB* txn_db, > + std::shared_ptr<TransactionDBMutexFactory> mutex_factory) : > + my_txn_db(txn_db), mutex_factory_(mutex_factory) { > + ltm.create(on_create, on_destroy, on_escalate, NULL, mutex_factory_); > lt= nullptr; > cmp_initialized_= false; > } > diff --git a/utilities/transactions/transaction_lock_mgr.h b/utilities/transactions/transaction_lock_mgr.h > index 0c3284519..9f73e5a07 100644 > --- a/utilities/transactions/transaction_lock_mgr.h > +++ b/utilities/transactions/transaction_lock_mgr.h > @@ -215,11 +215,10 @@ class RangeLockMgr : > void UnLock(PessimisticTransaction* txn, uint32_t column_family_id, > const std::string& key, Env* env) override ; > > - RangeLockMgr(TransactionDB* txn_db); > + RangeLockMgr(TransactionDB* txn_db, > + std::shared_ptr<TransactionDBMutexFactory> mutex_factory); > ~RangeLockMgr(); > > - void KillLockWait(TransactionID txnid); > - > int set_max_lock_memory(size_t max_lock_memory) override > { > return ltm.set_max_lock_memory(max_lock_memory); > @@ -251,6 +250,7 @@ class RangeLockMgr : > compare_endpoints_func compare_endpoints; > > TransactionDB* my_txn_db; > + std::shared_ptr<TransactionDBMutexFactory> mutex_factory_; > > static int compare_dbt_endpoints(__toku_db*, void *arg, const DBT *a_key, const DBT *b_key); > > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

2 1

Re: [Maria-developers] [Commits] test
by Roberto Spadim 12 Mar '19

12 Mar '19

noise ignored =) Em seg, 11 de mar de 2019 às 15:11, Faustin Lammler <faustin(a)mariadb.org> escreveu: > Hi, > this is a test, please do not respond. > > And sorry for the noise. > > -- > Faustin Lammler > MariaDB Foundation > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- Roberto Spadim SPAEmpresarial - Software ERP Eng. Automação e Controle

1 0

[Maria-developers] MariaDB-10.4 / Galera and GTIDs - aka we now have two different GTID implementations
by Daniel Black 04 Mar '19

04 Mar '19

Hi, I was excited to come across the Galera-4 release notes and blog. http://galeracluster.com/2019/02/galera-cluster-4-available-for-use-in-the-… It was good to see causal read functions that I'd hoped for in https://jira.mariadb.org/browse/MDEV-10715. WSREP_LAST_WRITTEN_GTID/WSREP_LAST_SEEN_GTID/ WSREP_SYNC_WAIT_UPTO_GTID functions which seemed to mirror existing functions like MASTER_GTID_WAIT and GTID variables. The reason these are different names is because a MariaDB GTID is different from a Galera one. *Having two GTID implementations is insane*. To state the obvious from a user point of view, we've now got two Global Transaction Identification implementations. Application and framework writers like Django/Drupal now have to handle now try to handle consistently with 00000000-0000-0000-0000-000000000000:25 for Galera and 0-1-33 for MariaDB async gtid replication. This is not just a format string difference. The an implementation requires a disparate set of functions to implement when they could of been consolidated. A mixed topology setup could be hidden from the application implementation if the same functions where used. There is/was also a option/proposal that had MariaDB gtid in the protocol save a round trip retrieval of a GTID - quite useful. There is also a bunch of documentation that refers to GTID and its going to absolutely horrible to try to keep the concepts separated as you try to configure a system. An issue like MDEV-10715, the most voted for and watched open issue in JIRA, (and related MDEV-14153), should of had a very priority than this when the implementation from Codership was been merged. This shoehorned in implementation from Codership is a very poor fit. Giving MDEV-10715 a critical priority and a 10.5 target is fiction when there isn't going to be a new galera wsrep api implementation for a number of years. An extra field in the protocol is all it would of taken (comment in MDEV-10715 from 8 months ago). 10.4.3 is out (really an RC?), the galera code got merge in 10.4.2, and the source code of galera-4 isn't even released. Hopefully the tight relationship between MariaDB Corporation and Codership is enough to get this fixed because from an architectural point of view, because the current implementation shouldn't of been merged (contractual arrangement or not). Developers, MariaDB and Codership you've lost track of your user base because the feedback, that is readily available, is ignored. Sadly, Daniel

2 1

Re: [Maria-developers] [Commits] ce29dfac72e: MDEV-18601: Can't create table with ENCRYPTED=DEFAULT when innodb_default_encryption_key_id!=1
by Marko Mäkelä 01 Mar '19

01 Mar '19

Greetings from MariaDB Openworks in New York City. I just discussed this with Monty. His idea is that "don’t care" attributes should be ignored, or at most a warning could be issued for them. This allows ALTER TABLE from one storage engine to another while preserving attributes that only matter for some storage engines. Based on that basic principle, we should ignore the encryption_key_id attribute (whether or not it was explicitly specified by the user, or inherited from the session variable innodb_default_encryption_key_id). We could issue a warning if innodb_default_encryption_key_id is assigned to an invalid value, or an invalid encryption_key_id is being implicitly or explicitly passed to CREATE TABLE or ALTER TABLE. On Thu, Feb 21, 2019 at 2:53 PM jan <jan.lindstrom(a)mariadb.com> wrote: [snip] > diff --git a/mysql-test/suite/encryption/r/innodb-encryption-alter.result b/mysql-test/suite/encryption/r/innodb-encryption-alter.result > index 5245d1da7d0..038e2a2fd47 100644 > --- a/mysql-test/suite/encryption/r/innodb-encryption-alter.result > +++ b/mysql-test/suite/encryption/r/innodb-encryption-alter.result > @@ -3,24 +3,18 @@ SET GLOBAL innodb_file_per_table = ON; > SET GLOBAL innodb_encrypt_tables = ON; > SET GLOBAL innodb_encryption_threads = 4; > CREATE TABLE t1 (pk INT PRIMARY KEY AUTO_INCREMENT, c VARCHAR(256)) ENGINE=INNODB ENCRYPTED=NO ENCRYPTION_KEY_ID=4; > -Warnings: > -Warning 140 InnoDB: Ignored ENCRYPTION_KEY_ID 4 when encryption is disabled > DROP TABLE t1; > set innodb_default_encryption_key_id = 99; > +Warnings: > +Warning 1210 InnoDB: Ignored innodb_default_encryption_key_id=99 as it is not available in the key file. Using default=1. I think that the warning during CREATE TABLE was appropriate and should not be removed. A warning for the SET is appropriate, but I do not like the "Using default=1" part. I think that we should keep the invalid attribute, and throw an error or warning during CREATE TABLE as appropriate. > CREATE TABLE t1 (pk INT PRIMARY KEY AUTO_INCREMENT, c VARCHAR(256)) ENGINE=INNODB; > -ERROR HY000: Can't create table `test`.`t1` (errno: 140 "Wrong create options") > SHOW WARNINGS; > Level Code Message > -Warning 140 InnoDB: ENCRYPTION_KEY_ID 99 not available > -Error 1005 Can't create table `test`.`t1` (errno: 140 "Wrong create options") > -Warning 1030 Got error 140 "Wrong create options" from storage engine InnoDB I believe that this CREATE TABLE should continue to be disallowed, because innodb_encrypt_tables=ON and innodb_default_encryption_key_id=9 should imply ENCRYPTED=YES and ENCRYPTION_KEY_ID=99, and the key 99 is not available. Even if you think that it should be allowed (I find it hard to agree with that), I believe that we should at least be issuing warnings. [snip] > SET GLOBAL innodb_encrypt_tables=OFF; > CREATE TABLE t1 (a int not null primary key) engine=innodb; > ALTER TABLE t1 ENCRYPTION_KEY_ID=4; > -ERROR HY000: Table storage engine 'InnoDB' does not support the create option 'ENCRYPTION_KEY_ID' > -SHOW WARNINGS; > -Level Code Message > -Warning 140 InnoDB: innodb_encrypt_tables=OFF only allows ENCRYPTION_KEY_ID=1 > -Error 1478 Table storage engine 'InnoDB' does not support the create option 'ENCRYPTION_KEY_ID' Here, it is correct that we should not throw a warning, because innodb_encrypt_tables=OFF causes the encryption_key_id to not matter. But we could issue a warning if ENCRYPTION_KEY_ID is not available. I am going to push a revised patch to the bb-10.1-marko branch and ask for your review. With best regards, Marko -- Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation

2 2

Re: [Maria-developers] 0608c273598: MDEV-15966: Behavior for TRUNCATE versioned table is not documented and not covered by tests
by Sergei Golubchik 01 Mar '19

01 Mar '19

Hi, Nikita! On Dec 26, Nikita Malyavin wrote: > revision-id: 0608c273598 (versioning-1.0.5-112-g0608c273598) > parent(s): 49d506cd1d7 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2018-09-05 04:42:17 +1000 > message: > > MDEV-15966: Behavior for TRUNCATE versioned table is not documented and not covered by tests > > * MDEV-15966: Behavior for TRUNCATE versioned table is not documented and not covered by tests > > * add error for truncation of versioned tables: `ER_TRUNCATE_ILLEGAL_VERS` > * extract reading `extra2` section in function `dd_read_extra2` > * update `dd_frm_type` and `ha_table_exists` signatures: add new parameter `is_versioned` > > test suites run: main, parts, versioning > > [fixes tempesta-tech/mariadb#261] I don't quite like that a small and simple function dd_frm_type(), which used to just read few first bytes of the frm file, gets more and more complex, growing into a complete frm-open method. So, perhaps, I'd consider changing TRUNCATE to open the table properly without all that just-read-few-bytes shortcuts. Or, may be, simply document that TRUNCATE works on versioning tables just as DROP+CREATE does. There is no logical reason why it shouldn't - if one can do SHOW CREATE TABLE and CREATE OR REPLACE, then TRUNCATE won't add any new functionality on top of that. That's the easiest and most logical "fix" to this bug. Agree? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 5

Re: [Maria-developers] df203d1daa1: MDEV-15990: REPLACE on a precise-versioned table returns duplicate key error (ER_DUP_ENTRY)
by Sergei Golubchik 28 Feb '19

28 Feb '19

Hi, Nikita! Sorry, I don't understand what are you fixing and why :( Could you please elaborate? See few minor comments below and the main question at the very end. On Dec 26, Nikita Malyavin wrote: > revision-id: df203d1daa1 (versioning-1.0.5-42-gdf203d1daa1) > parent(s): 5e89a0dc25a > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2018-07-21 00:14:57 +1000 > message: > > MDEV-15990: REPLACE on a precise-versioned table returns duplicate key error (ER_DUP_ENTRY) > > * handle zero value in `row_start` `trx_id` > * refuse to do more than one versioned insert on the same transaction and same row > * generalize versioned cases with and without triggers > > [closes tempesta-tech/mariadb#517] > [closes tempesta-tech/mariadb#520] > > diff --git a/mysql-test/suite/versioning/r/replace.result b/mysql-test/suite/versioning/r/replace.result > --- a/mysql-test/suite/versioning/r/replace.result > +++ b/mysql-test/suite/versioning/r/replace.result > @@ -7,10 +7,31 @@ period for system_time (row_start, row_end) > ) with system versioning; > insert t values (1, 2); > replace t values (1, 3); > -select *, current_row(row_end) as current from t for system_time all order by x; > -id x current > -1 2 0 > -1 3 1 > +replace t values (1, 3); > +# MDEV-15990: REPLACE on a precise-versioned table > +# returns duplicate key error (ER_DUP_ENTRY) > +replace t values (1, 3), (1, 30), (1, 40); > +create table log(s varchar(3)); > +create trigger tr1del before delete on t > +for each row insert into log values('DEL'); > +replace t values (1, 4), (1, 40), (1, 400); > +select *, check_row(row_start, row_end), row_start != 0 as start_nonzero > +from t for system_time all order by x, row_end; > +id x check_row(row_start, row_end) start_nonzero > +1 2 HISTORICAL ROW 1 > +1 3 HISTORICAL ROW 1 > +1 3 HISTORICAL ROW 1 > +1 40 HISTORICAL ROW 1 > +1 400 CURRENT ROW 1 > +select * from log; > +s > +DEL > +DEL > +DEL better to insert OLD.x too, to be able to see where the DEL comes from > +# ensure that row_start is not duplicated > +select count(row_start) as empty from t for system_time all > +group by row_start having count(row_start) > 1; > +empty > drop table t; > create table t ( > id int unique, > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -1969,23 +1952,22 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info) > TRG_ACTION_BEFORE, TRUE)) > goto before_trg_err; > > - if (!table->versioned(VERS_TIMESTAMP)) > + if (table->versioned(VERS_TRX_ID) && table->vers_write) > + { can table->vers_write be false here? I suspect it can never be false for REPLACE, so I'd write it as if (table->versioned(VERS_TRX_ID)) { DBUG_ASSERT(table->vers_write); > + bitmap_set_bit(table->write_set, table->vers_start_field()->field_index); > + table->vers_start_field()->store(0, false); > + } > + > + if (!table->versioned()) > error= table->file->ha_delete_row(table->record[1]); > else > - { > - store_record(table, record[2]); > - restore_record(table, record[1]); > - table->vers_update_end(); > error= table->file->ha_update_row(table->record[1], > table->record[0]); > - restore_record(table, record[2]); > - } > - if (unlikely(error)) > + if (unlikely(error && error != HA_ERR_RECORD_IS_THE_SAME)) > goto err; > - if (!table->versioned(VERS_TIMESTAMP)) > + if (likely(!error)) > info->deleted++; > - else > - info->updated++; > + error= 0; > if (!table->file->has_transactions()) > thd->transaction.stmt.modified_non_trans_table= TRUE; > if (table->triggers && > @@ -1995,6 +1977,17 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info) > trg_error= 1; > goto ok_or_after_trg_err; > } > + if (table->versioned(VERS_TIMESTAMP) && table->vers_write) same > + { > + store_record(table, record[2]); > + error = vers_insert_history_row(table); why are you doing that (insert + goto after_trg_n_copied_inc) instead of going the normal REPLACE code path? > + restore_record(table, record[2]); > + if (unlikely(error)) > + goto err; > + } > + if (table->versioned()) > + goto after_trg_n_copied_inc; > + > /* Let us attempt do write_row() once more */ > } > } > diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc > index db755077e0b..139c43ced38 100644 > --- a/storage/innobase/handler/ha_innodb.cc > +++ b/storage/innobase/handler/ha_innodb.cc > @@ -8916,6 +8916,19 @@ ha_innobase::update_row( > upd_t* uvect = row_get_prebuilt_update_vector(m_prebuilt); > ib_uint64_t autoinc; > > + bool force_insert= table->versioned_write(VERS_TRX_ID) > + && table->vers_start_id() == 0; hmm, so you're using vers_start_id() == 0 as a special hack to signal REPLACE to the storage engine... > + > + if (force_insert) { > + table->vers_start_field()->store(trx->id); > + > + Field *start= table->vers_start_field(); > + trx_id_t old_trx_id= uint8korr(start->ptr_in_record(old_row)); > + > + /* avoid double versioned insert on the same transaction and same row */ > + force_insert= (old_trx_id != trx->id); okay, I'm confused. What was wrong in the old code? Normally REPLACE is DELETE + INSERT, why was it not enough for InnoDB and you needed some special logic here to handle the REPLACE case? > + } > + > /* Build an update vector from the modified fields in the rows > (uses m_upd_buf of the handle) */ > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

Re: [Maria-developers] 773d24d457a: MDEV-15951 system versioning by trx id doesn't work with partitioning
by Sergei Golubchik 28 Feb '19

28 Feb '19

Hi, Nikita! On Dec 25, Nikita Malyavin wrote: > revision-id: 773d24d457a (versioning-1.0.6-100-g773d24d457a) > parent(s): b26736cdb11 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2018-12-21 03:13:47 +1000 > message: > > MDEV-15951 system versioning by trx id doesn't work with partitioning > > Fix partitioning for trx_id-versioned tables. > `partition by hash`, `range` and others now work. > `partition by system_time` is forbidden. > Currently we cannot use row_start and row_end in `partition by`, because > insertion of versioned field is done by engine's handler, as well as > row_start/row_end's value set up, which is a transaction id -- so it's > also forbidden. > > The drawback is that it's now impossible to use `partition by key()` > without parameters for such tables, because it references row_start and > row_end implicitly. > > * add handler::vers_can_native() > * drop Table_scope_and_contents_source_st::vers_native() > * drop partition_element::find_engine_flag as unused > * forbid versioning partitioning for trx_id as not supported > * adopt vers tests for trx_id partitioning > * forbid any row_end referencing in `partition by` clauses, > including implicit `by key()` > > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > index 9e6c333d3c9..48bcb828d56 100644 > --- a/sql/sql_partition.cc > +++ b/sql/sql_partition.cc > @@ -341,7 +341,16 @@ static bool set_up_field_array(THD *thd, TABLE *table, > while ((field= *(ptr++))) > { > if (field->flags & GET_FIXED_FIELDS_FLAG) > + { > + if (table->versioned(VERS_TRX_ID) > + && unlikely(field->flags & VERS_SYSTEM_FIELD)) > + { > + my_error(ER_VERS_NOT_SUPPORTED, MYF(0), > + "Using ROW START/END for patitioning", "transactional"); This is incorrect usage of error message parameters. The error message can be translated, parameters can be reordered - you should not use English as parameter values. Consider: transactional cистемно-версионированные таблицы не поддерживают Using ROW START/END for patitioning But looks like ER_VERS_NOT_SUPPORTED is not used anywhere at the moment. So you can change it to be anything you want, even remove all parameters: Transactional system versioned tables do not support partitioning by ROW START or ROW END > + DBUG_RETURN(TRUE); > + } > num_fields++; > + } > } > if (unlikely(num_fields > MAX_REF_PARTS)) > { > diff --git a/sql/table.cc b/sql/table.cc > index ce7a34a8fe2..0c2ff1eea5a 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -1776,7 +1776,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > goto err; > DBUG_PRINT("info", ("Columns with system versioning: [%d, %d]", row_start, row_end)); > versioned= VERS_TIMESTAMP; > - vers_can_native= plugin_hton(se_plugin)->flags & HTON_NATIVE_SYS_VERSIONING; > + vers_can_native= handler_file->vers_can_native(thd); Interesting. How does that work? Your ha_partition::vers_can_native() is bool can= !thd->lex->part_info || thd->lex->part_info->part_type != VERSIONING_PARTITION; for (uint i= 0; i < m_tot_parts && can; i++) can= can && m_file[i]->vers_can_native(thd); return can; But it doesn't look like thd->lex->part_info->part_type is initialized at this point in time. Try to add to your test case flush tables; select * from t1; Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

Re: [Maria-developers] bc7d600a27f: MDEV-15951 system versioning by trx id doesn't work with partitioning
by Sergei Golubchik 28 Feb '19

28 Feb '19

Hi, Nikita! On Dec 21, Nikita Malyavin wrote: > > > @@ -4938,6 +4938,7 @@ int ha_create_table(THD *thd, const char *path, > > > !create_info->tmp_table(); > > > > > > share.frm_image= frm; > > > + share.orig_table_name= table_name; > > > > why? if orig_table_name is NULL, error_table_name() will use table_name, > > so there's never need to make orig_table_name==table_name. > > > > and I don't understand all your other manipulations with > > orig_table_name. Could you please explain that? > > It affects ALTER TABLE error messages. table_name will contain temporary name, > and error_table_name() will output "(temporary)", which is rather inconvenient. > AFAICS, orig_table_name purpose is exactly for fixing that, but some > corner cases were not covered to this moment. I don't understand that. You've made a lot of changes in many places passing orig table name around, it's difficult to see what corner cases you were trying to fix. Could you please fix it in a separate commit? With tests and an explanation in a commit comment, describing what exactly corner cases were not covered. > > > // open an frm image > > > if (share.init_from_binary_frm_image(thd, write_frm_now, Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1