maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

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@xxxxxxxxxxx> 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