Hi, Andrei! On Mar 22, Andrei Elkin wrote:
Here it goes.
As after master crash and failover to slave, the demoted-to-slave ex-master must be ready to face and accept its own (generated by) events, without generally necessary --replicate-same-server-id.
So the acceptance conditions are refined/relaxed for the semisync slave connected to master in the GTID mode which ensures (under gtid_strict_mode ON) that there can't be any duplicate events for execution on such an ex-master slave. Non-GTID "binlog-truncated" ex-master semisync slave has to be configured with --replicate-same-server-id.
Why would you generally care about same server id in the gtid mode? If gtid is not present locally, then it should be applied, right? Why to look at server id at all? It's a thing of the past, from pre-gtid times.
+--let $query1 = INSERT INTO t VALUES (20) +--let $query2 = DELETE FROM t2 WHERE f = 666 /* no such record */ +--source binlog_truncate_active_log.inc + +--echo # Case B. +# The inverted sequence ends up to truncate only $query2 +--let $this_search_pattern = Successfully truncated.*to remove transactions starting from GTID 0-1-10 +--let $query1 = DELETE FROM t2 WHERE f = 0
why not `= 666 /* no such record */` ? is `= 0` important here?
Yes, there's a fine detail here.
'The inverted sequence ends up' to leave in binlog
$query1 = DELETE FROM t2 WHERE f = 0
which is as ineffective as 666. The patch recognizes the ineffectiveness through a special value of engines involved counter in Gtid event. In principle the truncation could start from it rather than the following $query2. But that would mean more complicated coding. So the semantics of the zero is to be good, as we're so used to :-)!
Sorry, I didn't understand anything. What does the value in the WHERE clause has to do with engines and where the binlog is truncated?
ok. after discussing the issue on slack, the answer appears to be "the value in WHERE has nothing to do with any of that, it doesn't matter whether it's 0 or 666 as long as it doesn't match any rows" then, I'd say, better change 0 to 667 in the test. We wouldn't be having this discussion at all if you'd have some number that appeared arbitrary (and "667" is fine). My confusion was exactly caused by the fact that it looked like =0 matters, that it was important to hav 0 there.
+--let $query2 = INSERT INTO t VALUES (20) +--source binlog_truncate_active_log.inc ... diff --git a/sql/sql_class.h b/sql/sql_class.h index 140394fefc1..87fa88d4c89 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -4632,7 +4632,7 @@ class THD :public Statement, LF_PINS *tdc_hash_pins; LF_PINS *xid_hash_pins; bool fix_xid_hash_pins(); - + bool is_1pc_ro_trans;
I don't like how it looks, it's in THD (for every single connection and more) and it's only used in some exotic cases. It's set in two places around ha_commit_one_phase(), when it would be better to set it inside ha_commit_one_phase(), in one place only.
But I cannot suggest how to get rid of it yet as I don't understand what it's for, I've asked a question about it below, in Gtid_log_event::Gtid_log_event
The flag is to help sorting out ineffective (no engine data change) STATEMENT binlog-format logged transaction. I thought it's practical enough to face and important to handle. Through an example, let the table `t' be initially empty, and let binlog contain
trx1: INSERT INTO t SET a = 1; /* slave acked, trx got committed in engine */ trx2: INSERT INTO t SET a = 2; /* not acked, trx remains prepared in engine */ trx3: DELETE FROM t WHERE a = 0; /* ineffective, does not have Xid event */
prior a crash with their states as commented. The truncate trx should be trx2 as the trx3's status suggests it's harmless for involving into truncation. However we have yet to distinguish it from unsafe (to truncate) event group ("transaction") that also ends with COMMIT query not Xid event.
Hmm. I see. But thd isn't a universal dumpster for passing arguments to the function, don't use it like that.
Well, I had to resort to that. To explain,
Please, pass this "read-only transaction with no xid" flag to the Gtid_log_event constructor as an argument. Or recalculate it inside the constructor, it's not such an expensive thing to do.
To pass a flag ensues adding an extra arg to struct handlerton::commit which I don't feel easy about considering the reason.
yes, one more reason to remove binlog_hton. But for now, like with binlog_commit_by_xid, I'd say, call binlog_commit directly and make binlog_hton->commit a no-op. Like: --- a/sql/handler.cc +++ b/sql/handler.cc @@ -1935,6 +1935,7 @@ commit_one_phase_2(THD *thd, bool all, THD_TRANS *trans, if (ha_info) { + binlog_commit(ht, thd, all); for (; ha_info; ha_info= ha_info_next) { int err;
+ if (!truncate_validated) + { + DBUG_ASSERT(round <= 2); + /* + Either truncate was not set or was reset, else + it gets incremented, otherwise it may only set to an earlier + offset only at the turn out of the 1st round.
sorry, I cannot parse that :(
The comments are for the assert in a branch that acts when the truncate position (gtid) is not yet finalized. Let me annotate under each assert's line.
+ */ + DBUG_ASSERT(truncate_gtid.seq_no == 0 ||
'truncate was not set or was reset' ^, that is truncate_gtid. The 'was reset' is actually not present in the assert - that'd be reflected with truncate_reset_done == true
+ last_gtid_coord >= binlog_truncate_coord ||
'it gets incremented' ^
why does that mean "it gets incremented" ?
I think my English failed me.
It is getting incremented
I understand that "it gets incremented" means "it = it + 1", no problem. How this assert verifies that "it gets incremented" ?
meant to say (in my last reply as well). The use case is that at this point there's a truncate candidate /* 1 */ and it has not been settled yet /* 2 */,
!truncate_validated == true /* 2 */ && truncate_gtid.seq_no > 0 /* 1 */
and the current gtid is still "better" to take over as the candidate in this block. The candidate's offset must not decrease (unless...
+ (round == 2 && truncate_set_in_1st));
And if it's neither of the above there must've been truncate candidate set in the 1st round.
... ... the above ).
switch (typ) { + case FORMAT_DESCRIPTION_EVENT: + if (round > 1) + { + if (fdle != fdle_arg) + delete fdle; + fdle= (Format_description_log_event*) ev;
why is it needed? all binlog files should have the same Format_description_log_event anyway.
Actually contrary to what was stated (the patch removed that claim) in the parent revision of this commit, the FD:s of the recovery binlog files may have different checksums. I.e
set @@global.binlog_checksum = value
can be issued to rotate binlog to a new hot file having different checksum value while the binlog *CheckPoint* file is behind. So this is possible to have at recovery:
B_1(CP, checksum = NONE), B_2(hot, checksum = CRC32)
which also manifests a normal use case bug I have not reported any bug (though wanted to).
okay, checksum can differ. But how does it affect Format_description_log_event ?
FD holds knowledge of whether events in the file are with the checksum info. In my example above B_1 events would be (are) considered as checksummed by the base.
Okay. Please, 1. add a comment about it 2. add a test case for it ideally, also, move it into a separate commit Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org