MDEV-40501: Assertion `info->type == READ_CACHE || info->type == WRITE_CACHE' failed in reinit_io_cache upon CHANGE MASTER

[hemantdangi-gc]

Issue:
CHANGE MASTER ... FOR CHANNEL with a channel name within MAX_CONNECTION_NAME can still overflow the OS file name limit once escaped into the relay log file name. The failed open then leaves the relay log's index IO_CACHE uninitialized, but the CHANGE MASTER error-cleanup path unconditionally calls reset_logs() on it, hitting the assertion in reinit_io_cache().

Solution:
Guard reinit_io_cache() in find_log_pos() with my_b_inited() so an unopened index file returns a clean error instead of asserting. Raise a proper client-visible error from Relay_log_info::init() when the relay log fails to open, guarding the other error paths that raise a more generic error so they don't double-set the diagnostics area. Use MY_SAFE_PATH in open_index_file() so an over-length name fails deterministically instead of silently falling back to a mangled one.

[knielsen]

I do not like those random conditions added in the patch - the if (!my_b_inited(&index_file)), the !thd->is_error(). I wonder if it is not merely working around the problem rather than addressing the root cause.

Why are these extra checks necessary? Why doesn't normal error handling ensure that the right error (and only that) is thrown, and we do not try to access non-initialised index_file?

I'm thinking perhaps the real problem is that there is insufficient error handling somewhere on the call chain. What would it take to make sure the error is handled correctly when it occurs, and we avoid getting into subsequent code that then requires these extra checks?

[CLAassistant]

All committers have signed the CLA.

[hemantdangi-gc]

I do not like those random conditions added in the patch - the if (!my_b_inited(&index_file)), the !thd->is_error(). I wonder if it is not merely working around the problem rather than addressing the root cause.

Why are these extra checks necessary? Why doesn't normal error handling ensure that the right error (and only that) is thrown, and we do not try to access non-initialised index_file?

I'm thinking perhaps the real problem is that there is insufficient error handling somewhere on the call chain. What would it take to make sure the error is handled correctly when it occurs, and we avoid getting into subsequent code that then requires these extra checks?

The actual bug is in Master_info_index::remove_master_info() (sql/rpl_mi.cc): when CHANGE MASTER fails, its cleanup path calls mi->rli.relay_log.reset_logs() unconditionally, without checking whether the relay log was ever successfully opened. Relay_log_info::init() can fail before ever opening it (e.g. the file name overflows the OS limit once the channel name is escaped into it), which leaves the relay log's index_file IO_CACHE uninitialized (type == TYPE_NOT_SET). reset_logs() calls find_log_pos(), which calls reinit_io_cache() on that uninitialized cache — that's where the assertion fires (mysys/mf_iocache.c:428).

So the real fix is to check the relay log's actual state before acting on it:

  // sql/rpl_mi.cc, Master_info_index::remove_master_info()
  if (mi->rli.relay_log.is_open())
    mi->rli.relay_log.reset_logs(current_thd, 0, (rpl_gtid*) 0, 0, 0);

With that in place, reset_logs()/find_log_pos() never get called with an unopened log, so the assertion at mf_iocache.c:428 is never reached - no need to guard reinit_io_cache() itself. I've dropped the my_b_inited() check I'd added in find_log_pos() and the !thd->is_error() guards around the error reporting in change_master()/start_slave(), since both were only needed to paper over this same call site. The original unconditional ER_MASTER_INFO error report is correct and sufficient once the actual misuse is fixed.

Also kept the unrelated MY_SAFE_PATH addition to open_index_file()'s fn_format() call, so an over-length name fails deterministically with ENAMETOOLONG instead of fn_format() silently falling back to a mangled name - that one's orthogonal to the crash, just makes the failure path itself well-defined.

Please let me know if updaed patch is ok with changes you suggested.