MDEV-34705: Binlog-in-engine: Fix hang with event group of specific size

If the event group fitted in the binlog cache without the GTID event but not
with, the code would attempt to spill part of the GTID event as out-of-band
data, which is not correct. In release builds this would hang the server as
the spilling would try to lock an already owned mutex.

Fix by checking if the GTID event fits, and spilling any non-GTID data as
oob if it does not.

Signed-off-by: Kristian Nielsen <knielsen@knielsen-hq.org>

mysql-test/suite/binlog_in_engine/rpl_oob.result

@@ -15,5 +15,6 @@ SELECT COUNT(*), SUM(a), SUM(b), SUM(LENGTH(c)) FROM t1;
 COUNT(*)	SUM(a)	SUM(b)	SUM(LENGTH(c))
 2552	33150	128776	5000383
 connection master;
+connection master;
 DROP TABLE t1;
 include/rpl_end.inc

mysql-test/suite/binlog_in_engine/rpl_oob.test

@@ -62,6 +62,20 @@ SELECT COUNT(*), SUM(a), SUM(b), SUM(LENGTH(c)) FROM t1;
 SELECT COUNT(*), SUM(a), SUM(b), SUM(LENGTH(c)) FROM t1;
 
 
+--connection master
+# Test various event group sizes close to the binlog cache size.
+# There was a bug where if the event group fit in the cache without the GTID
+# event, but not with the GTID, then the code would incorrectly attempt to
+# spill part of the GTID event as oob data and the server would hang on
+# incorrect double locking of LOCK_commit_ordered.
+--disable_query_log
+--let $i= 50
+while ($i > 0) {
+  eval INSERT INTO t1 VALUES (1000000, $i, REPEAT('#', @@binlog_cache_size - $i*20));
+  dec $i;
+}
+--enable_query_log
+
 # Cleanup.
 --connection master
 DROP TABLE t1;

sql/log.cc

@@ -7224,6 +7224,22 @@ MYSQL_BIN_LOG::write_gtid_event(THD *thd, IO_CACHE *dest, bool standalone,
     gtid_event.cache_type= is_transactional ?
       Log_event::EVENT_TRANSACTIONAL_CACHE : Log_event::EVENT_STMT_CACHE;
 
+  if (opt_binlog_engine_hton)
+  {
+    DBUG_ASSERT(!direct_write);
+    uint32_t avail= (uint32_t)(dest->write_end - dest->write_pos);
+    if (unlikely(avail < Gtid_log_event::max_size) &&
+        avail < gtid_event.get_size())
+    {
+      /*
+        The GTID event doesn't fit in the cache, so we have to spill the
+        contents as oob event data.
+      */
+      if (my_b_flush_io_cache(dest, 0))
+        DBUG_RETURN(true);
+    }
+  }
+
   /* Write the event to the binary log. */
   DBUG_ASSERT(this == &mysql_bin_log);
 

sql/log_event.h

@@ -3262,6 +3262,40 @@ public:
 };
 
 
+#ifdef MYSQL_SERVER
+/*
+  This is used to compute a compile-time constant max for the size (in bytes)
+  of a GTID event (Gtid_log_event::max_size).
+
+  It is carefully written to take boolean parameters corresponding directly
+  to each conditional in Gtid_log_event::write(), so that the calculation here
+  will match the actual length computed by write().
+
+  Please ensure that that any new conditionals added in write() that affect
+  the event length are similarly extended with a boolean parameter for this
+  function so future code changes do not introduce incorrect result of this
+  function.
+*/
+static constexpr uint32_t
+cap_gtid_event_size(uint32_t proposed_size)
+{
+  /* This just because std::min is not constexpr in c++11. */
+  return LOG_EVENT_HEADER_LEN +
+    (proposed_size < GTID_HEADER_LEN ? GTID_HEADER_LEN : proposed_size);
+}
+static constexpr uint32_t
+get_gtid_event_size(bool fl_commit_id, bool fl_xa, bool fl_extra,
+                    bool fl_multi_engine, bool fl_alter,
+                    int bq_size, int gt_size)
+{
+  return cap_gtid_event_size((fl_commit_id ? GTID_HEADER_LEN + 2 : 13) +
+                             (fl_xa ? 6 + bq_size + gt_size : 0) +
+                             (fl_extra ? 1 : 0) +
+                             (fl_multi_engine ? 1 : 0) +
+                             (fl_alter ? 8 : 0));
+}
+#endif
+
 /**
   @class Gtid_log_event
 
@@ -3392,12 +3426,19 @@ public:
     involving multiple storage engines. No flag and extra data are added
     to the event when the transaction involves only one engine.
   */
