Oh! I like this one more, too. Alright!

On Thu, 10 Aug 2023 at 18:53, Sergei Golubchik <serg@mariadb.org> wrote:

Hi, Nikita,

On Aug 09, Nikita Malyavin wrote:
> > > diff --git a/sql/log_event.cc b/sql/log_event.cc
> > > index 10180a95d99..0c4c284a40f 100644
> > > --- a/sql/log_event.cc
> > > +++ b/sql/log_event.cc
> > > @@ -761,16 +761,21 @@ Log_event::Log_event(const uchar *buf,
> > >
> > >  int Log_event::read_log_event(IO_CACHE* file, String* packet,
> > >                                const Format_description_log_event *fdle,
> > > -                              enum enum_binlog_checksum_alg checksum_alg_arg)
> > > +                              enum enum_binlog_checksum_alg checksum_alg_arg,
> > > +                              size_t max_allowed_packet)
> > >  {
> > >    ulong data_len;
> > >    char buf[LOG_EVENT_MINIMAL_HEADER_LEN];
> > >    uchar ev_offset= packet->length();
> > >  #if !defined(MYSQL_CLIENT)
> > > -  THD *thd=current_thd;
> > > -  ulong max_allowed_packet= thd ? thd->slave_thread ? slave_max_allowed_packet
> > > -                                                    : thd->variables.max_allowed_packet
> > > -                                : ~(uint)0;
> > > +  if (max_allowed_packet == 0)
> >
> > may be better pass THD as an argument?
> > or even correct value of max_allowed_packet. As
> >
> >   ulong max_allowed_packet(THD *thd)
> >   { return thd ? ... : ~(uint)0; }
>
> I thought about it, but let me avoid it this time. There are ~15 calls of
> both read_log_event functions, and it's hard to say what's called from
> where.
> I'd better leave this insane knot as it is now.
>
> Also as a bonus mysql_bin_log will not depend on an external
> max_allowed_packet, which is fixed as 1GiB.

still, I don't like a run-time condition for what should be a
compile-time thing. Another way of solving it is

   static int read_log_event(IO_CACHE* file, String* packet,
                             const Format_description_log_event *fdle,
                             enum enum_binlog_checksum_alg checksum_alg_arg)
   {
     size_t max_packet= thd ? ... : ~0U;
     return read_log_event(file, packet, fdle, checksum_alg_arg, max_packet);
   }

   static int read_log_event(IO_CACHE* file, String* packet,
                             const Format_description_log_event *fdle,
                             enum enum_binlog_checksum_alg checksum_alg_arg,
                             size_t max_allowed_packet);

> > on the second thought, do you need to ignore max_allowed_packet
> > here for online ALTER? Is there no event size limit when the
> > event is written into binlog?
> >
> > > +  {
> > > +    THD *thd=current_thd;
> > > +    max_allowed_packet= thd ? thd->slave_thread
> > > +                              ? slave_max_allowed_packet
> > > +                              : thd->variables.max_allowed_packet
> > > +                            : ~(uint)0;
> > > +  }
> > >  #endif
> > >    DBUG_ENTER("Log_event::read_log_event(IO_CACHE*,String*...)");
> >
Regards,
Sergei
VP of MariaDB Server Engineering
and security@mariadb.org

--

Yours truly,

Nikita Malyavin