MySQL Bugs: #109435: better to use wdlock for global_sid_lock when only read gtid_state->get

Bug #109435	better to use wdlock for global_sid_lock when only read gtid_state->get_executed
Submitted:	20 Dec 2022 8:36	Modified:	20 Dec 2022 14:13
Reporter:	dennis GAO (OCA)	Email Updates:
Status:	Verified	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S5 (Performance)
Version:	8.0	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
Currently, if one transaction try to read gtid_state->get_executed, it will always acquire global_sid_lock->wrlock.
IMO, global_sid_lock->rdlock is enough.

The following two codes are some examples:

1. the following code, gtid_state->get_executed_gtids is only need to be read a copy and store into m_gtid_set, which is a session side variable.

bool Session_consistency_gtids_ctx::notify_after_transaction_commit(
    const THD *thd) {
  DBUG_TRACE;
  assert(thd);
  bool res = false;

  if (!shall_collect(thd)) return res;

  if (m_curr_session_track_gtids == SESSION_TRACK_GTIDS_ALL_GTIDS) {
    /*  
     If one is configured to read all writes, we always collect
     the GTID_EXECUTED.

     NOTE: in the future optimize to collect deltas instead maybe.
    */
    global_sid_lock->wrlock();
    res = m_gtid_set->add_gtid_set(gtid_state->get_executed_gtids()) !=
          RETURN_STATUS_OK;
    global_sid_lock->unlock();

    if (!res) notify_ctx_change_listener();
  }

  return res;
}

2. the following code only read the length of executed_gtids, there is no need to acquire rw lock

class Sys_var_gtid_executed : Sys_var_charptr_func {
 public:
  Sys_var_gtid_executed(const char *name_arg, const char *comment_arg)
      : Sys_var_charptr_func(name_arg, comment_arg, GLOBAL) {}

  const uchar *global_value_ptr(THD *thd, std::string_view) override {
    DBUG_TRACE;
    global_sid_lock->wrlock();
    const Gtid_set *gs = gtid_state->get_executed_gtids();
    char *buf = (char *)thd->alloc(gs->get_string_length() + 1);
    if (buf == nullptr)
      my_error(ER_OUT_OF_RESOURCES, MYF(0));
    else 
      gs->to_string(buf);
    global_sid_lock->unlock();
    return (uchar *)buf;
  }
};

How to repeat:
null

Suggested fix:
if only need to read gtid_state->get_executed_gtids, use global_sid_lock->rdlock rather than global_sid_lock->wrlock.

Hi Mr. gao,

Thank you for your bug report.

We totally agree with you.

Verified as reported.

Hi Mr. gao,

I don't think your improvement is wise, which would Cause some issues.

For performance or concurrency, it acquires a rd lock in `global_sid_lock` to add transaction owned GTID into global `executed_gtids` during committing the transaction.

Obviously, it will be wrong when reading the executed_gtids with the rd lock in `global_sid_lock`.

Hi Mr. xu,

Thank you for your contribution.