[tarantool-patches] Re: [PATCH v2 1/6] xlog: store prev vclock in xlog header

  • From: Vladimir Davydov <vdavydov.dev@xxxxxxxxx>
  • To: Konstantin Osipov <kostja@xxxxxxxxxxxxx>
  • Date: Thu, 5 Jul 2018 11:23:51 +0300

On Thu, Jul 05, 2018 at 09:52:41AM +0300, Konstantin Osipov wrote:

* Vladimir Davydov <vdavydov.dev@xxxxxxxxx> [18/06/29 19:49]:
This patch adds a new key to xlog header, PrevVclock, which contains the
vclock of the previous xlog file in the xlog directory. It is set by
xdir_create_xlog() to the last vclock in xdir::index. The new key is
only present in XLOG files (it doesn't make sense for SNAP or VYLOG
anyway). It will be used to make the check for xlog gaps more thorough.

What about the following diff:

---
diff --git a/src/box/xlog.c b/src/box/xlog.c
index 59459b25d..a60f2ffa8 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -126,12 +126,10 @@ xlog_meta_format(const struct xlog_meta *meta, char 
*buf, int size)
              VCLOCK_KEY ": %s\n",
              meta->filetype, v13, PACKAGE_VERSION, instance_uuid, vstr);
      free(vstr);
-     if (meta->has_prev_vclock) {
-             vstr = vclock_to_string(&meta->prev_vclock);
-             SNPRINT(total, snprintf, buf, size,
-                     PREV_VCLOCK_KEY ": %s\n", vstr);
-             free(vstr);
-     }
+     vstr = vclock_to_string(&meta->prev_vclock);
+     SNPRINT(total, snprintf, buf, size,
+             PREV_VCLOCK_KEY ": %s\n", vstr);
+     free(vstr);
      SNPRINT(total, snprintf, buf, size, "\n");
      assert(total > 0);
      return total;
@@ -263,7 +261,6 @@ xlog_meta_parse(struct xlog_meta *meta, const char **data,
                       */
                      if (parse_vclock(val, val_end, &meta->prev_vclock) != 0)
                              return -1;
-                     meta->has_prev_vclock = true;

We need to know if PrevVclock is available so that we can skip the check
when recovering from an xlog generated by a previous version.

              } else if (memcmp(key, VERSION_KEY, key_end - key) == 0) {
                      /* Ignore Version: for now */
              } else {
@@ -926,12 +923,10 @@ xdir_create_xlog(struct xdir *dir, struct xlog *xlog,
       * For WAL dir: store vclock of the previous xlog file
       * to check for gaps on recovery.
       */
-     if (dir->type == XLOG && !vclockset_empty(&dir->index)) {
+     if (!vclockset_empty(&dir->index)) {
              vclock_copy(&meta.prev_vclock, vclockset_last(&dir->index));
-             meta.has_prev_vclock = true;
      } else {
              vclock_create(&meta.prev_vclock);
-             meta.has_prev_vclock = false;
      }

Storing vclock in snap and vylog files doesn't make any sense IMHO.
I'd prefer to check dir->type explicitly.

If you dislike the extra variable, we can probably introduce a helper to
check if vclock is set, something like

        static inline bool
        vclock_is_set(const struct vclock *vclock)
        {
                return vclock->signature >= 0;
        }

Other related posts: