Tarantool development patches archive
 help / color / mirror / Atom feed
From: Nikita Pettik <korablev@tarantool.org>
To: Aleksandr Lyapunov <alyapunov@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2] vinyl: restart read iterator in case of rolled back WAL
Date: Fri, 19 Jun 2020 12:24:12 +0000	[thread overview]
Message-ID: <20200619122412.GA19725@tarantool.org> (raw)
In-Reply-To: <4a83c68d-2dfd-23a8-97cd-5a429639dc3c@tarantool.org>

On 16 Jun 15:10, Aleksandr Lyapunov wrote:
> Thanks for the patch! See my 2 comments below:
> 
> On 6/1/20 7:46 PM, Nikita Pettik wrote:
> > +	if (vy_mem_tree_iterator_is_invalid(&src->mem_iterator.curr_pos)) {
> > +		assert(src->mem_iterator.curr.stmt == NULL);
> > +		return 1;
> > +	}
> I'm afraid that the iterator will not always be invalid in the given case.
> As I see, if a mem holds any older tuple with the same key (e.g. older
> version of that tuple), the restoration routine will find the older tuple
> with the non-invalid iterator.
> I also think that mem_restore must handle all the possible data
> changes by itself without concern of read_iterator.

You are likely to be right, but I followed suggestion below
to simplify resotration procedure.

> > -	if (vy_read_iterator_restore_mem(itr, &next) != 0)
> > +	int rc = vy_read_iterator_restore_mem(itr, &next);
> > +	if (rc < 0)
> >   		return -1;
> > +	if (rc > 0) {
> > +		vy_read_iterator_restore(itr);
> > +		goto restart;
> > +	}
> The read iterator was rewritten several times and still have at least
> several bugs. I think we should admit that we cannot support such a
> complicated solution. How about some stupid solution: if ANY change
> has been happened during yield - restart advancing?

Ok, I'm fine with it. Consider this change:

diff --git a/src/box/vy_read_iterator.c b/src/box/vy_read_iterator.c
index 62a8722d9..409796910 100644
--- a/src/box/vy_read_iterator.c
+++ b/src/box/vy_read_iterator.c
@@ -378,65 +378,6 @@ vy_read_iterator_scan_disk(struct vy_read_iterator *itr, uint32_t disk_src,
        return 0;
 }
 
-/**
- * Restore the position of the active in-memory tree iterator
- * after a yield caused by a disk read and update 'next'
- * if necessary.
- * @retval -1 In case of error (e.g. OOM);
- * @retval 0 Successful execution;
- * @retval 1 Restart of advance_iterator is required.
- */
-static NODISCARD int
-vy_read_iterator_restore_mem(struct vy_read_iterator *itr,
-                            struct vy_entry *next)
-{
-       int rc;
-       int cmp;
-       struct vy_read_src *src = &itr->src[itr->mem_src];
-
-       rc = vy_mem_iterator_restore(&src->mem_iterator,
-                                    itr->last, &src->history);
-       if (rc < 0)
-               return -1; /* memory allocation error */
-       if (rc == 0)
-               return 0; /* nothing changed */
-
-       if (vy_mem_tree_iterator_is_invalid(&src->mem_iterator.curr_pos)) {
-               assert(src->mem_iterator.curr.stmt == NULL);
-               return 1;
-       }
-       struct vy_entry entry = vy_history_last_stmt(&src->history);
-       cmp = vy_read_iterator_cmp_stmt(itr, entry, *next);
-       if (cmp > 0) {
-               /*
-                * Memory trees are append-only so if the
-                * source is not on top of the heap after
-                * restoration, it was not before.
-                */
-               assert(src->front_id < itr->front_id);
-               return 0;
-       }
-       if (cmp < 0) {
-               /*
-                * The new statement precedes the current
-                * candidate for the next key.
-                */
-               *next = entry;
-               itr->front_id++;
-       } else {
-               /*
-                * The new statement updates the next key.
-                * Make sure we don't read the old value
-                * from the cache while applying UPSERTs.
-                */
-               struct vy_read_src *cache_src = &itr->src[itr->cache_src];
-               if (cache_src->front_id == itr->front_id)
-                       vy_history_cleanup(&cache_src->history);
-       }
-       src->front_id = itr->front_id;
-       return 0;
-}
-
 static void
 vy_read_iterator_restore(struct vy_read_iterator *itr);
 
@@ -536,10 +477,8 @@ rescan_disk:
         * as it is owned exclusively by the current fiber so the only
         * source to check is the active in-memory tree.
         */
-       int rc = vy_read_iterator_restore_mem(itr, &next);
-       if (rc < 0)
-               return -1;
-       if (rc > 0) {
+       struct vy_mem_iterator *mem_itr = &itr->src[itr->mem_src].mem_iterator;
+       if (mem_itr->version != mem_itr->mem->version) {
                vy_read_iterator_restore(itr);
                goto restart;
        }

  reply	other threads:[~2020-06-19 12:24 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-01 16:46 Nikita Pettik
2020-06-01 17:40 ` Vladislav Shpilevoy
2020-06-19 12:34   ` Nikita Pettik
2020-06-20 16:33     ` Vladislav Shpilevoy
2020-06-16 12:10 ` Aleksandr Lyapunov
2020-06-19 12:24   ` Nikita Pettik [this message]
2020-06-19 12:42     ` Konstantin Osipov
2020-06-23  5:15       ` Aleksandr Lyapunov
2020-06-23 11:08         ` Konstantin Osipov
2020-06-19 13:01     ` Aleksandr Lyapunov
2020-06-24 13:41       ` Nikita Pettik
2020-06-20 16:33     ` Vladislav Shpilevoy

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=20200619122412.GA19725@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=alyapunov@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2] vinyl: restart read iterator in case of rolled back WAL' \
    /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