-  static const uchar FL_EXTRA_MULTI_ENGINE_E1= 1;
-  static const uchar FL_START_ALTER_E1= 2;
-  static const uchar FL_COMMIT_ALTER_E1= 4;
-  static const uchar FL_ROLLBACK_ALTER_E1= 8;
+  static constexpr uchar FL_EXTRA_MULTI_ENGINE_E1= 1;
+  static constexpr uchar FL_START_ALTER_E1= 2;
+  static constexpr uchar FL_COMMIT_ALTER_E1= 4;
+  static constexpr uchar FL_ROLLBACK_ALTER_E1= 8;
 
 #ifdef MYSQL_SERVER
+  static constexpr uint32_t max_size=
+    get_gtid_event_size(FL_GROUP_COMMIT_ID,
+                        (bool)(FL_PREPARED_XA|FL_COMPLETED_XA),
+                        true, FL_EXTRA_MULTI_ENGINE_E1,
+                        (bool)(FL_COMMIT_ALTER_E1|FL_ROLLBACK_ALTER_E1),
+                        MAXBQUALSIZE, MAXGTRIDSIZE);
+
   Gtid_log_event(THD *thd_arg, uint64 seq_no, uint32 domain_id, bool standalone,
                  uint16 flags, bool is_transactional, uint64 commit_id,
                  bool has_xid= false, bool is_ro_1pc= false);
@@ -3430,6 +3471,7 @@ public:
   }
 
 #ifdef MYSQL_SERVER
+  uint32_t get_size() const noexcept;
   bool write(Log_event_writer *writer) override;
   static int make_compatible_event(String *packet, bool *need_dummy_event,
                                     ulong ev_offset, enum_binlog_checksum_alg checksum_alg);

sql/log_event_server.cc

@@ -2973,6 +2973,18 @@ Gtid_log_event::peek(const uchar *event_start, size_t event_len,
 }
 
 
+uint32_t
+Gtid_log_event::get_size() const noexcept
+{
+  return get_gtid_event_size(flags2 & FL_GROUP_COMMIT_ID,
+                             flags2 & (FL_PREPARED_XA | FL_COMPLETED_XA),
+                             flags_extra > 0,
+                             flags_extra & FL_EXTRA_MULTI_ENGINE_E1,
+                             flags_extra & (FL_COMMIT_ALTER_E1 | FL_ROLLBACK_ALTER_E1),
+                             xid.bqual_length, xid.gtrid_length);
+}
+
+
 bool
 Gtid_log_event::write(Log_event_writer *writer)
 {
@@ -3042,6 +3054,17 @@ Gtid_log_event::write(Log_event_writer *writer)
     bzero(buf+write_len, GTID_HEADER_LEN-write_len);
     write_len= GTID_HEADER_LEN;
   }
+  /*
+    Whenever updating this function, make sure that Gtid_log_event::get_size()
+    still computes the same consistent event length! Do not just rely on this
+    assertion, in case test coverage is not 100%.
+  */
+  DBUG_ASSERT(DBUG_IF("negate_xid_from_gtid") ||
+              DBUG_IF("negate_xid_data_from_gtid") ||
+              DBUG_IF("negate_alter_fl_from_gtid") ||
+              DBUG_IF("inject_fl_extra_multi_engine_into_gtid") ||
+              write_len + LOG_EVENT_HEADER_LEN == get_size());
+
   return write_header(writer, write_len) ||
          write_data(writer, buf, write_len) ||
          write_footer(writer);

storage/innobase/handler/innodb_binlog.cc

@@ -302,22 +302,7 @@ struct chunk_data_cache : public chunk_data_base {
     header_remain= (uint32_t)(p - header_buf);
     ut_ad((size_t)(p - header_buf) <= sizeof(header_buf));
 
-    if (cache->pos_in_file > binlog_info->out_of_band_offset) {
-      /*
-        ToDo: A limitation in mysys IO_CACHE. If I change (reinit_io_cache())
-        the cache from WRITE_CACHE to READ_CACHE without seeking out of the
-        current buffer, then the cache will not be flushed to disk (which is
-        good for small cache that fits completely in buffer). But then if I
-        later my_b_seek() or reinit_io_cache() it again and seek out of the
-        current buffer, the buffered data will not be flushed to the file
-        because the cache is now a READ_CACHE! The result is that the end of the
-        cache will be lost if the cache doesn't fit in memory.
-
-        So for now, have to do this somewhat in-elegant conditional flush
-        myself.
-      */
-      flush_io_cache(cache);
-    }
+    ut_ad (cache->pos_in_file <= binlog_info->out_of_band_offset);
 
     /* Start with the GTID event, which is put at the end of the IO_CACHE. */
     my_bool res= reinit_io_cache(cache, READ_CACHE, binlog_info->gtid_offset, 0, 0);