Re: [Maria-developers] 4af7a583802: Refactor MYSQL_BIN_LOG: extract Event_log ancestor
Hi, Nikita! On Nov 12, Nikita Malyavin wrote:
revision-id: 4af7a583802 (mariadb-10.5.2-474-g4af7a583802) parent(s): 2da3dc5d422 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2021-01-08 20:59:09 +1000 message:
Refactor MYSQL_BIN_LOG: extract Event_log ancestor
Would be good to have some explanation here. I understand that you'll use this refactoring in following commits, but conceptually, what Event_log is? I mean, like Event_log is a log where Log_event_xxx events are written, but not _something else that MYSQL_BIN_LOG is and Event_log isn't_ or Event_log is a log where Log_event_xxx events are written. While MYSQL_BIN_LOG is Event_log that also does ....
diff --git a/sql/log.cc b/sql/log.cc index 827a34c2883..6f4e3191588 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -3701,64 +3765,25 @@ bool MYSQL_BIN_LOG::open(const char *log_name, write_file_name_to_index_file= 1; }
a bit strange that MYSQL_BIN_LOG::open doesn't use Event_log::open
diff --git a/sql/log.h b/sql/log.h index 73cd66d5a4a..74c409e1ac7 100644 --- a/sql/log.h +++ b/sql/log.h @@ -327,6 +327,7 @@ class MYSQL_LOG bool strip_ext, char *buff); virtual int generate_new_name(char *new_name, const char *log_name, ulong next_log_number); + inline mysql_mutex_t* get_log_lock() { return &LOCK_log; } protected: /* LOCK_log is inited by init_pthread_objects() */ mysql_mutex_t LOCK_log; @@ -376,6 +377,64 @@ struct Rows_event_factory } };
+class Event_log: public MYSQL_LOG +{ +public: +#if !defined(MYSQL_CLIENT) + Rows_log_event* + prepare_pending_rows_event(THD *thd, TABLE* table, + binlog_cache_data *cache_data, + uint32 serv_id, size_t needed, + bool is_transactional, + Rows_event_factory event_factory);
forgot to delete the same from MYSQL_BIN_LOG
+#endif + + bool flush_and_sync(bool *synced);
same
+ void cleanup() + { + if (inited) + mysql_mutex_destroy(&LOCK_binlog_end_pos); + + MYSQL_LOG::cleanup(); + } + void init_pthread_objects() + { + MYSQL_LOG::init_pthread_objects(); + + mysql_mutex_init(m_key_LOCK_binlog_end_pos, &LOCK_binlog_end_pos, + MY_MUTEX_INIT_SLOW); + } + + bool open( + const char *log_name, + enum_log_type log_type, + const char *new_name, ulong next_file_number, + enum cache_type io_cache_type_arg); + IO_CACHE *get_log_file() { return &log_file; } + + int write_description_event(enum_binlog_checksum_alg checksum_alg, + bool encrypt, bool dont_set_created); + + bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); +}; + /* Tell the io thread if we can delay the master info sync. */ #define SEMI_SYNC_SLAVE_DELAY_SYNC 1 /* Tell the io thread if the current event needs a ack. */ @@ -451,7 +510,7 @@ class binlog_cache_data; struct rpl_gtid; struct wait_for_commit;
-class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG +class MYSQL_BIN_LOG: public TC_LOG, private Event_log { /** The instrumentation key to use for @ LOCK_index. */ PSI_mutex_key m_key_LOCK_index; @@ -849,15 +901,13 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG
- bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); + bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file) + { return Event_log::write_event(ev, data, file); }
Perhaps using Event_log::write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); ?
bool write_event(Log_event *ev) { return write_event(ev, 0, &log_file); }
bool write_event_buffer(uchar* buf,uint len); @@ -918,7 +968,6 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG uint next_file_id(); inline char* get_index_fname() { return index_file_name;} inline char* get_log_fname() { return log_file_name; } - inline char* get_name() { return name; } inline mysql_mutex_t* get_log_lock() { return &LOCK_log; }
should remove that too, I reckon
inline mysql_cond_t* get_bin_log_cond() { return &COND_bin_log_updated; } inline IO_CACHE* get_log_file() { return &log_file; }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hello Serg! TLDR the commit is verbalized, junks are removed, `using` is preferred except for tiny getters. On Fri, 12 Nov 2021 at 19:20, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita!
On Nov 12, Nikita Malyavin wrote:
revision-id: 4af7a583802 (mariadb-10.5.2-474-g4af7a583802) parent(s): 2da3dc5d422 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2021-01-08 20:59:09 +1000 message:
Refactor MYSQL_BIN_LOG: extract Event_log ancestor
Would be good to have some explanation here. I understand that you'll use this refactoring in following commits, but conceptually, what Event_log is?
I mean, like
Event_log is a log where Log_event_xxx events are written, but not _something else that MYSQL_BIN_LOG is and Event_log isn't_
or
Event_log is a log where Log_event_xxx events are written. While MYSQL_BIN_LOG is Event_log that also does ....
Right. Adding as follows: Event_log is supposed to be a basic logging class that can write events in a single file. MYSQL_BIN_LOG in comparison will have: * rotation support * index files * purging * gtid, xid and other transactional information handling. * is dedicated for a general-purpose binlog
diff --git a/sql/log.cc b/sql/log.cc index 827a34c2883..6f4e3191588 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -3701,64 +3765,25 @@ bool MYSQL_BIN_LOG::open(const char *log_name, write_file_name_to_index_file= 1; }
a bit strange that MYSQL_BIN_LOG::open doesn't use Event_log::open
Yes, I think MYSQL_BIN_LOG is left too specialized for some particular needs. I did not want to touch all that tech debt and left as is. Besides, there's nothing weird in not calling the superclass's method. I mean, in future commits [spoiler alert!] it will be used anyway:)
diff --git a/sql/log.h b/sql/log.h index 73cd66d5a4a..74c409e1ac7 100644 --- a/sql/log.h +++ b/sql/log.h @@ -327,6 +327,7 @@ class MYSQL_LOG bool strip_ext, char *buff); virtual int generate_new_name(char *new_name, const char *log_name, ulong next_log_number); + inline mysql_mutex_t* get_log_lock() { return &LOCK_log; } protected: /* LOCK_log is inited by init_pthread_objects() */ mysql_mutex_t LOCK_log; @@ -376,6 +377,64 @@ struct Rows_event_factory } };
+class Event_log: public MYSQL_LOG +{ +public: +#if !defined(MYSQL_CLIENT) + Rows_log_event* + prepare_pending_rows_event(THD *thd, TABLE* table, + binlog_cache_data *cache_data, + uint32 serv_id, size_t needed, + bool is_transactional, + Rows_event_factory event_factory);
forgot to delete the same from MYSQL_BIN_LOG
+#endif + + bool flush_and_sync(bool *synced);
same
wow, i wonder how did you notice.
+ void cleanup() + { + if (inited) + mysql_mutex_destroy(&LOCK_binlog_end_pos); + + MYSQL_LOG::cleanup(); + } + void init_pthread_objects() + { + MYSQL_LOG::init_pthread_objects(); + + mysql_mutex_init(m_key_LOCK_binlog_end_pos, &LOCK_binlog_end_pos, + MY_MUTEX_INIT_SLOW); + } + + bool open( + const char *log_name, + enum_log_type log_type, + const char *new_name, ulong next_file_number, + enum cache_type io_cache_type_arg); + IO_CACHE *get_log_file() { return &log_file; } + + int write_description_event(enum_binlog_checksum_alg checksum_alg, + bool encrypt, bool dont_set_created); + + bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); +}; + /* Tell the io thread if we can delay the master info sync. */ #define SEMI_SYNC_SLAVE_DELAY_SYNC 1 /* Tell the io thread if the current event needs a ack. */ @@ -451,7 +510,7 @@ class binlog_cache_data; struct rpl_gtid; struct wait_for_commit;
-class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG +class MYSQL_BIN_LOG: public TC_LOG, private Event_log { /** The instrumentation key to use for @ LOCK_index. */ PSI_mutex_key m_key_LOCK_index; @@ -849,15 +901,13 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG
- bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); + bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file) + { return Event_log::write_event(ev, data, file); }
Perhaps
using Event_log::write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file);
?
Indeed:)
bool write_event(Log_event *ev) { return write_event(ev, 0, &log_file); }
bool write_event_buffer(uchar* buf,uint len); @@ -918,7 +968,6 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG uint next_file_id(); inline char* get_index_fname() { return index_file_name;} inline char* get_log_fname() { return log_file_name; } - inline char* get_name() { return name; } inline mysql_mutex_t* get_log_lock() { return &LOCK_log; }
should remove that too, I reckon
Well, these are so tiny getters so I'm not sure, whether is it more comfortable to jump back and forth to check what's there in Event_log, or just admit the cost of duplication for a better read comfort. I'd leave it as it is now.
inline mysql_cond_t* get_bin_log_cond() { return &COND_bin_log_updated; } inline IO_CACHE* get_log_file() { return &log_file; }
-- Yours truly, Nikita Malyavin
The changes to this commit are secured in 15fe904d3 <https://github.com/MariaDB/server/commit/15fe904d3fd4583f1966c8a7ab6ff36111f1f920> fixup On Tue, 23 Nov 2021 at 22:54, Nikita Malyavin <nikitamalyavin@gmail.com> wrote:
Hello Serg!
TLDR the commit is verbalized, junks are removed, `using` is preferred except for tiny getters.
On Fri, 12 Nov 2021 at 19:20, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita!
On Nov 12, Nikita Malyavin wrote:
revision-id: 4af7a583802 (mariadb-10.5.2-474-g4af7a583802) parent(s): 2da3dc5d422 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2021-01-08 20:59:09 +1000 message:
Refactor MYSQL_BIN_LOG: extract Event_log ancestor
Would be good to have some explanation here. I understand that you'll use this refactoring in following commits, but conceptually, what Event_log is?
I mean, like
Event_log is a log where Log_event_xxx events are written, but not _something else that MYSQL_BIN_LOG is and Event_log isn't_
or
Event_log is a log where Log_event_xxx events are written. While MYSQL_BIN_LOG is Event_log that also does ....
Right. Adding as follows:
Event_log is supposed to be a basic logging class that can write events in a single file.
MYSQL_BIN_LOG in comparison will have: * rotation support * index files * purging * gtid, xid and other transactional information handling. * is dedicated for a general-purpose binlog
diff --git a/sql/log.cc b/sql/log.cc index 827a34c2883..6f4e3191588 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -3701,64 +3765,25 @@ bool MYSQL_BIN_LOG::open(const char *log_name, write_file_name_to_index_file= 1; }
a bit strange that MYSQL_BIN_LOG::open doesn't use Event_log::open
Yes, I think MYSQL_BIN_LOG is left too specialized for some particular needs. I did not want to touch all that tech debt and left as is.
Besides, there's nothing weird in not calling the superclass's method.
I mean, in future commits [spoiler alert!] it will be used anyway:)
diff --git a/sql/log.h b/sql/log.h index 73cd66d5a4a..74c409e1ac7 100644 --- a/sql/log.h +++ b/sql/log.h @@ -327,6 +327,7 @@ class MYSQL_LOG bool strip_ext, char *buff); virtual int generate_new_name(char *new_name, const char *log_name, ulong next_log_number); + inline mysql_mutex_t* get_log_lock() { return &LOCK_log; } protected: /* LOCK_log is inited by init_pthread_objects() */ mysql_mutex_t LOCK_log; @@ -376,6 +377,64 @@ struct Rows_event_factory } };
+class Event_log: public MYSQL_LOG +{ +public: +#if !defined(MYSQL_CLIENT) + Rows_log_event* + prepare_pending_rows_event(THD *thd, TABLE* table, + binlog_cache_data *cache_data, + uint32 serv_id, size_t needed, + bool is_transactional, + Rows_event_factory event_factory);
forgot to delete the same from MYSQL_BIN_LOG
+#endif + + bool flush_and_sync(bool *synced);
same
wow, i wonder how did you notice.
+ void cleanup() + { + if (inited) + mysql_mutex_destroy(&LOCK_binlog_end_pos); + + MYSQL_LOG::cleanup(); + } + void init_pthread_objects() + { + MYSQL_LOG::init_pthread_objects(); + + mysql_mutex_init(m_key_LOCK_binlog_end_pos, &LOCK_binlog_end_pos, + MY_MUTEX_INIT_SLOW); + } + + bool open( + const char *log_name, + enum_log_type log_type, + const char *new_name, ulong next_file_number, + enum cache_type io_cache_type_arg); + IO_CACHE *get_log_file() { return &log_file; } + + int write_description_event(enum_binlog_checksum_alg checksum_alg, + bool encrypt, bool dont_set_created); + + bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); +}; + /* Tell the io thread if we can delay the master info sync. */ #define SEMI_SYNC_SLAVE_DELAY_SYNC 1 /* Tell the io thread if the current event needs a ack. */ @@ -451,7 +510,7 @@ class binlog_cache_data; struct rpl_gtid; struct wait_for_commit;
-class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG +class MYSQL_BIN_LOG: public TC_LOG, private Event_log { /** The instrumentation key to use for @ LOCK_index. */ PSI_mutex_key m_key_LOCK_index; @@ -849,15 +901,13 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG
- bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file); + bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file) + { return Event_log::write_event(ev, data, file); }
Perhaps
using Event_log::write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file);
?
Indeed:)
bool write_event(Log_event *ev) { return write_event(ev, 0, &log_file); }
bool write_event_buffer(uchar* buf,uint len); @@ -918,7 +968,6 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG uint next_file_id(); inline char* get_index_fname() { return index_file_name;} inline char* get_log_fname() { return log_file_name; } - inline char* get_name() { return name; } inline mysql_mutex_t* get_log_lock() { return &LOCK_log; }
should remove that too, I reckon
Well, these are so tiny getters so I'm not sure, whether is it more comfortable to jump back and forth to check what's there in Event_log, or just admit the cost of duplication for a better read comfort. I'd leave it as it is now.
inline mysql_cond_t* get_bin_log_cond() { return &COND_bin_log_updated; } inline IO_CACHE* get_log_file() { return &log_file; }
-- Yours truly, Nikita Malyavin
-- Yours truly, Nikita Malyavin
Hi, Nikita! On Nov 23, Nikita Malyavin wrote:
Right. Adding as follows:
Event_log is supposed to be a basic logging class that can write events in a single file.
MYSQL_BIN_LOG in comparison will have: * rotation support * index files * purging * gtid, xid and other transactional information handling. * is dedicated for a general-purpose binlog
Great!
diff --git a/sql/log.h b/sql/log.h index 73cd66d5a4a..74c409e1ac7 100644 --- a/sql/log.h +++ b/sql/log.h @@ -918,7 +968,6 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG uint next_file_id(); inline char* get_index_fname() { return index_file_name;} inline char* get_log_fname() { return log_file_name; } - inline char* get_name() { return name; } inline mysql_mutex_t* get_log_lock() { return &LOCK_log; }
should remove that too, I reckon
Well, these are so tiny getters so I'm not sure, whether is it more comfortable to jump back and forth to check what's there in Event_log, or just admit the cost of duplication for a better read comfort. I'd leave it as it is now.
What I mean is, you moved get_log_lock() to MYSQL_LOG class. You don't need a second copy here, it's redundant. Except that MYSQL_LOG is private here and you want get_log_lock() public, apparently? Okay then it's needed here too. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Nikita Malyavin -
Sergei Golubchik