[Commits] a6fb98f: MDEV-6368: assertion xid_seqno > trx_sys_cur_xid_seqno

Nirbhay Choubey nirbhay at mariadb.com
Tue May 3 01:26:58 EEST 2016


revision-id: a6fb98fd74ab6f14fab0c1ef4630b010ff28880f (mariadb-10.1.13-6-ga6fb98f)
parent(s): f8cdb9e7e8fb692f8eb3b9bb4792cd60653e0eb8
author: Nirbhay Choubey
committer: Nirbhay Choubey
timestamp: 2016-05-02 18:26:58 -0400
message:

MDEV-6368: assertion xid_seqno > trx_sys_cur_xid_seqno

- Validate the specified wsrep_start_position value by also
checking the return status of wsrep->sst_received. This also
ensures that changes in wsrep_start_position is not allowed
when the node is not in JOINING state.
- Do not allow decrease in seqno within same UUID.
- The initial checkpoint in SEs should be [0...:-1].

---
 .../sys_vars/r/wsrep_start_position_basic.result   | 10 ++-
 .../sys_vars/t/wsrep_start_position_basic.test     |  6 +-
 sql/wsrep_priv.h                                   |  4 +-
 sql/wsrep_sst.cc                                   | 93 ++++++++++++++++-----
 sql/wsrep_var.cc                                   | 96 ++++++++++++++--------
 storage/innobase/trx/trx0sys.cc                    |  6 +-
 storage/xtradb/trx/trx0sys.cc                      |  6 +-
 7 files changed, 154 insertions(+), 67 deletions(-)

diff --git a/mysql-test/suite/sys_vars/r/wsrep_start_position_basic.result b/mysql-test/suite/sys_vars/r/wsrep_start_position_basic.result
index a49e613..036debb 100644
--- a/mysql-test/suite/sys_vars/r/wsrep_start_position_basic.result
+++ b/mysql-test/suite/sys_vars/r/wsrep_start_position_basic.result
@@ -17,10 +17,6 @@ SELECT @@global.wsrep_start_position;
 00000000-0000-0000-0000-000000000000:-1
 
 # valid values
-SET @@global.wsrep_start_position='00000000-0000-0000-0000-000000000000:-2';
-SELECT @@global.wsrep_start_position;
-@@global.wsrep_start_position
-00000000-0000-0000-0000-000000000000:-2
 SET @@global.wsrep_start_position='12345678-1234-1234-1234-123456789012:100';
 SELECT @@global.wsrep_start_position;
 @@global.wsrep_start_position
@@ -31,6 +27,12 @@ SELECT @@global.wsrep_start_position;
 00000000-0000-0000-0000-000000000000:-1
 
 # invalid values
+call mtr.add_suppression("WSREP: SST postion can't be set in past.*");
+SET @@global.wsrep_start_position='00000000-0000-0000-0000-000000000000:-2';
+ERROR 42000: Variable 'wsrep_start_position' can't be set to the value of '00000000-0000-0000-0000-000000000000:-2'
+SELECT @@global.wsrep_start_position;
+@@global.wsrep_start_position
+00000000-0000-0000-0000-000000000000:-1
 SET @@global.wsrep_start_position='000000000000000-0000-0000-0000-000000000000:-1';
 ERROR 42000: Variable 'wsrep_start_position' can't be set to the value of '000000000000000-0000-0000-0000-000000000000:-1'
 SET @@global.wsrep_start_position='12345678-1234-1234-12345-123456789012:100';
diff --git a/mysql-test/suite/sys_vars/t/wsrep_start_position_basic.test b/mysql-test/suite/sys_vars/t/wsrep_start_position_basic.test
index 3e57cfa..936646f 100644
--- a/mysql-test/suite/sys_vars/t/wsrep_start_position_basic.test
+++ b/mysql-test/suite/sys_vars/t/wsrep_start_position_basic.test
@@ -19,8 +19,6 @@ SELECT @@global.wsrep_start_position;
 
 --echo
 --echo # valid values
-SET @@global.wsrep_start_position='00000000-0000-0000-0000-000000000000:-2';
-SELECT @@global.wsrep_start_position;
 SET @@global.wsrep_start_position='12345678-1234-1234-1234-123456789012:100';
 SELECT @@global.wsrep_start_position;
 SET @@global.wsrep_start_position=default;
@@ -28,6 +26,10 @@ SELECT @@global.wsrep_start_position;
 
 --echo
 --echo # invalid values
