Brandon Nesterenko <brandon.nesterenko@mariadb.com> writes:
I always appreciate learning the history of why some things exist as they are :)
Oh yes, there's a _lot_ of history in the replication code...
is logged. Reading the whole comment really solidified it. I'll incorporate your feedback.
Thanks Brandon! I think generally the new patch looks good. It's a nice cleanup of the Query_log_event::begin_event() to simply pad with Q_DUMMY instead of special-casing different lengths (which then was buggy due to not being updated as GTID_EVENT was extended). Just one small but important oversight:
case Q_GTID_FLAGS3: { CHECK_SPACE(pos, end, 1); gtid_flags_extra= *pos++; if (gtid_flags_extra & (Gtid_log_event::FL_COMMIT_ALTER_E1 | Gtid_log_event::FL_ROLLBACK_ALTER_E1)) { CHECK_SPACE(pos, end, 8); sa_seq_no = uint8korr(pos); pos+= 8; } } case Q_DUMMY: { + /* + At some point, this query event was translated from a GTID event, with + these Q_DUMMY bytes added to pad the end of the header. We can skip the + rest of processing these vars. Note this is a separate case from the + default to avoid the DBUG_PRINT of an unknown status var. + */ + pos= (const uchar*) end; break; }
The `break` at the end of the prior case Q_GTID_FLAGS3 was lost, that needs to be restored. This part looks a bit odd:
+ int2store(q + Q_STATUS_VARS_LEN_OFFSET, dummy_bytes); + for (size_t i= 0; i < dummy_bytes; i++) + q[Q_DATA_OFFSET + i]= Q_DUMMY; + q+= dummy_bytes; + q[Q_DATA_OFFSET]= 0; /* Zero terminator for empty db */ + q+= Q_DATA_OFFSET + 1;
The line `q+= dummy_bytes;` leaves q with a strange value, which is Q_DATA_OFFSET before the end of the usevar section. I would have done something like this: q+= dummy_bytes + Q_DATA_OFFSET; *q+= 0; /* Zero terminator for empty db */ Or this: q[Q_DATA_OFFSET + dummy_bytes]= 0; /* Zero terminator for empty db */ q+= Q_DATA_OFFSET + dummy_bytes + 1; But I think the current code is correct, so it's up to you what you prefer. - Kristian.