Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Konstantin Osipov <kostja@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [RFC PATCH 01/23] vinyl: do not turn REPLACE into INSERT when processing DML request
Date: Wed, 11 Jul 2018 13:25:23 +0300	[thread overview]
Message-ID: <20180711102523.ocf5hbfuoovuzadq@esperanza> (raw)
In-Reply-To: <20180711075736.jupakwgkxgjikcvr@esperanza>

On Wed, Jul 11, 2018 at 10:57:36AM +0300, Vladimir Davydov wrote:
> On Tue, Jul 10, 2018 at 09:39:09PM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [18/07/10 15:22]:
> > > On Tue, Jul 10, 2018 at 03:15:27PM +0300, Konstantin Osipov wrote:
> > > > * Vladimir Davydov <vdavydov.dev@gmail.com> [18/07/08 22:52]:
> > > > > Since in presence of secondary indexes we read the primary index when
> > > > > processing a REPLACE request anyway, we turn it into INSERT if no tuple
> > > > > matching the new tuple is found so that INSERT+DELETE gets annihilated
> > > > > on compaction.
> > > > > 
> > > > > However, in the scope of #2129 we are planning to optimize the read out
> > > > > so that this transformation won't be possible anymore. So let's remove
> > > > > it now.
> > > > 
> > > > Ugh. What if we deal with a space which has a unique secondary
> > > > key, so optimization B is not applicable. You removed optimization
> > > > A for all spaces. 
> > > 
> > > The optimization works even if secondary indexes are unique.
> > 
> > Only for deletes. But not for replace/upsert.
> 
> It works for REPLACE as well.
> 
> > 
> > I thought you had the opinion that this optimization is
> > controversial - so ideally it should be optional. What do you
> > think now? Or you generally think that it's so controversial it's
> > not worth it, and if we do, we should not make it optional?
> 
> I made it mandatory for the sake of testing. We might want to make it
> optional one day - it isn't going to be a problem.

I removed this patch from the series and updated the branch. The only
patch that required manual rebase was

  [RFC PATCH 05/23] vinyl: fold vy_replace_one and vy_replace_impl

Posting the new version here:

From e0782b9d1b1d2bfc68aa2d460603281caaaaca17 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Sat, 23 Jun 2018 21:47:25 +0300
Subject: [PATCH] vinyl: fold vy_replace_one and vy_replace_impl