+call mtr.add_suppression("WSREP: SST postion can't be set in past.*");
+--error ER_WRONG_VALUE_FOR_VAR
+SET @@global.wsrep_start_position='00000000-0000-0000-0000-000000000000:-2';
+SELECT @@global.wsrep_start_position;
 --error ER_WRONG_VALUE_FOR_VAR
 SET @@global.wsrep_start_position='000000000000000-0000-0000-0000-000000000000:-1';
 --error ER_WRONG_VALUE_FOR_VAR
diff --git a/sql/wsrep_priv.h b/sql/wsrep_priv.h
index 30dce78..a6e57e3 100644
--- a/sql/wsrep_priv.h
+++ b/sql/wsrep_priv.h
@@ -40,8 +40,8 @@ wsrep_cb_status wsrep_sst_donate_cb (void* app_ctx,
 extern wsrep_seqno_t local_seqno;
 
 // a helper function
-void wsrep_sst_received(wsrep_t*, const wsrep_uuid_t&, wsrep_seqno_t,
-                        const void*, size_t);
+bool wsrep_sst_received(wsrep_t*, const wsrep_uuid_t&, wsrep_seqno_t,
+                        const void*, size_t, bool const);
 /*! SST thread signals init thread about sst completion */
 void wsrep_sst_complete(const wsrep_uuid_t*, wsrep_seqno_t, bool);
 
diff --git a/sql/wsrep_sst.cc b/sql/wsrep_sst.cc
index 4f6cc51..44fdd03 100644
--- a/sql/wsrep_sst.cc
+++ b/sql/wsrep_sst.cc
@@ -264,42 +264,91 @@ void wsrep_sst_complete (const wsrep_uuid_t* sst_uuid,
   mysql_mutex_unlock (&LOCK_wsrep_sst);
 }
 
-void wsrep_sst_received (wsrep_t*            const wsrep,
+/*
+  If wsrep provider is loaded, inform that the new state snapshot
+  has been received. Also update the local checkpoint.
+
+  @param wsrep     [IN]               wsrep handle
+  @param uuid      [IN]               Initial state UUID
+  @param seqno     [IN]               Initial state sequence number
+  @param state     [IN]               Always NULL, also ignored by wsrep provider (?)
+  @param state_len [IN]               Always 0, also ignored by wsrep provider (?)
+  @param implicit  [IN]               Whether invoked implicitly due to SST
+                                      (true) or explicitly because if change
+                                      in wsrep_start_position by user (false).
+  @return false                       Success
+          true                        Error
+
+*/
+bool wsrep_sst_received (wsrep_t*            const wsrep,
                          const wsrep_uuid_t&       uuid,
                          wsrep_seqno_t       const seqno,
                          const void*         const state,
-                         size_t              const state_len)
+                         size_t              const state_len,
+                         bool                const implicit)
 {
-    wsrep_get_SE_checkpoint(local_uuid, local_seqno);
+  /*
+    To keep track of whether the local uuid:seqno should be updated. Also, note
+    that local state (uuid:seqno) is updated/checkpointed only after we get an
+    OK from wsrep provider. By doing so, the values remain consistent across
+    the server & wsrep provider.
+  */
+  bool do_update= false;
+
+  // Get the locally stored uuid:seqno.
+  wsrep_get_SE_checkpoint(local_uuid, local_seqno);
 
-    if (memcmp(&local_uuid, &uuid, sizeof(wsrep_uuid_t)) ||
-        local_seqno < seqno || seqno < 0)
+  if (memcmp(&local_uuid, &uuid, sizeof(wsrep_uuid_t)) ||
+      local_seqno < seqno)
+  {
+    do_update= true;
+  }
+  else if (local_seqno > seqno)
+  {
+    WSREP_WARN("SST position can't be set in past. Requested: %lld, Current: "
+               " %lld.", (long long)seqno, (long long)local_seqno);
+    /*
+      If we are here because of SET command, simply return true (error) instead of
+      aborting.
+    */
+    if (implicit)
     {
-        wsrep_set_SE_checkpoint(uuid, seqno);
-        local_uuid = uuid;
-        local_seqno = seqno;
+      WSREP_WARN("Can't continue.");
+      unireg_abort(1);
     }
-    else if (local_seqno > seqno)
+    else
     {
-        WSREP_WARN("SST postion is in the past: %lld, current: %lld. "
-                   "Can't continue.",
-                   (long long)seqno, (long long)local_seqno);
-        unireg_abort(1);
+      return true;
     }
+  }
 
 #ifdef GTID_SUPPORT
-    wsrep_init_sidno(uuid);
+  wsrep_init_sidno(uuid);
 #endif /* GTID_SUPPORT */
 
-    if (wsrep)
-    {
-        int const rcode(seqno < 0 ? seqno : 0);
-        wsrep_gtid_t const state_id = {
-            uuid, (rcode ? WSREP_SEQNO_UNDEFINED : seqno)
-        };
+  if (wsrep)
+  {
+    int const rcode(seqno < 0 ? seqno : 0);
+    wsrep_gtid_t const state_id= {uuid,
+      (rcode ? WSREP_SEQNO_UNDEFINED : seqno)};
+
+    wsrep_status_t ret= wsrep->sst_received(wsrep, &state_id, state,
+                                            state_len, rcode);
 
-        wsrep->sst_received(wsrep, &state_id, state, state_len, rcode);
+    if (ret != WSREP_OK)
+    {
+      return true;
     }
+  }
+
+  // Now is the good time to update the local state and checkpoint.
+  if (do_update) {
+    wsrep_set_SE_checkpoint(uuid, seqno);
+    local_uuid= uuid;
+    local_seqno= seqno;
+  }
+
+  return false;
 }
 
 // Let applier threads to continue
@@ -308,7 +357,7 @@ void wsrep_sst_continue ()
   if (sst_needed)
   {
     WSREP_INFO("Signalling provider to continue.");
-    wsrep_sst_received (wsrep, local_uuid, local_seqno, NULL, 0);
+    wsrep_sst_received (wsrep, local_uuid, local_seqno, NULL, 0, true);
   }
 }
 
diff --git a/sql/wsrep_var.cc b/sql/wsrep_var.cc
index 9c01e54..bd94190 100644
--- a/sql/wsrep_var.cc
+++ b/sql/wsrep_var.cc
@@ -81,32 +81,68 @@ bool wsrep_sync_wait_update (sys_var* self, THD* thd, enum_var_type var_type)
   return false;
 }
 
