Bug #29390170: MOVE SEMANTICS NOT USED FOR CRUD OPERATIONS

Affecting same object will not trigger a new object, so not loosing the
Prepare state.
PS id can be shared with multiple operations.

Author: silvakid

common/op_impl.h

@@ -101,6 +101,7 @@ class Op_base
 
   using string = std::string;
   using Shared_session_impl = shared_ptr<Session_impl>;
+  using Shared_stmt_id = shared_ptr<uint32_t>;
 
   Shared_session_impl m_sess;
 
@@ -110,7 +111,7 @@ class Op_base
   */
   cdk::scoped_ptr<cdk::Reply> m_reply;
 
-  uint32_t m_stmt_id = 0;
+  Shared_stmt_id m_stmt_id;
   Prepare_state  m_prepare_state = PS_EXECUTE;
   bool m_inited = false;
   bool m_completed = false;
@@ -130,14 +131,13 @@ class Op_base
 
   Op_base(const Op_base& other)
     : m_sess(other.m_sess)
+    , m_stmt_id(other.m_stmt_id)
+    , m_prepare_state(other.m_prepare_state)
   {}
 
-  virtual ~Op_base()
+  virtual ~Op_base() override
   {
-    if (m_stmt_id != 0)
-    {
-      m_sess->release_stmt_id(m_stmt_id);
-    }
+    release_stmt_id();
   }
 
   cdk::Session& get_cdk_session()
@@ -148,19 +148,23 @@ class Op_base
 
   uint32_t create_stmt_id()
   {
-    if (m_stmt_id == 0)
+    assert(m_sess);
+    if (!m_stmt_id.unique())
     {
-      assert(m_sess);
-      m_stmt_id = m_sess->create_stmt_id();
+      uint32_t id = m_sess->create_stmt_id();
+      if(id != 0)
+        m_stmt_id.reset(new uint32_t(id));
+      else
+        m_stmt_id.reset();
     }
-    return m_stmt_id;
+    return get_stmt_id();
   }
 
   void release_stmt_id()
   {
-    if (m_stmt_id != 0)
-      m_sess->release_stmt_id(m_stmt_id);
-    m_stmt_id = 0;
+    if (m_stmt_id.unique())
+      m_sess->release_stmt_id(*m_stmt_id);
+    m_stmt_id.reset();
   }
 
 
@@ -171,9 +175,9 @@ class Op_base
 
   void reset_state()
   {
-    if (m_stmt_id != 0)
-      get_session()->error_stmt_id(m_stmt_id);
-    m_stmt_id = 0;
+    if (m_stmt_id.unique())
+      get_session()->error_stmt_id(*m_stmt_id);
+    m_stmt_id.reset();
     m_prepare_state = PS_EXECUTE;
     m_reply.reset();
     m_inited = false;
@@ -182,7 +186,7 @@ class Op_base
 
   uint32_t get_stmt_id()
   {
-    return m_stmt_id;
+    return m_stmt_id ? *m_stmt_id.get() : 0;
   }
 
   Prepare_state get_prepare_state()
@@ -475,23 +479,25 @@ class Op_base
     auto prepare = get_prepare_state();
 
     /*
-      Upon first execution, get_executed() and get_re_prepare() are false and
-      get_stmt_id() is 0. The new statement id is not allocated yet and function
-      returns false, meaning that the statement will be executed directly
-      without preparing.
+      Upon first execution, prepare is on PS_EXECUTE state and get_stmt_id() is
+      0. The new statement id is not allocated yet and function returns false,
+      meaning that the statement will be executed directly without preparing.
+      Also, prepare is set to PS_PREPARE_EXECUTE.
 
-      On next execution, get_executed() and get_re_prepare() are true. Then new
+      On next execution, prepare is then on PS_PREPARE_EXECUTE. Then new
       PS id is allocated and function returns false, meaning that the statement
-      gets prepared and executed. Also, the re-prepare flag is cleared.
+      gets prepared and executed. Also, the state is set to PS_EXECUTE_PREPARED.
 
-      On 3rd and following executions, if re-prepare flag stays false, this
+      On 3rd and following executions, if state stays PS_EXECUTE_PREPARED, this
       function will return true meaning that the prepared statement should be
       used.
     */
 
-    if (prepare != PS_EXECUTE )
+    if (prepare == PS_PREPARE_EXECUTE )
+    {
       create_stmt_id();
-    else
+    }
+    else if (prepare == PS_EXECUTE)
     {
       release_stmt_id();
     }

devapi/tests/bugs-t.cc

@@ -378,3 +378,115 @@ TEST_F(Bugs, list_initializer)
     std::cout << r.get(0).get<string>() << std::endl;
   }
 }
