Hi Kristian,
Thanks for undertaking this work! It is good to see GTID support being
added in semi-sync. This email only entails reviewing the refactor. I'll
follow-up with another email to review the GTID additions.
A couple of high-level questions/thoughts to start:
1. When do you envision this to go live? I think it is too big/late to go
into
12.3 at this point, given it wasn't in the preview.
2. Configurations of [new master using binlog-in-innodb] -> [old slave]
wouldn't
be able to use semi-sync, as there is no fallback for binlog-in-innodb
to use
offsets for ACKs. I think this is ok, but it should be documented.
Then to review specifics:
diff --git a/sql/mysqld.cc b/sql/mysqld.cc
index e7fb6f766d9..ed71ac2e992 100644
--- a/sql/mysqld.cc
+++ b/sql/mysqld.cc
@@ -5159,6 +5159,16 @@ static int init_server_components()
opt_bin_logname= my_once_strdup(buf2, MYF(MY_WME));
}
+ /*
+ The repl_semisync_master object needs to be constructed early, as it
can be
+ called into from eg. ha_commit_trans(). However the initialization of
+ semi-sync happens only later.
Do we want to keep this pattern? Another option would be to split the logic
in Repl_semi_sync_master::init_object(), where the constructor does the
actual
initialization logic, and then the "init_object" function is replaced with
just
starting the ack_receiver thread. I'd think this would improve readability,
though
is admittedly minor either way.
+ */
+ if (binlog_engine_used)
+ repl_semisync_master= new Repl_semi_sync_master_gtid();
This may be confusing to see when going through history, as it would be more
natural to see this in the follow-up patch. Perhaps add a bit to the git
commit
message explaining why you added the GTID-subclass instantiation to this
commit.
iff --git a/sql/log.cc b/sql/log.cc
index bac621882c4..2bcc7f308b8 100644
--- a/sql/log.cc
+++ b/sql/log.cc
@@ -8750,8 +8751,9 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info,
my_bool *with_annotate)
mysql_mutex_assert_not_owner(&LOCK_after_binlog_sync);
mysql_mutex_assert_not_owner(&LOCK_commit_ordered);
#ifdef HAVE_REPLICATION
- if (repl_semisync_master.report_binlog_update(thd, thd,
- log_file_name,
offset))
+ if (repl_semisync_master->report_binlog_update(thd, thd,
+ log_file_name,
offset,
+ commit_gtid))
This ties into a general concern I have with Trans_binlog_info (described
later
in my review, but perhaps instead of passing all log_file_name, offset, and
commit_gtid into all these functions; there could be some factory which
creates
some correct sub-class of only a GTID- or offset- based transaction
identifier.
When in GTID mode, I don't think log_file_name and offset are needed, so if
we
can abstract that away as well, I think it would make the interface for
Repl_semi_sync_trx_info/Trans_binlog_info a bit clearer.
diff --git a/sql/sql_repl.cc b/sql/sql_repl.cc
index b192dd644c0..ac14b24ca99 100644
--- a/sql/sql_repl.cc
+++ b/sql/sql_repl.cc
@@ -2356,37 +2365,48 @@ send_event_to_slave(binlog_send_info *info,
Log_event_type event_type,
Or the until condition is specified as SQL_BEFORE_GTIDS
*/
- info->gtid_skip_group = (flags2 &
Gtid_log_event::FL_STANDALONE ?
- GTID_SKIP_STANDALONE :
GTID_SKIP_TRANSACTION);
+ info->gtid_skip_group = true;
}
}
}
}
}
- /*
- Skip event group if we have not yet reached the correct slave GTID
position.
+ if (info->in_event_group)
+ {
+ if (info->standalone)
+ {
+ if (!Log_event::is_part_of_group(event_type))
+ {
+ info->in_event_group= false;
+ end_of_group_event= true;
+ }
+ }
+ else
+ {
+ if (event_type == XID_EVENT || event_type == XA_PREPARE_LOG_EVENT ||
+ (event_type == QUERY_EVENT && /* QUERY_COMPRESSED_EVENT would
never be commmit or rollback */
+ Query_log_event::peek_is_commit_rollback((uchar*) packet->ptr()
+
+ ev_offset,
+ len - ev_offset,
+ current_checksum_alg)))
+ {
+ info->in_event_group= false;
+ end_of_group_event= true;
+ }
+ }
Perhaps make an inline function to encapsulate all this, as the result of
each
check through the various blocks is the same.
diff --git a/sql/semisync_master.h b/sql/semisync_master.h
index b1b8c9633e1..ae48ec85bf6 100644
--- a/sql/semisync_master.h
+++ b/sql/semisync_master.h
@@ -32,11 +32,44 @@ struct Tranx_node {
char log_name[FN_REFLEN];
bool thd_valid; /* thd is valid for signalling
*/
my_off_t log_pos;
+ rpl_gtid gtid;
THD *thd; /* The thread awaiting an ACK
*/
struct Tranx_node *next; /* the next node in the sorted list
*/
struct Tranx_node *hash_next; /* the next node during hash collision
*/
};
+
+/*
+ Encapsulate a reference to a binlog position.
+*/
+struct Repl_semi_sync_trx_info {
+ rpl_gtid gtid;
+ const char *log_file;
+ my_off_t log_pos;
+
+ Repl_semi_sync_trx_info();
+ Repl_semi_sync_trx_info(const char *file_name, my_off_t log_pos);
+ Repl_semi_sync_trx_info(const char *file_name, my_off_t log_pos,
+ const rpl_gtid *gtid);
+};
+
+
+/*
+ Structure to save transaction log filename and position
+*/
+struct Trans_binlog_info : public Repl_semi_sync_trx_info {
+ char file_name_buf[FN_REFLEN];
+
+ Trans_binlog_info();
+ Trans_binlog_info(const char *file_name, my_off_t log_pos);
+ Trans_binlog_info(const char *file_name, my_off_t log_pos, const
rpl_gtid *gtid);
+ void set(const Repl_semi_sync_trx_info *inf);
+ void set(const char *log_file, my_off_t log_pos, const rpl_gtid *gtid);
+ void clear();
+ bool is_clear() { return log_pos == 0; }
+};
The above hierarchy of Repl_semi_sync_trx_info and Trans_binlog_info isn't
really clear. It takes a fair bit of investigation to understand the purpose
of each class, and I'm still not entirely sure. It looks like
Trans_binlog_info
is used in log.cc when actually doing the binlogging, and
Repl_semi_sync_trx_info is used by Active_tranx logic to maintain the
semi-sync
aspects. So when binlogging creates _its_ object, it creates a
Trans_binlog_info, and then Acive_tranx refers to it as
Repl_semi_sync_trx_info. However, the life-cycles of the different
implementations (when instantiated differently) are hard to understand.
I.e.,
different details are filled out at different times in quite different
places.
A couple thoughts here:
1. The interface of each makes it look as if Repl_semi_sync_trx_info is
immutable because Trans_binlog_info has set() and clear(),
yet Repl_semi_sync_trx_info is not immutable. But rather, things are
set
in hidden areas (e.g. gtid in is_thd_waiter()). It would make the
interface
cleaner if Repl_semi_sync_trx_info was immutable (though it looks to be
at
the cost of performance :/). Something to think about. If you decide to
not
change it, the life-cycles should at least be clearly documented (e.g.
Trans_binlog_info objects are re-cycled, but Repl_semi_sync_trx_info
objects are not.
2. Would it be worth abstracting away the way a transaction is binlogged
from
here as well? (Mentioned in my previous comment).
@@ -388,6 +420,37 @@ class Active_tranx
};
+
+/*
+ Subclasses of Active_tranx, used for doing the acknowledgement with
+ old-style filename/offset respectively GTID.
+*/
Some comment should exist on `Active_tranx` to define the hierarchy.
@@ -518,7 +574,13 @@ class Repl_semi_sync_master
int init_object();
/* Enable the object to enable semi-sync replication inside the master.
*/
- int enable_master();
+ virtual int enable_master() = 0;
+protected:
+ template <typename F> int
+ enable_master(F tranx_alloc);
Why is this a template? Both implementations give lambdas with the same
signatures. I'd think this would be clearer if the type was a
function-pointer, or whatever C++ equivalent you want.
diff --git a/sql/semisync_master.cc b/sql/semisync_master.cc
index db3b01185b0..dc5d49c892e 100644
--- a/sql/semisync_master.cc
+++ b/sql/semisync_master.cc
@@ -334,18 +361,22 @@ void Active_tranx::unlink_thd_as_waiter(const char
*log_file_name,
}
Tranx_node *
-Active_tranx::is_thd_waiter(const char *log_file_name, my_off_t
log_file_pos)
+Active_tranx::is_thd_waiter(Repl_semi_sync_trx_info *inf)
{
DBUG_ENTER("Active_tranx::assert_thd_is_waiter");
mysql_mutex_assert_owner(m_lock);
- unsigned int hash_val = get_hash_value(log_file_name, log_file_pos);
+ unsigned int hash_val = get_hash_value(inf);
Tranx_node *entry = m_trx_htb[hash_val];
while (entry != NULL)
{
- if (!compare(entry, log_file_name, log_file_pos))
+ if (!compare(entry, inf))
+ {
+ if (inf->gtid.seq_no == 0)
+ inf->gtid= entry->gtid;
Overwriting the Repl_semi_sync_trx_info::gtid should be documented at the
function-level (also probably at the definition in Repl_semi_sync_trx_info
to help explain its life-cycle. It is very hidden here.
@@ -353,6 +384,94 @@ Active_tranx::is_thd_waiter(const char *log_file_name,
my_off_t log_file_pos)
DBUG_RETURN(entry);
}
+
+Repl_semi_sync_trx_info::Repl_semi_sync_trx_info()
+{
+ gtid.domain_id= 0;
+ gtid.server_id= 0;
+ gtid.seq_no= 0;
+ log_file= "";
+ log_pos= 0;
+}
+
+
+Repl_semi_sync_trx_info::Repl_semi_sync_trx_info(const char *file_name_,
+ my_off_t log_pos_)
+{
+ gtid.domain_id= 0;
+ gtid.server_id= 0;
+ gtid.seq_no= 0;
+ log_file= file_name_;
+ log_pos= log_pos_;
+}
+
+Repl_semi_sync_trx_info::Repl_semi_sync_trx_info(const char *file_name_,
+ my_off_t log_pos_,
+ const rpl_gtid *gtid_)
+{
+ gtid= *gtid_;
+ log_file= file_name_;
+ log_pos= log_pos_;
+}
+
+
+Trans_binlog_info::Trans_binlog_info()
+ : Repl_semi_sync_trx_info(file_name_buf, 0)
+{
+ file_name_buf[0]= '\0';
+}
+
+
+Trans_binlog_info::Trans_binlog_info(const char *file_name,
+ my_off_t log_pos)
weird indentation
+ : Repl_semi_sync_trx_info(file_name_buf, log_pos)
+{
+ strmake_buf(file_name_buf, file_name);
+}
+
+
+Trans_binlog_info::Trans_binlog_info(const char *file_name,
+ my_off_t log_pos,
+ const rpl_gtid *gtid)
weird indentation
_______________________________________________
developers mailing list -- [email protected]
To unsubscribe send an email to [email protected]