-static int wsrep_start_position_verify (const char* start_str)
+
+/*
+  Verify the format of the given UUID:seqno.
+
+  @return
+    true                    Fail
+    false                   Pass
+*/
+static
+bool wsrep_start_position_verify (const char* start_str)
 {
   size_t        start_len;
   wsrep_uuid_t  uuid;
   ssize_t       uuid_len;
 
+  // Check whether it has minimum acceptable length.
   start_len = strlen (start_str);
   if (start_len < 34)
-    return 1;
+    return true;
 
+  /*
+    Parse the input to check whether UUID length is acceptable
+    and seqno has been provided.
+  */
   uuid_len = wsrep_uuid_scan (start_str, start_len, &uuid);
   if (uuid_len < 0 || (start_len - uuid_len) < 2)
-    return 1;
+    return true;
 
-  if (start_str[uuid_len] != ':') // separator should follow UUID
-    return 1;
+  // Separator must follow the UUID.
+  if (start_str[uuid_len] != ':')
+    return true;
 
   char* endptr;
   wsrep_seqno_t const seqno __attribute__((unused)) // to avoid GCC warnings
     (strtoll(&start_str[uuid_len + 1], &endptr, 10));
 
-  if (*endptr == '\0') return 0; // remaining string was seqno
+  // Remaining string was seqno.
+  if (*endptr == '\0') return false;
 
-  return 1;
+  return true;
+}
+
+
+static
+bool wsrep_set_local_position(const char* const value, size_t length,
+                              bool const sst)
+{
+  wsrep_uuid_t uuid;
+  size_t const uuid_len = wsrep_uuid_scan(value, length, &uuid);
+  wsrep_seqno_t const seqno = strtoll(value + uuid_len + 1, NULL, 10);
+
+  if (sst) {
+    return wsrep_sst_received (wsrep, uuid, seqno, NULL, 0, false);
+  } else {
+    // initialization
+    local_uuid = uuid;
+    local_seqno = seqno;
+  }
+  return false;
 }
 
+
 bool wsrep_start_position_check (sys_var *self, THD* thd, set_var* var)
 {
   char start_pos_buf[FN_REFLEN];
@@ -119,40 +155,34 @@ bool wsrep_start_position_check (sys_var *self, THD* thd, set_var* var)
          var->save_result.string_value.length);
   start_pos_buf[var->save_result.string_value.length]= 0;
 
-  if (!wsrep_start_position_verify(start_pos_buf)) return 0;
+  // Verify the format.
+  if (wsrep_start_position_verify(start_pos_buf)) return true;
+
+  /*
+    As part of further verification, we try to update the value and catch
+    errors (if any).
+  */
+  if (wsrep_set_local_position(var->save_result.string_value.str,
+                               var->save_result.string_value.length,
+                               true))
+  {
+    goto err;
+  }
+
+  return false;
 
 err:
   my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str,
            var->save_result.string_value.str ?
            var->save_result.string_value.str : "NULL");