+
+TEST_F(Bugs, crud_move)
+{
+  SKIP_IF_NO_XPLUGIN;
+
+  auto coll = get_sess().createSchema("test",true).createCollection("c1",true);
+
+  coll.remove("true").execute();
+
+  Result add_res = coll.add(
+                 "{ \"_id\": \"myuuid-1\", \"name\": \"foo\", \"age\": 7 }",
+                 "{ \"name\": \"buz\", \"age\": 17 }",
+                 "{ \"name\": \"bar\", \"age\": 3 }"
+                 ).execute();
+
+  auto find = coll.find();
+  // query
+  find.execute();
+  // prepare+execute
+  find.execute();
+
+  EXPECT_EQ(1,
+            sql("select count(*) from performance_schema.prepared_statements_instances").fetchOne()[0].get<int>());
+
+  {
+    auto tmp_find = find;
+    //Since limit will prepare+execute right away, we should test it here:
+    find.limit(2);
+    // prepare+execute
+    find.execute();
+    // execute
+    find.execute();
+    find = find.limit(1);
+    find.execute();
+
+    EXPECT_EQ(2,
+              sql("select count(*) from performance_schema.prepared_statements_instances").fetchOne()[0].get<int>());
+  }
+
+  //Force stmt_id cleanup
+  find.sort("name ASC");
+  find.execute();
+  find.execute();
+
+  { //Find2 scope
+    CollectionFind find2 = find.limit(1);
+    //With assign, both point to same implementation (also same PS id), untill one
+    //changes some parameter, which in that case, will create a clone.
+
+    // execute
+    find.execute();
+
+    find2.limit(2);
+    // execute
+    find.execute();
+
+    // execute
+    find2.execute();
+    find2.execute();
+
+    EXPECT_EQ(1,
+              sql("select count(*) from performance_schema.prepared_statements_instances").fetchOne()[0].get<int>());
+
+    //Move works just like assignment (same as shared_ptr behaviour)
+    find = std::move(find2);
+    { //find3 scope
+      auto find3 = find;
+
+      // execute
+      find2.execute();
+      find2.execute();
+      // execute
+      find2.execute();
+      find2.execute();
+      // execute
+      find3.execute();
+      find3.execute();
+
+      EXPECT_EQ(1,
+                sql("select count(*) from performance_schema.prepared_statements_instances").fetchOne()[0].get<int>());
+
+      find.sort("name ASC");
+
+      // query
+      find.execute();
+      // prepare+execute
+      find.execute();
+      // execute
+      find2.execute();
+      // execute
+      find3.execute();
+
+      EXPECT_EQ(2,
+                sql("select count(*) from performance_schema.prepared_statements_instances").fetchOne()[0].get<int>());
+
+    } //find3 scope
+
+    EXPECT_EQ(2,
+              sql("select count(*) from performance_schema.prepared_statements_instances").fetchOne()[0].get<int>());
+
+  }// find2 scope
+
+  find.sort("name DESC");
+
+  find.execute();
+  find.execute();
+
+  EXPECT_EQ(1,
+            sql("select count(*) from performance_schema.prepared_statements_instances").fetchOne()[0].get<int>());
+
+
+}