From 4cd73b15d4b24c6d1dad5376d04f4a294f32bc61 Mon Sep 17 00:00:00 2001
From: Konstantin Osipov <kostja@tarantool.org>
Date: Wed, 26 Aug 2015 21:35:12 +0300
Subject: [PATCH] sophia: review fixes
---
src/box/alter.cc | 2 --
src/box/box.cc | 2 +-
src/box/engine.cc | 4 +--
src/box/engine.h | 17 ----------
src/box/index.cc | 18 ----------
src/box/index.h | 12 -------
src/box/memtx_bitset.h | 1 -
src/box/memtx_engine.cc | 73 ++++++++++++++++++----------------------
src/box/memtx_hash.h | 1 -
src/box/memtx_index.cc | 21 +++++++++++-
src/box/memtx_index.h | 14 ++++++++
src/box/memtx_rtree.h | 2 --
src/box/memtx_tree.h | 1 -
src/box/sophia_engine.cc | 3 +-
src/box/sophia_index.cc | 2 +-
src/box/space.cc | 11 ++++++
src/box/tuple.h | 4 +--
src/box/txn.cc | 10 ++----
src/box/txn.h | 18 +++-------
19 files changed, 90 insertions(+), 126 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 5c42af9146..456f02620b 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -903,8 +903,6 @@ AddIndex::alter(struct alter_space *alter)
* added to the index (insufficient number of fields,
* etc., the build is aborted.
*/
- new_index->beginBuild();
- new_index->endBuild();
/* Build the new index. */
struct tuple *tuple;
struct tuple_format *format = alter->new_space->format;
diff --git a/src/box/box.cc b/src/box/box.cc
index 94457db85d..3288794d37 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -500,7 +500,7 @@ box_on_cluster_join(const tt_uuid *server_uuid)
{
/** Find the largest existing server id. */
struct space *space = space_cache_find(BOX_CLUSTER_ID);
- class MemtxIndex *index = (MemtxIndex *)index_find(space, 0);
+ class MemtxIndex *index = index_find_system(space, 0);
struct iterator *it = index->position();
index->initIterator(it, ITER_LE, NULL, 0);
struct tuple *tuple = it->next(it);
diff --git a/src/box/engine.cc b/src/box/engine.cc
index 07f7668c35..a3b6100cff 100644
--- a/src/box/engine.cc
+++ b/src/box/engine.cc
@@ -148,9 +148,7 @@ Handler::executeSelect(struct txn* /* txn */, struct space *space,
key_validate(index->key_def, type, key, part_count);
struct iterator *it = index->allocIterator();
- auto it_guard = make_scoped_guard([=] {
- it->free(it);
- });
+ IteratorGuard guard(it);
index->initIterator(it, type, key, part_count);
struct tuple *tuple;
diff --git a/src/box/engine.h b/src/box/engine.h
index c570610dc0..33ca816485 100644
--- a/src/box/engine.h
+++ b/src/box/engine.h
@@ -39,17 +39,6 @@ class Relay;
enum engine_flags {
ENGINE_CAN_BE_TEMPORARY = 1,
- /**
- * Identifies that engine can handle changes
- * of primary key during update.
- * During update operation user could change primary
- * key of a tuple, which is prohibited, to avoid funny
- * effects during replication. Some engines can
- * track down this situation and abort the operation;
- * such engines should set this flag.
- * If the flag is not set, the server will verify
- * that the primary key is not changed.
- */
ENGINE_AUTO_CHECK_UPDATE = 2,
};
@@ -307,12 +296,6 @@ engine_can_be_temporary(uint32_t flags)
return flags & ENGINE_CAN_BE_TEMPORARY;
}
-static inline bool
-engine_auto_check_update(uint32_t flags)
-{
- return flags & ENGINE_AUTO_CHECK_UPDATE;
-}
-
static inline uint32_t
engine_id(Handler *space)
{
diff --git a/src/box/index.cc b/src/box/index.cc
index 48624be9b9..4d4a3ee94a 100644
--- a/src/box/index.cc
+++ b/src/box/index.cc
@@ -148,24 +148,6 @@ Index::Index(struct key_def *key_def_arg)
sc_version(::sc_version)
{}
-void
-Index::beginBuild()
-{}
-
-void
-Index::reserve(uint32_t /* size_hint */)
-{}
-
-void
-Index::buildNext(struct tuple *tuple)
-{
- replace(NULL, tuple, DUP_INSERT);
-}
-
-void
-Index::endBuild()
-{}
-
Index::~Index()
{
key_def_delete(key_def);
diff --git a/src/box/index.h b/src/box/index.h
index 26ff881e24..b00c4c3b23 100644
--- a/src/box/index.h
+++ b/src/box/index.h
@@ -170,18 +170,6 @@ class Index: public Object {
public:
virtual ~Index();
- /**
- * Two-phase index creation: begin building, add tuples, finish.
- */
- virtual void beginBuild();
- /**
- * Optional hint, given to the index, about
- * the total size of the index. If given,
- * is given after beginBuild().
- */
- virtual void reserve(uint32_t /* size_hint */);
- virtual void buildNext(struct tuple *tuple);
- virtual void endBuild();
virtual size_t size() const;
virtual struct tuple *min(const char *key, uint32_t part_count) const;
virtual struct tuple *max(const char *key, uint32_t part_count) const;
diff --git a/src/box/memtx_bitset.h b/src/box/memtx_bitset.h
index 1690693a5a..528c2fa91a 100644
--- a/src/box/memtx_bitset.h
+++ b/src/box/memtx_bitset.h
@@ -35,7 +35,6 @@
* @brief Index API wrapper for bitset_index
* @see bitset/index.h
*/
-#include "index.h"
#include "memtx_index.h"
#include "bitset/index.h"
diff --git a/src/box/memtx_engine.cc b/src/box/memtx_engine.cc
index cc8a40e405..5683c5444f 100644
--- a/src/box/memtx_engine.cc
+++ b/src/box/memtx_engine.cc
@@ -96,9 +96,9 @@ memtx_replace_no_keys(struct txn * /* txn */, struct space *space,
struct tuple * /* new_tuple */,
enum dup_replace_mode /* mode */)
{
- Index *index = index_find(space, 0);
- assert(index == NULL); /* not reached. */
- (void) index;
+ Index *index = index_find(space, 0);
+ assert(index == NULL); /* not reached. */
+ (void) index;
}
struct MemtxSpace: public Handler {
@@ -113,15 +113,15 @@ struct MemtxSpace: public Handler {
/* engine->close(this); */
}
virtual void executeReplace(struct txn*, struct space*,
- struct request*, struct port*);
+ struct request*, struct port*);
virtual void executeDelete(struct txn*, struct space *space,
- struct request *request, struct port *port);
+ struct request *request, struct port *port);
virtual void executeUpdate(struct txn*, struct space *space,
- struct request *request, struct port *port);
+ struct request *request, struct port *port);
virtual void executeUpsert(struct txn*, struct space *space,
- struct request *request, struct port *port);
+ struct request *request, struct port *port);
virtual void executeSelect(struct txn*, struct space *space,
- struct request *request, struct port *port);
+ struct request *request, struct port *port);
};
static inline enum dup_replace_mode
@@ -132,8 +132,8 @@ dup_replace_mode(uint32_t op)
void
MemtxSpace::executeReplace(struct txn *txn, struct space *space,
- struct request *request,
- struct port *port)
+ struct request *request,
+ struct port *port)
{
struct tuple *new_tuple = tuple_new(space->format, request->tuple,
request->tuple_end);
@@ -152,8 +152,8 @@ MemtxSpace::executeReplace(struct txn *txn, struct space *space,
void
MemtxSpace::executeDelete(struct txn *txn, struct space *space,
- struct request *request,
- struct port *port)
+ struct request *request,
+ struct port *port)
{
/* Try to find the tuple by unique key. */
Index *pk = index_find_unique(space, request->index_id);
@@ -166,7 +166,8 @@ MemtxSpace::executeDelete(struct txn *txn, struct space *space,
return;
}
TupleGuard old_guard(old_tuple);
- space->handler->replace(txn, space, old_tuple, NULL, DUP_REPLACE_OR_INSERT);
+ space->handler->replace(txn, space, old_tuple, NULL,
+ DUP_REPLACE_OR_INSERT);
txn_commit_stmt(txn);
/*
* Adding result to port must be after possible WAL write.
@@ -178,8 +179,8 @@ MemtxSpace::executeDelete(struct txn *txn, struct space *space,
void
MemtxSpace::executeUpdate(struct txn *txn, struct space *space,
- struct request *request,
- struct port *port)
+ struct request *request,
+ struct port *port)
{
/* Try to find the tuple by unique key. */
Index *pk = index_find_unique(space, request->index_id);
@@ -203,8 +204,6 @@ MemtxSpace::executeUpdate(struct txn *txn, struct space *space,
request->index_base);
TupleGuard guard(new_tuple);
space_validate_tuple(space, new_tuple);
- if (! engine_auto_check_update(space->handler->engine->flags))
- space_check_update(space, old_tuple, new_tuple);
space->handler->replace(txn, space, old_tuple, new_tuple, DUP_REPLACE);
txn_commit_stmt(txn);
/*
@@ -217,8 +216,8 @@ MemtxSpace::executeUpdate(struct txn *txn, struct space *space,
void
MemtxSpace::executeUpsert(struct txn *txn, struct space *space,
- struct request *request,
- struct port *port)
+ struct request *request,
+ struct port *port)
{
(void)port;
Index *pk = index_find_unique(space, request->index_id);
@@ -247,8 +246,6 @@ MemtxSpace::executeUpsert(struct txn *txn, struct space *space,
TupleGuard guard(new_tuple);
space_validate_tuple(space, new_tuple);
- if (!engine_auto_check_update(space->handler->engine->flags))
- space_check_update(space, old_tuple, new_tuple);
space->handler->replace(txn, space, old_tuple, new_tuple, DUP_REPLACE);
}
@@ -258,8 +255,8 @@ MemtxSpace::executeUpsert(struct txn *txn, struct space *space,
void
MemtxSpace::executeSelect(struct txn* /* txn */, struct space *space,
- struct request *request,
- struct port *port)
+ struct request *request,
+ struct port *port)
{
MemtxIndex *index = (MemtxIndex *)
index_find(space, request->index_id);
@@ -282,7 +279,6 @@ MemtxSpace::executeSelect(struct txn* /* txn */, struct space *space,
struct tuple *tuple;
while ((tuple = it->next(it)) != NULL) {
- TupleGuard tuple_gc(tuple);
if (offset > 0) {
offset--;
continue;
@@ -292,7 +288,7 @@ MemtxSpace::executeSelect(struct txn* /* txn */, struct space *space,
port_add_tuple(port, tuple);
}
if (! in_txn()) {
- /* no txn is created, so simply collect garbage here */
+ /* no txn is created, so simply collect garbage here */
fiber_gc();
}
}
@@ -343,7 +339,7 @@ memtx_replace_build_next(struct txn * /* txn */, struct space *space,
panic("Failed to commit transaction when loading "
"from snapshot");
}
- space->index[0]->buildNext(new_tuple);
+ ((MemtxIndex *) space->index[0])->buildNext(new_tuple);
tuple_ref(new_tuple);
}
@@ -412,7 +408,7 @@ memtx_end_build_primary_key(struct space *space, void *param)
space->handler->replace == memtx_replace_all_keys)
return;
- space->index[0]->endBuild();
+ ((MemtxIndex *) space->index[0])->endBuild();
space->handler->replace = memtx_replace_primary_key;
}
@@ -453,8 +449,7 @@ MemtxEngine::MemtxEngine()
m_checkpoint(0),
m_state(MEMTX_INITIALIZED)
{
- flags = ENGINE_CAN_BE_TEMPORARY |
- ENGINE_AUTO_CHECK_UPDATE;
+ flags = ENGINE_CAN_BE_TEMPORARY;
}
/**
@@ -485,9 +480,7 @@ recover_snap(struct recovery_state *r)
int64_t signature = vclock_sum(res);
struct xlog *snap = xlog_open(&r->snap_dir, signature);
- auto guard = make_scoped_guard([=]{
- xlog_close(snap);
- });
+ auto guard = make_scoped_guard([=]{ xlog_close(snap); });
/* Save server UUID */
r->server_uuid = snap->server_uuid;
@@ -533,17 +526,17 @@ memtx_add_primary_key(struct space *space, enum memtx_recovery_state state)
panic("can't create a new space before snapshot recovery");
break;
case MEMTX_READING_SNAPSHOT:
- space->index[0]->beginBuild();
+ ((MemtxIndex *) space->index[0])->beginBuild();
space->handler->replace = memtx_replace_build_next;
break;
case MEMTX_READING_WAL:
- space->index[0]->beginBuild();
- space->index[0]->endBuild();
+ ((MemtxIndex *) space->index[0])->beginBuild();
+ ((MemtxIndex *) space->index[0])->endBuild();
space->handler->replace = memtx_replace_primary_key;
break;
case MEMTX_OK:
- space->index[0]->beginBuild();
- space->index[0]->endBuild();
+ ((MemtxIndex *) space->index[0])->beginBuild();
+ ((MemtxIndex *) space->index[0])->endBuild();
space->handler->replace = memtx_replace_all_keys;
break;
}
@@ -1112,7 +1105,7 @@ memtx_index_extent_alloc()
/* same error as in mempool_alloc */
tnt_raise(OutOfMemory, MEMTX_EXTENT_SIZE,
"mempool", "new slab")
- );
+ );
return mempool_alloc(&memtx_index_extent_pool);
}
@@ -1133,10 +1126,10 @@ void
memtx_index_extent_reserve(int num)
{
ERROR_INJECT(ERRINJ_INDEX_ALLOC,
- /* same error as in mempool_alloc */
+ /* same error as in mempool_alloc */
tnt_raise(OutOfMemory, MEMTX_EXTENT_SIZE,
"mempool", "new slab")
- );
+ );
while (memtx_index_num_reserved_extents < num) {
void *ext = mempool_alloc(&memtx_index_extent_pool);
*(void **)ext = memtx_index_reserved_extents;
diff --git a/src/box/memtx_hash.h b/src/box/memtx_hash.h
index a0ba6ad15f..c32746884c 100644
--- a/src/box/memtx_hash.h
+++ b/src/box/memtx_hash.h
@@ -31,7 +31,6 @@
* SUCH DAMAGE.
*/
-#include "index.h"
#include "memtx_index.h"
struct light_index_core;
diff --git a/src/box/memtx_index.cc b/src/box/memtx_index.cc
index 4698ea30fd..8c0da8c48d 100644
--- a/src/box/memtx_index.cc
+++ b/src/box/memtx_index.cc
@@ -37,6 +37,24 @@
#include "user_def.h"
#include "space.h"
+void
+MemtxIndex::beginBuild()
+{}
+
+void
+MemtxIndex::reserve(uint32_t /* size_hint */)
+{}
+
+void
+MemtxIndex::buildNext(struct tuple *tuple)
+{
+ replace(NULL, tuple, DUP_INSERT);
+}
+
+void
+MemtxIndex::endBuild()
+{}
+
struct tuple *
MemtxIndex::min(const char *key, uint32_t part_count) const
{
@@ -54,7 +72,8 @@ MemtxIndex::max(const char *key, uint32_t part_count) const
}
size_t
-MemtxIndex::count(enum iterator_type type, const char *key, uint32_t part_count) const
+MemtxIndex::count(enum iterator_type type, const char *key,
+ uint32_t part_count) const
{
if (type == ITER_ALL && key == NULL)
return size(); /* optimization */
diff --git a/src/box/memtx_index.h b/src/box/memtx_index.h
index 3a4b541eaa..8731b74b2a 100644
--- a/src/box/memtx_index.h
+++ b/src/box/memtx_index.h
@@ -30,6 +30,7 @@
* THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
+#include "index.h"
class MemtxIndex: public Index {
public:
@@ -51,6 +52,19 @@ class MemtxIndex: public Index {
m_position = allocIterator();
return m_position;
}
+
+ /**
+ * Two-phase index creation: begin building, add tuples, finish.
+ */
+ virtual void beginBuild();
+ /**
+ * Optional hint, given to the index, about
+ * the total size of the index. If given,
+ * is given after beginBuild().
+ */
+ virtual void reserve(uint32_t /* size_hint */);
+ virtual void buildNext(struct tuple *tuple);
+ virtual void endBuild();
protected:
/*
* Pre-allocated iterator to speed up the main case of
diff --git a/src/box/memtx_rtree.h b/src/box/memtx_rtree.h
index 40d99187af..66ff499d30 100644
--- a/src/box/memtx_rtree.h
+++ b/src/box/memtx_rtree.h
@@ -30,8 +30,6 @@
* THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
-
-#include "index.h"
#include "memtx_index.h"
#include <salad/rtree.h>
diff --git a/src/box/memtx_tree.h b/src/box/memtx_tree.h
index 1625dc7654..0b4c89dd84 100644
--- a/src/box/memtx_tree.h
+++ b/src/box/memtx_tree.h
@@ -31,7 +31,6 @@
* SUCH DAMAGE.
*/
-#include "index.h"
#include "memtx_index.h"
#include "memtx_engine.h"
diff --git a/src/box/sophia_engine.cc b/src/box/sophia_engine.cc
index ea63aae317..f9dccb3d0b 100644
--- a/src/box/sophia_engine.cc
+++ b/src/box/sophia_engine.cc
@@ -160,8 +160,7 @@ SophiaSpace::executeUpdate(struct txn *txn, struct space *space,
TupleGuard guard(new_tuple);
space_validate_tuple(space, new_tuple);
- if (! engine_auto_check_update(space->handler->engine->flags))
- space_check_update(space, old_tuple, new_tuple);
+ space_check_update(space, old_tuple, new_tuple);
index->replace_or_insert(new_tuple->data,
new_tuple->data + new_tuple->bsize,
diff --git a/src/box/sophia_index.cc b/src/box/sophia_index.cc
index b95b76491f..4feada466a 100644
--- a/src/box/sophia_index.cc
+++ b/src/box/sophia_index.cc
@@ -560,7 +560,7 @@ SophiaIndex::replace_or_insert(const char *tuple,
enum dup_replace_mode mode)
{
uint32_t size = tuple_end - tuple;
- const char *key = tuple_field_of(tuple, size, key_def->parts[0].fieldno);
+ const char *key = tuple_field_raw(tuple, size, key_def->parts[0].fieldno);
/* insert: ensure key does not exists */
if (mode == DUP_INSERT) {
struct tuple *found = findByKey(key);
diff --git a/src/box/space.cc b/src/box/space.cc
index abdfc8fcae..ffb9dcdb88 100644
--- a/src/box/space.cc
+++ b/src/box/space.cc
@@ -208,6 +208,17 @@ space_stat(struct space *sp)
return &space_stat;
}
+/**
+ * We do not allow changes of the primary key during
+ * update.
+ * The syntax of update operation allows the user to primary
+ * key of a tuple, which is prohibited, to avoid funny
+ * effects during replication. Some engines can
+ * track down this situation and abort the operation;
+ * such engines (memtx) don't use this function.
+ * Other engines can't do it, so they ask the server to
+ * verify that the primary key of the tuple has not changed.
+ */
void
space_check_update(struct space *space,
struct tuple *old_tuple,
diff --git a/src/box/tuple.h b/src/box/tuple.h
index 11bce2fc4c..c80554afac 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -285,7 +285,7 @@ tuple_field_count_validate(struct tuple_format *format, const char *data)
* @returns field data if field exists or NULL
*/
inline const char *
-tuple_field_of(const char *data, uint32_t bsize, uint32_t i)
+tuple_field_raw(const char *data, uint32_t bsize, uint32_t i)
{
const char *pos = data;
uint32_t size = mp_decode_array(&pos);
@@ -325,7 +325,7 @@ tuple_field_old(const struct tuple_format *format,
return tuple->data + field_map[idx];
}
}
- return tuple_field_of(tuple->data, tuple->bsize, i);
+ return tuple_field_raw(tuple->data, tuple->bsize, i);
}
/**
diff --git a/src/box/txn.cc b/src/box/txn.cc
index f33a2515e9..6c88ad545b 100644
--- a/src/box/txn.cc
+++ b/src/box/txn.cc
@@ -28,19 +28,13 @@
* THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
-#include "engine.h"
#include "txn.h"
-#include "box.h"
+#include "engine.h"
+#include "box.h" /* global recovery */
#include "tuple.h"
-#include "space.h"
-#include "main.h"
-#include "cluster.h"
#include "recovery.h"
#include <fiber.h>
#include "request.h" /* for request_name */
-#include "session.h"
-#include "port.h"
-#include "iproto_constants.h"
double too_long_threshold;
diff --git a/src/box/txn.h b/src/box/txn.h
index e9e11614be..9442981ad2 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -31,8 +31,6 @@
* SUCH DAMAGE.
*/
#include "space.h"
-#include "xrow.h"
-#include "iproto_constants.h"
#include "trigger.h"
#include "fiber.h"
@@ -127,18 +125,10 @@ txn_commit_stmt(struct txn *txn)
* of tuples in the trigger.
*/
struct txn_stmt *stmt = txn->stmt;
- if (stmt->row && stmt->space) {
- switch (stmt->row->type) {
- case IPROTO_INSERT:
- case IPROTO_REPLACE:
- case IPROTO_UPDATE:
- case IPROTO_DELETE: {
- if (! rlist_empty(&stmt->space->on_replace) &&
- stmt->space->run_triggers)
- trigger_run(&stmt->space->on_replace, txn);
- break;
- }
- }
+ if (stmt->row && stmt->space && !rlist_empty(&stmt->space->on_replace)
+ && stmt->space->run_triggers) {
+
+ trigger_run(&stmt->space->on_replace, txn);
}
if (txn->autocommit)
txn_commit(txn);
--
GitLab