-  return 1;
-}
-
-static
-void wsrep_set_local_position(const char* const value, bool const sst)
-{
-  size_t const value_len = strlen(value);
-  wsrep_uuid_t uuid;
-  size_t const uuid_len = wsrep_uuid_scan(value, value_len, &uuid);
-  wsrep_seqno_t const seqno = strtoll(value + uuid_len + 1, NULL, 10);
-
-  if (sst) {
-    wsrep_sst_received (wsrep, uuid, seqno, NULL, 0);
-  } else {
-    // initialization
-    local_uuid = uuid;
-    local_seqno = seqno;
-  }
+  return true;
 }
 
 bool wsrep_start_position_update (sys_var *self, THD* thd, enum_var_type type)
 {
-  WSREP_INFO ("wsrep_start_position var submitted: '%s'",
-              wsrep_start_position);
-  // since this value passed wsrep_start_position_check, don't check anything
-  // here
-  wsrep_set_local_position (wsrep_start_position, true);
-  return 0;
+  // Print a confirmation that wsrep_start_position has been updated.
+  WSREP_INFO ("wsrep_start_position set to '%s'", wsrep_start_position);
+  return false;
 }
 
 void wsrep_start_position_init (const char* val)
@@ -164,7 +194,7 @@ void wsrep_start_position_init (const char* val)
     return;
   }
 
-  wsrep_set_local_position (val, false);
+  wsrep_set_local_position (val, strlen(val), false);
 }
 
 static bool refresh_provider_options()
diff --git a/storage/innobase/trx/trx0sys.cc b/storage/innobase/trx/trx0sys.cc
index 76c8613..1625e8d 100644
--- a/storage/innobase/trx/trx0sys.cc
+++ b/storage/innobase/trx/trx0sys.cc
@@ -349,10 +349,10 @@ void read_wsrep_xid_uuid(const XID* xid, unsigned char* buf)
             unsigned char xid_uuid[16];
             long long xid_seqno = read_wsrep_xid_seqno(xid);
             read_wsrep_xid_uuid(xid, xid_uuid);
-            if (!memcmp(xid_uuid, trx_sys_cur_xid_uuid, 8))
+            if (!memcmp(xid_uuid, trx_sys_cur_xid_uuid, 16) &&
+                (xid_seqno > -1))
             {
                 ut_ad(xid_seqno > trx_sys_cur_xid_seqno);
-                trx_sys_cur_xid_seqno = xid_seqno;
             }
             else
             {
@@ -411,6 +411,8 @@ void read_wsrep_xid_uuid(const XID* xid, unsigned char* buf)
                                       + TRX_SYS_WSREP_XID_MAGIC_N_FLD))
             != TRX_SYS_WSREP_XID_MAGIC_N) {
                 memset(xid, 0, sizeof(*xid));
+                long long seqno= -1;
+                memcpy(xid->data + 24, &seqno, sizeof(long long));
                 xid->formatID = -1;
                 trx_sys_update_wsrep_checkpoint(xid, sys_header, &mtr);
                 mtr_commit(&mtr);
diff --git a/storage/xtradb/trx/trx0sys.cc b/storage/xtradb/trx/trx0sys.cc
index 9b136cc..08bb74a 100644
--- a/storage/xtradb/trx/trx0sys.cc
+++ b/storage/xtradb/trx/trx0sys.cc
@@ -353,10 +353,10 @@ void read_wsrep_xid_uuid(const XID* xid, unsigned char* buf)
             unsigned char xid_uuid[16];
             long long xid_seqno = read_wsrep_xid_seqno(xid);
             read_wsrep_xid_uuid(xid, xid_uuid);
-            if (!memcmp(xid_uuid, trx_sys_cur_xid_uuid, 8))
+            if (!memcmp(xid_uuid, trx_sys_cur_xid_uuid, 16) &&
+                (xid_seqno > -1))
             {
                 ut_ad(xid_seqno > trx_sys_cur_xid_seqno);
-                trx_sys_cur_xid_seqno = xid_seqno;
             }
             else
             {
@@ -415,6 +415,8 @@ void read_wsrep_xid_uuid(const XID* xid, unsigned char* buf)
                                       + TRX_SYS_WSREP_XID_MAGIC_N_FLD))
             != TRX_SYS_WSREP_XID_MAGIC_N) {
                 memset(xid, 0, sizeof(*xid));
+                long long seqno= -1;
+                memcpy(xid->data + 24, &seqno, sizeof(long long));
                 xid->formatID = -1;
                 trx_sys_update_wsrep_checkpoint(xid, sys_header, &mtr);
                 mtr_commit(&mtr);


More information about the commits mailing list