There's no point in separating REPLACE path between the cases when
the space has secondary indexes and when it only has the primary
index, because they are quite similar. Let's fold vy_replace_one
and vy_replace_impl into vy_replace to remove code duplication.

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index c95d05c1..44f8afaa 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1540,160 +1540,6 @@ vy_insert_secondary(struct vy_env *env, struct vy_tx *tx, struct space *space,
 }
 
 /**
- * Execute REPLACE in a space with a single index, possibly with
- * lookup for an old tuple if the space has at least one
- * on_replace trigger.
- * @param env     Vinyl environment.
- * @param tx      Current transaction.
- * @param space   Space in which replace.
- * @param request Request with the tuple data.
- * @param stmt    Statement for triggers is filled with old
- *                statement.
- *
- * @retval  0 Success.
- * @retval -1 Memory error OR duplicate key error OR the primary
- *            index is not found OR a tuple reference increment
- *            error.
- */
-static inline int
-vy_replace_one(struct vy_env *env, struct vy_tx *tx, struct space *space,
-	       struct request *request, struct txn_stmt *stmt)
-{
-	(void)env;
-	assert(tx != NULL && tx->state == VINYL_TX_READY);
-	struct vy_lsm *pk = vy_lsm(space->index[0]);
-	assert(pk->index_id == 0);
-	if (tuple_validate_raw(pk->mem_format, request->tuple))
-		return -1;
-	struct tuple *new_tuple =
-		vy_stmt_new_replace(pk->mem_format, request->tuple,
-				    request->tuple_end);
-	if (new_tuple == NULL)
-		return -1;
-	/**
-	 * If the space has triggers, then we need to fetch the
-	 * old tuple to pass it to the trigger.
-	 */
-	if (stmt != NULL && !rlist_empty(&space->on_replace)) {
-		if (vy_get(pk, tx, vy_tx_read_view(tx),
-			   new_tuple, &stmt->old_tuple) != 0)
-			goto error_unref;
-	}
-	if (vy_tx_set(tx, pk, new_tuple))
-		goto error_unref;
-
-	if (stmt != NULL)
-		stmt->new_tuple = new_tuple;
-	else
-		tuple_unref(new_tuple);
-	return 0;
-
-error_unref:
-	tuple_unref(new_tuple);
-	return -1;
-}
-
-/**
- * Execute REPLACE in a space with multiple indexes and lookup for
- * an old tuple, that should has been set in \p stmt->old_tuple if
- * the space has at least one on_replace trigger.
- * @param env     Vinyl environment.
- * @param tx      Current transaction.
- * @param space   Vinyl space.
- * @param request Request with the tuple data.
- * @param stmt    Statement for triggers filled with old
- *                statement.
- *
- * @retval  0 Success
- * @retval -1 Memory error OR duplicate key error OR the primary
- *            index is not found OR a tuple reference increment
- *            error.
- */
-static inline int
-vy_replace_impl(struct vy_env *env, struct vy_tx *tx, struct space *space,
-		struct request *request, struct txn_stmt *stmt)
-{
-	assert(tx != NULL && tx->state == VINYL_TX_READY);
-	struct tuple *old_stmt = NULL;
-	struct tuple *new_stmt = NULL;
-	struct tuple *delete = NULL;
-	struct vy_lsm *pk = vy_lsm_find(space, 0);
-	if (pk == NULL) /* space has no primary key */
-		return -1;
-	/* Primary key is dumped last. */
-	assert(!vy_is_committed_one(env, pk));
-	assert(pk->index_id == 0);
-	if (tuple_validate_raw(pk->mem_format, request->tuple))
-		return -1;
-	new_stmt = vy_stmt_new_replace(pk->mem_format, request->tuple,
-				       request->tuple_end);
-	if (new_stmt == NULL)
-		return -1;
-
-	/* Get full tuple from the primary index. */
-	if (vy_get(pk, tx, vy_tx_read_view(tx), new_stmt, &old_stmt) != 0)
-		goto error;
-
-	if (old_stmt == NULL) {
-		/*
-		 * We can turn REPLACE into INSERT if the new key
-		 * does not have history.
-		 */
-		vy_stmt_set_type(new_stmt, IPROTO_INSERT);
-	}
-
-	/*
-	 * Replace in the primary index without explicit deletion
-	 * of the old tuple.
-	 */
-	if (vy_tx_set(tx, pk, new_stmt) != 0)
-		goto error;
-
-	if (space->index_count > 1 && old_stmt != NULL) {
-		delete = vy_stmt_new_surrogate_delete(pk->mem_format, old_stmt);
-		if (delete == NULL)
-			goto error;
-	}
-
-	/* Update secondary keys, avoid duplicates. */
-	for (uint32_t iid = 1; iid < space->index_count; ++iid) {
-		struct vy_lsm *lsm = vy_lsm(space->index[iid]);
-		if (vy_is_committed_one(env, lsm))
-			continue;
-		/*
-		 * Delete goes first, so if old and new keys
-		 * fully match, there is no look up beyond the
-		 * transaction index.
-		 */
-		if (old_stmt != NULL) {
-			if (vy_tx_set(tx, lsm, delete) != 0)
-				goto error;
-		}
-		if (vy_insert_secondary(env, tx, space, lsm, new_stmt) != 0)
-			goto error;
-	}
-	if (delete != NULL)
-		tuple_unref(delete);
-	/*
-	 * The old tuple is used if there is an on_replace
-	 * trigger.
-	 */
-	if (stmt != NULL) {
-		stmt->new_tuple = new_stmt;
-		stmt->old_tuple = old_stmt;
-	}
-	return 0;
-error:
-	if (delete != NULL)
-		tuple_unref(delete);
-	if (old_stmt != NULL)
-		tuple_unref(old_stmt);
-	if (new_stmt != NULL)
-		tuple_unref(new_stmt);
-	return -1;
-}
-
-/**
  * Check that the key can be used for search in a unique index
  * LSM tree.
  * @param  lsm        LSM tree for checking.
@@ -2316,18 +2162,86 @@ static int
 vy_replace(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
 	   struct space *space, struct request *request)
 {
+	assert(tx != NULL && tx->state == VINYL_TX_READY);
 	if (vy_is_committed(env, space))
 		return 0;
 	if (request->type == IPROTO_INSERT)
 		return vy_insert(env, tx, stmt, space, request);
 
-	if (space->index_count == 1) {
-		/* Replace in a space with a single index. */
-		return vy_replace_one(env, tx, space, request, stmt);
-	} else {
-		/* Replace in a space with secondary indexes. */
-		return vy_replace_impl(env, tx, space, request, stmt);
+	struct vy_lsm *pk = vy_lsm_find(space, 0);
+	if (pk == NULL)
+		return -1;
+	/* Primary key is dumped last. */
+	assert(!vy_is_committed_one(env, pk));
+
+	/* Validate and create a statement for the new tuple. */
+	if (tuple_validate_raw(pk->mem_format, request->tuple))
+		return -1;
+	stmt->new_tuple = vy_stmt_new_replace(pk->mem_format, request->tuple,
+					      request->tuple_end);
+	if (stmt->new_tuple == NULL)
+		return -1;
+	/*
+	 * Get the overwritten tuple from the primary index if
+	 * the space has on_replace triggers, in which case we
+	 * need to pass the old tuple to trigger callbacks, or
+	 * if the space has secondary indexes and so we need
+	 * the old tuple to delete it from them.
+	 */
+	if (space->index_count > 1 || !rlist_empty(&space->on_replace)) {
+		if (vy_get(pk, tx, vy_tx_read_view(tx),
+			   stmt->new_tuple, &stmt->old_tuple) != 0)
+			return -1;
+		if (stmt->old_tuple == NULL) {
+			/*
+			 * We can turn REPLACE into INSERT if the
+			 * new key does not have history.
+			 */
+			vy_stmt_set_type(stmt->new_tuple, IPROTO_INSERT);
+		}
+	}
+	/*
+	 * Replace in the primary index without explicit deletion
+	 * of the old tuple.
+	 */
+	if (vy_tx_set(tx, pk, stmt->new_tuple) != 0)
+		return -1;
+	if (space->index_count == 1)
+		return 0;
+	/*
+	 * Replace in secondary indexes with explicit deletion
+	 * of the old tuple, if any.
+	 */
+	int rc = 0;
+	struct tuple *delete = NULL;
+	if (stmt->old_tuple != NULL) {
+		delete = vy_stmt_new_surrogate_delete(pk->mem_format,
+						      stmt->old_tuple);
+		if (delete == NULL)
+			return -1;
+	}
+	for (uint32_t i = 1; i < space->index_count; i++) {
+		struct vy_lsm *lsm = vy_lsm(space->index[i]);
+		if (vy_is_committed_one(env, lsm))
+			continue;
+		/*
+		 * DELETE goes first, so if old and new keys
+		 * fully match, there is no look up beyond the
+		 * transaction write set.
+		 */
+		if (delete != NULL) {
+			rc = vy_tx_set(tx, lsm, delete);
+			if (rc != 0)
+				break;
+		}
+		rc = vy_insert_secondary(env, tx, space, lsm,
+					 stmt->new_tuple);
+		if (rc != 0)
+			break;
 	}
+	if (delete != NULL)
+		tuple_unref(delete);
+	return rc;
 }
 
 static int

  reply	other threads:[~2018-07-11 10:25 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-08 16:48 [RFC PATCH 02/23] vinyl: always get full tuple from pk after reading from secondary index Vladimir Davydov
2018-07-08 16:48 ` [RFC PATCH 00/23] vinyl: eliminate read on REPLACE/DELETE Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 01/23] vinyl: do not turn REPLACE into INSERT when processing DML request Vladimir Davydov
2018-07-10 12:15     ` Konstantin Osipov
2018-07-10 12:19       ` Vladimir Davydov
2018-07-10 18:39         ` Konstantin Osipov
2018-07-11  7:57           ` Vladimir Davydov
2018-07-11 10:25             ` Vladimir Davydov [this message]
2018-07-08 16:48   ` [RFC PATCH 03/23] vinyl: use vy_mem_iterator for point lookup Vladimir Davydov
2018-07-17 10:14     ` Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 04/23] vinyl: make point lookup always return the latest tuple version Vladimir Davydov
2018-07-10 16:19     ` Konstantin Osipov
2018-07-10 16:43       ` Vladimir Davydov
2018-07-11 16:33         ` Vladimir Davydov
2018-07-31 19:17           ` Konstantin Osipov
2018-07-08 16:48   ` [RFC PATCH 05/23] vinyl: fold vy_replace_one and vy_replace_impl Vladimir Davydov
2018-07-31 20:28     ` Konstantin Osipov
2018-07-08 16:48   ` [RFC PATCH 06/23] vinyl: fold vy_delete_impl Vladimir Davydov
2018-07-31 20:28     ` Konstantin Osipov
2018-07-08 16:48   ` [RFC PATCH 07/23] vinyl: refactor unique check Vladimir Davydov
2018-07-31 20:28     ` Konstantin Osipov
2018-07-08 16:48   ` [RFC PATCH 08/23] vinyl: check key uniqueness before modifying tx write set Vladimir Davydov
2018-07-31 20:34     ` Konstantin Osipov
2018-08-01 10:42       ` Vladimir Davydov
2018-08-09 20:26     ` Konstantin Osipov
2018-08-10  8:26       ` Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 09/23] vinyl: remove env argument of vy_check_is_unique_{primary,secondary} Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 10/23] vinyl: store full tuples in secondary index cache Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 11/23] xrow: allow to store flags in DML requests Vladimir Davydov
2018-07-31 20:36     ` Konstantin Osipov
2018-08-01 14:10       ` Vladimir Davydov
2018-08-17 13:34         ` Vladimir Davydov
2018-08-17 13:34           ` [PATCH 1/2] xrow: allow to store tuple metadata in request Vladimir Davydov
2018-08-17 13:34           ` [PATCH 2/2] vinyl: introduce statement flags Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 12/23] vinyl: do not pass region explicitly to write iterator functions Vladimir Davydov
2018-07-17 10:16     ` Vladimir Davydov
2018-07-31 20:38     ` Konstantin Osipov
2018-08-01 14:14       ` Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 13/23] vinyl: fix potential use-after-free in vy_read_view_merge Vladimir Davydov
2018-07-17 10:16     ` Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 14/23] test: unit/vy_write_iterator: minor refactoring Vladimir Davydov
2018-07-17 10:17     ` Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 15/23] vinyl: teach write iterator to return overwritten tuples Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 16/23] vinyl: allow to skip certain statements on read Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 17/23] vinyl: do not free pending tasks on shutdown Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 18/23] vinyl: store pointer to scheduler in struct vy_task Vladimir Davydov
2018-07-31 20:39     ` Konstantin Osipov
2018-07-08 16:48   ` [RFC PATCH 19/23] vinyl: rename some members of vy_scheduler and vy_task struct Vladimir Davydov
2018-07-31 20:40     ` Konstantin Osipov
2018-07-08 16:48   ` [RFC PATCH 20/23] vinyl: use cbus for communication between scheduler and worker threads Vladimir Davydov
2018-07-31 20:43     ` Konstantin Osipov
2018-08-01 14:26       ` Vladimir Davydov
2018-07-08 16:48   ` [RFC PATCH 21/23] vinyl: zap vy_scheduler::is_worker_pool_running Vladimir Davydov
2018-07-31 20:43     ` Konstantin Osipov
2018-07-08 16:48   ` [RFC PATCH 22/23] vinyl: rename vy_task::status to is_failed Vladimir Davydov
2018-07-31 20:44     ` Konstantin Osipov
2018-07-08 16:48   ` [RFC PATCH 23/23] vinyl: eliminate read on REPLACE/DELETE Vladimir Davydov
2018-07-13 10:53     ` Vladimir Davydov
2018-07-13 10:53       ` [PATCH 1/3] stailq: add stailq_insert function Vladimir Davydov
2018-07-15  7:02         ` Konstantin Osipov
2018-07-15 13:17           ` Vladimir Davydov
2018-07-15 18:40             ` Konstantin Osipov
2018-07-17 10:18         ` Vladimir Davydov
2018-07-13 10:53       ` [PATCH 2/3] vinyl: link all indexes of the same space Vladimir Davydov
2018-07-13 10:53       ` [PATCH 3/3] vinyl: generate deferred DELETEs on tx commit Vladimir Davydov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180711102523.ocf5hbfuoovuzadq@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=kostja@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [RFC PATCH 01/23] vinyl: do not turn REPLACE into INSERT when processing DML request' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox