MDEV-11675. Cleanup and review notes address

- Two, incl a trivial line break, unnessary hunks are removed;
- RAII Write_log_with_flags made simplified, the only its invocation
  with other than FL_START_ALTER_E1 is always preceeded to set the SA's
  seq_no (log_event_server.cc:1736);
- types of accessers and setters to flags_extra made consistent with
  the object itself (uchar);
- corrected the transaction type to be false in
  write_bin_log_with_if_exists() of write_bin_log_start_alter().

4 files changed, 35 insertions, 22 deletions

diff --git a/sql/log.cc b/sql/log.cc
index f048b1854b0..ad78f40d00f 100644
--- a/sql/log.cc
+++ b/sql/log.cc
@@ -578,8 +578,8 @@ public:
   /* Set if we get an error during commit that must be returned from unlog(). */
   bool delayed_error;
   //Will be reset when gtid is written into binlog
-  uint16 gtid_flags3;
-  uint64 sa_seq_no;
+  uchar  gtid_flags3;
+  decltype (rpl_gtid::seq_no) sa_seq_no;
 private:
 
   binlog_cache_mngr& operator=(const binlog_cache_mngr& info);
@@ -646,9 +646,13 @@ bool write_bin_log_start_alter(THD *thd, bool& partial_alter,
                            Gtid_log_event::FL_COMMIT_ALTER_E1 |
                            Gtid_log_event::FL_ROLLBACK_ALTER_E1)));
 
-    // After logging binlog state stays flagged with SA flags3 an seq_no
+    /*
+      After logging binlog state stays flagged with SA flags3 an seq_no.
+      The state is not reset after write_bin_log() is done which is
+      deferred for the second logging phase.
+    */
     thd->set_binlog_flags_for_alter(Gtid_log_event::FL_START_ALTER_E1);
-    if(write_bin_log_with_if_exists(thd, false, true, if_exists, false))
+    if(write_bin_log_with_if_exists(thd, false, false, if_exists, false))
     {
       DBUG_ASSERT(thd->is_error());
       return true;
@@ -5839,15 +5843,21 @@ binlog_cache_mngr *THD::binlog_setup_trx_data()
 /*
   Two phase logged ALTER getter and setter methods.
 */
-uint16 THD::get_binlog_flags_for_alter()
+uchar THD::get_binlog_flags_for_alter()
 {
   return mysql_bin_log.is_open() ? binlog_setup_trx_data()->gtid_flags3 : 0;
 }
 
-void THD::set_binlog_flags_for_alter(uint16 flags)
+void THD::set_binlog_flags_for_alter(uchar flags)
 {
   if (mysql_bin_log.is_open())
+  {
+    // SA must find the flag set empty
+    DBUG_ASSERT(flags != Gtid_log_event::FL_START_ALTER_E1 ||
+                binlog_setup_trx_data()->gtid_flags3 == 0);
+
     binlog_setup_trx_data()->gtid_flags3= flags;
+  }
 }
 
 uint64 THD::get_binlog_start_alter_seq_no()
diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc
index d346ef3628f..af4898dde58 100644
--- a/sql/log_event_server.cc
+++ b/sql/log_event_server.cc
@@ -1307,7 +1307,7 @@ bool Query_log_event::write()
   if (gtid_flags_extra)
   {
     *start++= Q_GTID_FLAGS3;
-    buf[start++]= gtid_flags_extra;
+    *start++= gtid_flags_extra;
     if (gtid_flags_extra &
         (Gtid_log_event::FL_COMMIT_ALTER_E1 |
          Gtid_log_event::FL_ROLLBACK_ALTER_E1))
diff --git a/sql/sql_class.h b/sql/sql_class.h
index 6d08c59aae3..0bfd9163e4b 100644
--- a/sql/sql_class.h
+++ b/sql/sql_class.h
@@ -2999,8 +2999,8 @@ public:
   bool binlog_table_should_be_logged(const LEX_CSTRING *db);
 
   // Accessors and setters of two-phase loggable ALTER binlog properties
-  uint16 get_binlog_flags_for_alter();
-  void   set_binlog_flags_for_alter(uint16);
+  uchar get_binlog_flags_for_alter();
+  void   set_binlog_flags_for_alter(uchar);
   uint64 get_binlog_start_alter_seq_no();
   void   set_binlog_start_alter_seq_no(uint64);
 #endif /* MYSQL_CLIENT */
@@ -7799,13 +7799,9 @@ public:
     m_thd->set_binlog_start_alter_seq_no(0);
   }
 
-  Write_log_with_flags(THD *thd, uint16 flags, uint64 seq_no= 0) : m_thd(thd)
+  Write_log_with_flags(THD *thd, uchar flags) : m_thd(thd)
   {
     m_thd->set_binlog_flags_for_alter(flags);
-    if (seq_no == 0)
-      seq_no= m_thd->get_binlog_start_alter_seq_no();
-    if (seq_no != 0)
-      m_thd->set_binlog_start_alter_seq_no(seq_no);
   }
 };
 
diff --git a/sql/sql_table.cc b/sql/sql_table.cc
index 9490de15108..4e8b2003b2e 100644
--- a/sql/sql_table.cc
+++ b/sql/sql_table.cc
@@ -7312,8 +7312,9 @@ static bool notify_tabledef_changed(TABLE_LIST *table_list)
   false on Success
   true  otherwise
 */
-bool write_bin_log_start_alter_rollback(THD *thd, uint64 &start_alter_id,
-                                        bool &partial_alter, bool if_exists)
+static bool
+write_bin_log_start_alter_rollback(THD *thd, uint64 &start_alter_id,
+                                   bool &partial_alter, bool if_exists)
 {
   if (start_alter_id)
   {
@@ -7361,6 +7362,13 @@ bool write_bin_log_start_alter_rollback(THD *thd, uint64 &start_alter_id,
                             used during different phases.
   @param target_mdl_request Metadata request/lock on the target table name.
   @param alter_ctx          ALTER TABLE runtime context.
+  @param partial_alter      Is set to true to return the fact of the first
+                            "START ALTER" binlogging phase is done.
+  @param[in/out]
+         start_alter_id     Gtid seq_no of START ALTER or zero otherwise;
+                            it may get changed to return to the caller.
+  @param if_exists          True indicates the binary logging of the query
+                            should be done with "if exists" option.
 
   @retval   >=1               Error{ 1= ROLLBACK recieved from master , 2= error
                                     in alter so no ROLLBACK in binlog }
@@ -10580,6 +10588,7 @@ do_continue:;
         goto err_cleanup;
       }
       cleanup_table_after_inplace_alter_keep_files(&altered_table);
+
       goto end_inplace;
     }
     else
@@ -10793,7 +10802,9 @@ do_continue:;
       thd->variables.option_bits&= ~OPTION_BIN_COMMIT_OFF;
       binlog_commit(thd, true);
     }
-    DBUG_ASSERT(!start_alter_id);
+
+    DBUG_ASSERT(!start_alter_id);  // no 2 phase logging for
+    DBUG_ASSERT(!partial_alter);   // temporary table alter
 
     /* We don't replicate alter table statement on temporary tables */
     if (!thd->is_current_stmt_binlog_format_row() &&
@@ -10803,8 +10814,7 @@ do_continue:;
       int tmp_error;
       thd->binlog_xid= thd->query_id;
       ddl_log_update_xid(&ddl_log_state, thd->binlog_xid);
-      tmp_error= write_bin_log_with_if_exists(thd, true, false, log_if_exists,
-                                              partial_alter);
+      tmp_error= write_bin_log_with_if_exists(thd, true, false, log_if_exists);
       thd->binlog_xid= 0;
       if (tmp_error)
         goto err_cleanup;
@@ -11157,9 +11167,6 @@ err_with_mdl:
     remove all references to the altered table from the list of locked
     tables and release the exclusive metadata lock.
   */
-  if (partial_alter || start_alter_id)
-    write_bin_log_start_alter_rollback(thd, start_alter_id, partial_alter,
-                                       if_exists);
   thd->locked_tables_list.unlink_all_closed_tables(thd, NULL, 0);
   if (!table_list->table)
     thd->mdl_context.release_all_locks_for_name(mdl_ticket);