Skip to content

Commit f40ea8f

Browse files
longjinvansvoj
longjinvan
authored and
svoj
committed
MDEV-39548 Further cleanup of MDL_request boilerplate
- Change thd->backup_commit_lock from MDL_request* to MDL_ticket*, and convert related call sites in handler.cc, log.cc, sql_class.cc, and xa.cc to use MDL_ACQUIRE_LOCK. - Convert reload_acl_and_cache() in sql_reload.cc, purge_master_logs() and reset_master() in sql_repl.cc to MDL_ACQUIRE_LOCK, holding the ticket in a local MDL_ticket* (these functions used a local MDL_request originally and should not touch thd->backup_commit_lock). - Convert acquire_lock() in partition_info.cc to MDL_ACQUIRE_LOCK. - Add MDL_REQUEST_LIST_ADD() helper for enqueuing lock requests into MDL_request_list, and convert call sites in sql_base.cc and sp.cc. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
1 parent 02e1853 commit f40ea8f

11 files changed

Lines changed: 99 additions & 109 deletions

File tree

‎sql/handler.cc‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1852,7 +1852,7 @@ int ha_commit_trans(THD *thd, bool all)
18521852
&no_rollback);
18531853
/* rw_trans is TRUE when we in a transaction changing data */
18541854
bool rw_trans= is_real_trans && rw_ha_count > 0;
1855-
MDL_request mdl_backup;
1855+
bool backup_commit_lock_acquired= false;
18561856
DBUG_PRINT("info", ("is_real_trans: %d rw_trans: %d rw_ha_count: %d",
18571857
is_real_trans, rw_trans, rw_ha_count));
18581858

@@ -1872,19 +1872,17 @@ int ha_commit_trans(THD *thd, bool all)
18721872
We allow the owner of FTWRL to COMMIT; we assume that it knows
18731873
what it does.
18741874
*/
1875-
MDL_REQUEST_INIT(&mdl_backup, MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT,
1876-
MDL_EXPLICIT);
1877-
18781875
if (!WSREP(thd))
18791876
{
1880-
if (thd->mdl_context.acquire_lock(&mdl_backup,
1881-
thd->variables.lock_wait_timeout))
1877+
if (!(thd->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK(
1878+
MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT,
1879+
MDL_EXPLICIT, thd->variables.lock_wait_timeout)))
18821880
{
18831881
my_error(ER_ERROR_DURING_COMMIT, MYF(0), 1);
18841882
ha_rollback_trans(thd, all);
18851883
DBUG_RETURN(1);
18861884
}
1887-
thd->backup_commit_lock= &mdl_backup;
1885+
backup_commit_lock_acquired= true;
18881886
}
18891887
DEBUG_SYNC(thd, "ha_commit_trans_after_acquire_commit_lock");
18901888
}
@@ -2134,17 +2132,16 @@ int ha_commit_trans(THD *thd, bool all)
21342132
thd->rgi_slave->is_parallel_exec);
21352133
}
21362134
end:
2137-
// reset the pointer to the ticket when it's stack instantiated
2138-
if (thd->backup_commit_lock == &mdl_backup)
2135+
// release the backup commit lock if we acquired it in this function
2136+
if (backup_commit_lock_acquired && thd->backup_commit_lock)
21392137
{
21402138
/*
21412139
We do not always immediately release transactional locks
21422140
after ha_commit_trans() (see uses of ha_enable_transaction()),
21432141
thus we release the commit blocker lock as soon as it's
21442142
not needed.
21452143
*/
2146-
if (mdl_backup.ticket)
2147-
thd->mdl_context.release_lock(mdl_backup.ticket);
2144+
thd->mdl_context.release_lock(thd->backup_commit_lock);
21482145
thd->backup_commit_lock= 0;
21492146
}
21502147
#ifdef WITH_WSREP

‎sql/log.cc‎

Lines changed: 20 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -8811,19 +8811,15 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info, my_bool *with_annotate)
88118811
}
88128812
else
88138813
{
8814-
MDL_request mdl_request;
8815-
8816-
MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT,
8817-
MDL_EXPLICIT);
8818-
if (thd->mdl_context.acquire_lock(&mdl_request,
8819-
thd->variables.lock_wait_timeout))
8814+
if (!(thd->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK(
8815+
MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT,
8816+
MDL_EXPLICIT, thd->variables.lock_wait_timeout)))
88208817
DBUG_RETURN(1);
8821-
thd->backup_commit_lock= &mdl_request;
88228818

88238819
if ((res= thd->wait_for_prior_commit()))
88248820
{
8825-
if (mdl_request.ticket)
8826-
thd->mdl_context.release_lock(mdl_request.ticket);
8821+
if (thd->backup_commit_lock)
8822+
thd->mdl_context.release_lock(thd->backup_commit_lock);
88278823
thd->backup_commit_lock= 0;
88288824
DBUG_RETURN(res);
88298825
}
@@ -8833,8 +8829,8 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info, my_bool *with_annotate)
88338829
prev_binlog_id= current_binlog_id;
88348830
res= write_gtid_event(thd, nullptr, true, using_trans, commit_id,
88358831
false, false, false);
8836-
if (mdl_request.ticket)
8837-
thd->mdl_context.release_lock(mdl_request.ticket);
8832+
if (thd->backup_commit_lock)
8833+
thd->mdl_context.release_lock(thd->backup_commit_lock);
88388834
thd->backup_commit_lock= 0;
88398835
if (res)
88408836
goto err;
@@ -8944,7 +8940,6 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info, my_bool *with_annotate)
89448940
&cache_data->engine_binlog_info;
89458941
engine_context->gtid_offset= my_b_tell(file);
89468942
my_off_t binlog_total_bytes;
8947-
MDL_request mdl_request;
89488943
int res;
89498944

89508945
if (engine_context->out_of_band_offset)
@@ -8957,17 +8952,15 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info, my_bool *with_annotate)
89578952
goto engine_fail;
89588953
}
89598954

8960-
MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT,
8961-
MDL_EXPLICIT);
8962-
if (thd->mdl_context.acquire_lock(&mdl_request,
8963-
thd->variables.lock_wait_timeout))
8955+
if (!(thd->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK(
8956+
MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT,
8957+
MDL_EXPLICIT, thd->variables.lock_wait_timeout)))
89648958
goto engine_fail;
8965-
thd->backup_commit_lock= &mdl_request;
89668959

89678960
if (thd->wait_for_prior_commit())
89688961
{
8969-
if (mdl_request.ticket)
8970-
thd->mdl_context.release_lock(mdl_request.ticket);
8962+
if (thd->backup_commit_lock)
8963+
thd->mdl_context.release_lock(thd->backup_commit_lock);
89718964
thd->backup_commit_lock= 0;
89728965
goto engine_fail;
89738966
}
@@ -8985,8 +8978,8 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info, my_bool *with_annotate)
89858978
mysql_mutex_lock(&LOCK_log);
89868979
res= write_gtid_event(thd, cache_data, true, using_trans, commit_id,
89878980
false, false, false);
8988-
if (mdl_request.ticket)
8989-
thd->mdl_context.release_lock(mdl_request.ticket);
8981+
if (thd->backup_commit_lock)
8982+
thd->mdl_context.release_lock(thd->backup_commit_lock);
89908983
thd->backup_commit_lock= 0;
89918984
if (res)
89928985
{
@@ -10258,12 +10251,11 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry)
1025810251
yet have the MDL_BACKUP_COMMIT_LOCK) and any threads using
1025910252
BACKUP LOCK BLOCK_COMMIT.
1026010253
*/
10261-
if (thd->backup_commit_lock && thd->backup_commit_lock->ticket &&
10262-
!backup_lock_released)
10254+
if (thd->backup_commit_lock)
1026310255
{
1026410256
backup_lock_released= 1;
10265-
thd->mdl_context.release_lock(thd->backup_commit_lock->ticket);
10266-
thd->backup_commit_lock->ticket= 0;
10257+
thd->mdl_context.release_lock(thd->backup_commit_lock);
10258+
thd->backup_commit_lock= 0;
1026710259
}
1026810260

1026910261
/*
@@ -10527,8 +10519,9 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry)
1052710519

1052810520
end:
1052910521
if (backup_lock_released)
10530-
thd->mdl_context.acquire_lock(thd->backup_commit_lock,
10531-
thd->variables.lock_wait_timeout);
10522+
thd->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK(
10523+
MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT,
10524+
MDL_EXPLICIT, thd->variables.lock_wait_timeout);
1053210525
DBUG_RETURN(result);
1053310526
}
1053410527

‎sql/mdl.h‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -574,6 +574,10 @@ typedef void (*mdl_cached_object_release_hook)(void *);
574574
#define MDL_TRY_ACQUIRE_LOCK(NAMESPACE, DB, NAME, TYPE, DURATION, ERROR) \
575575
try_acquire_lock(NAMESPACE, DB, NAME, TYPE, DURATION, ERROR, __FILE__, __LINE__)
576576

577+
#define MDL_REQUEST_LIST_ADD(LIST, MEM_ROOT, NAMESPACE, DB, NAME, TYPE, DURATION) \
578+
mdl_request_list_add(LIST, MEM_ROOT, NAMESPACE, DB, NAME, TYPE, DURATION, \
579+
__FILE__, __LINE__)
580+
577581

578582
/**
579583
An abstract class for inspection of a connected
@@ -794,6 +798,22 @@ typedef I_P_List<MDL_request, I_P_List_adapter<MDL_request,
794798
I_P_List_counter>
795799
MDL_request_list;
796800

801+
inline bool mdl_request_list_add(MDL_request_list *list, MEM_ROOT *mem_root,
802+
MDL_key::enum_mdl_namespace mdl_namespace,
803+
const char *db, const char *name,
804+
enum_mdl_type mdl_type,
805+
enum_mdl_duration mdl_duration,
806+
const char *src_file, uint src_line)
807+
{
808+
MDL_request *r= new (mem_root) MDL_request;
809+
if (!r)
810+
return true;
811+
r->init_with_source(mdl_namespace, db, name, mdl_type, mdl_duration,
812+
src_file, src_line);
813+
list->push_front(r);
814+
return false;
815+
}
816+
797817
/**
798818
Context of the owner of metadata locks. I.e. each server
799819
connection has such a context.

‎sql/partition_info.cc‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -926,12 +926,11 @@ bool vers_create_partitions(THD *thd, TABLE_LIST* tl, uint num_parts)
926926
create_info.alter_info= &alter_info;
927927
Alter_table_ctx alter_ctx(thd, tl, 1, &table->s->db, &table->s->table_name);
928928

929-
MDL_REQUEST_INIT(&tl->mdl_request, MDL_key::TABLE, tl->db.str,
930-
tl->table_name.str, MDL_SHARED_NO_WRITE, MDL_TRANSACTION);
931-
if (thd->mdl_context.acquire_lock(&tl->mdl_request,
932-
thd->variables.lock_wait_timeout))
929+
if (!(table->mdl_ticket= thd->mdl_context.MDL_ACQUIRE_LOCK(
930+
MDL_key::TABLE, tl->db.str, tl->table_name.str,
931+
MDL_SHARED_NO_WRITE, MDL_TRANSACTION,
932+
thd->variables.lock_wait_timeout)))
933933
goto exit;
934-
table->mdl_ticket= tl->mdl_request.ticket;
935934

936935
create_info.db_type= table->s->db_type();
937936
create_info.options|= HA_VERSIONED_TABLE;

‎sql/sp.cc‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1876,14 +1876,17 @@ bool lock_db_routines(THD *thd, const Lex_ident_db_normalized &db)
18761876
continue;
18771877

18781878
longlong sp_type= table->field[MYSQL_PROC_MYSQL_TYPE]->val_int();
1879-
MDL_request *mdl_request= new (thd->mem_root) MDL_request;
18801879
const Sp_handler *sph= Sp_handler::handler((enum_sp_type)
18811880
sp_type);
18821881
if (!sph)
18831882
sph= &sp_handler_procedure;
1884-
MDL_REQUEST_INIT(mdl_request, sph->get_mdl_type(), db.str, sp_name,
1885-
MDL_EXCLUSIVE, MDL_TRANSACTION);
1886-
mdl_requests.push_front(mdl_request);
1883+
if (MDL_REQUEST_LIST_ADD(&mdl_requests, thd->mem_root,
1884+
sph->get_mdl_type(), db.str, sp_name,
1885+
MDL_EXCLUSIVE, MDL_TRANSACTION))
1886+
{
1887+
table->file->ha_index_end();
1888+
goto error;
1889+
}
18871890
} while (! (nxtres= table->file->ha_index_next_same(table->record[0], keybuf, key_len)));
18881891
}
18891892
table->file->ha_index_end();

‎sql/sql_base.cc‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4490,12 +4490,10 @@ lock_table_names(THD *thd, const DDL_options_st &options,
44904490
/* Scoped locks: Take intention exclusive locks on all involved schemas. */
44914491
if (!(flags & MYSQL_OPEN_SKIP_SCOPED_MDL_LOCK))
44924492
{
4493-
MDL_request *schema_request= new (thd->mem_root) MDL_request;
4494-
if (schema_request == NULL)
4493+
if (MDL_REQUEST_LIST_ADD(&mdl_requests, thd->mem_root,
4494+
MDL_key::SCHEMA, table->db.str, "",
4495+
MDL_INTENTION_EXCLUSIVE, MDL_TRANSACTION))
44954496
DBUG_RETURN(TRUE);
4496-
MDL_REQUEST_INIT(schema_request, MDL_key::SCHEMA, table->db.str, "",
4497-
MDL_INTENTION_EXCLUSIVE, MDL_TRANSACTION);
4498-
mdl_requests.push_front(schema_request);
44994497
}
45004498

45014499
mdl_requests.push_front(&table->mdl_request);

‎sql/sql_class.cc‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8565,11 +8565,11 @@ wait_for_commit::wait_for_prior_commit2(THD *thd, bool allow_kill)
85658565
yet have the MDL_BACKUP_COMMIT_LOCK) and any threads using
85668566
BACKUP LOCK BLOCK_COMMIT.
85678567
*/
8568-
if (thd->backup_commit_lock && thd->backup_commit_lock->ticket)
8568+
if (thd->backup_commit_lock)
85698569
{
85708570
backup_lock_released= true;
8571-
thd->mdl_context.release_lock(thd->backup_commit_lock->ticket);
8572-
thd->backup_commit_lock->ticket= 0;
8571+
thd->mdl_context.release_lock(thd->backup_commit_lock);
8572+
thd->backup_commit_lock= 0;
85738573
}
85748574

85758575
mysql_mutex_lock(&LOCK_wait_commit);
@@ -8625,15 +8625,17 @@ wait_for_commit::wait_for_prior_commit2(THD *thd, bool allow_kill)
86258625
*/
86268626
DEBUG_SYNC(thd, "wait_for_prior_commit_killed");
86278627
if (unlikely(backup_lock_released))
8628-
thd->mdl_context.acquire_lock(thd->backup_commit_lock,
8629-
thd->variables.lock_wait_timeout);
8628+
thd->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK(
8629+
MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT,
8630+
MDL_EXPLICIT, thd->variables.lock_wait_timeout);
86308631
return wakeup_error;
86318632

86328633
end:
86338634
thd->EXIT_COND(&old_stage);
86348635
if (unlikely(backup_lock_released))
8635-
thd->mdl_context.acquire_lock(thd->backup_commit_lock,
8636-
thd->variables.lock_wait_timeout);
8636+
thd->backup_commit_lock= thd->mdl_context.MDL_ACQUIRE_LOCK(
8637+
MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT,
8638+
MDL_EXPLICIT, thd->variables.lock_wait_timeout);
86378639
return wakeup_error;
86388640
}
86398641

‎sql/sql_class.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3369,7 +3369,7 @@ class THD: public THD_count, /* this must be first */
33693369
/* Used for BACKUP LOCK */
33703370
MDL_ticket *mdl_backup_ticket, *mdl_backup_lock;
33713371
/* Used to register that thread has a MDL_BACKUP_WAIT_COMMIT lock */
3372-
MDL_request *backup_commit_lock;
3372+
MDL_ticket *backup_commit_lock;
33733373

33743374
void reset_for_next_command(bool do_clear_errors= 1);
33753375

‎sql/sql_reload.cc‎

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -173,18 +173,14 @@ bool reload_acl_and_cache(THD *thd, unsigned long long options,
173173
tmp_write_to_binlog= 0;
174174
if (mysql_bin_log.is_open())
175175
{
176-
MDL_request mdl_request;
177-
MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_START,
178-
MDL_EXPLICIT);
176+
MDL_ticket *ticket= NULL;
179177
if (thd &&
180-
thd->mdl_context.acquire_lock(&mdl_request,
181-
thd->variables.lock_wait_timeout))
178+
!(ticket= thd->mdl_context.MDL_ACQUIRE_LOCK(
179+
MDL_key::BACKUP, "", "", MDL_BACKUP_START,
180+
MDL_EXPLICIT, thd->variables.lock_wait_timeout)))
182181
result= 1;
183182
else
184183
{
185-
if (thd)
186-
thd->backup_commit_lock= &mdl_request;
187-
188184
DYNAMIC_ARRAY *drop_gtid_domain=
189185
(thd && (thd->lex->delete_gtid_domain.elements > 0)) ?
190186
&thd->lex->delete_gtid_domain : NULL;
@@ -198,12 +194,8 @@ bool reload_acl_and_cache(THD *thd, unsigned long long options,
198194
/* Wait for last binlog checkpoint event to be logged. */
199195
mysql_bin_log.wait_for_last_checkpoint_event();
200196
}
201-
if (thd)
202-
{
203-
if (mdl_request.ticket)
204-
thd->mdl_context.release_lock(mdl_request.ticket);
205-
thd->backup_commit_lock= 0;
206-
}
197+
if (ticket)
198+
thd->mdl_context.release_lock(ticket);
207199
}
208200
}
209201
}

‎sql/sql_repl.cc‎

Lines changed: 10 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -800,13 +800,11 @@ bool purge_master_logs(THD* thd, const char* to_log)
800800
return FALSE;
801801
}
802802

803-
MDL_request mdl_request;
804-
MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_START,
805-
MDL_EXPLICIT);
806-
if (thd->mdl_context.acquire_lock(&mdl_request,
807-
thd->variables.lock_wait_timeout))
803+
MDL_ticket *ticket;
804+
if (!(ticket= thd->mdl_context.MDL_ACQUIRE_LOCK(
805+
MDL_key::BACKUP, "", "", MDL_BACKUP_START,
806+
MDL_EXPLICIT, thd->variables.lock_wait_timeout)))
808807
return TRUE;
809-
thd->backup_commit_lock= &mdl_request;
810808

811809
int res;
812810
if (!opt_binlog_engine_hton)
@@ -849,9 +847,7 @@ bool purge_master_logs(THD* thd, const char* to_log)
849847
purge_info.nonpurge_filename, true);
850848
}
851849

852-
if (mdl_request.ticket)
853-
thd->mdl_context.release_lock(mdl_request.ticket);
854-
thd->backup_commit_lock= 0;
850+
thd->mdl_context.release_lock(ticket);
855851

856852
return purge_error_message(thd, res);
857853
}
@@ -4904,23 +4900,19 @@ int reset_master(THD* thd, rpl_gtid *init_state, uint32 init_state_len,
49044900
#endif /* WITH_WSREP */
49054901
bool ret= 0;
49064902

4907-
MDL_request mdl_request;
4908-
MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_START,
4909-
MDL_EXPLICIT);
4910-
if (thd->mdl_context.acquire_lock(&mdl_request,
4911-
thd->variables.lock_wait_timeout))
4903+
MDL_ticket *ticket;
4904+
if (!(ticket= thd->mdl_context.MDL_ACQUIRE_LOCK(
4905+
MDL_key::BACKUP, "", "", MDL_BACKUP_START,
4906+
MDL_EXPLICIT, thd->variables.lock_wait_timeout)))
49124907
return 1;
4913-
thd->backup_commit_lock= &mdl_request;
49144908

49154909
/* Temporarily disable master semisync before resetting master. */
49164910
repl_semisync_master.before_reset_master();
49174911
ret= mysql_bin_log.reset_logs(thd, 1, init_state, init_state_len,
49184912
next_log_number);
49194913
repl_semisync_master.after_reset_master();
49204914

4921-
if (mdl_request.ticket)
4922-
thd->mdl_context.release_lock(mdl_request.ticket);
4923-
thd->backup_commit_lock= 0;
4915+
thd->mdl_context.release_lock(ticket);
49244916

49254917
DBUG_EXECUTE_IF("crash_after_reset_master", DBUG_SUICIDE(););
49264918

0 commit comments

Comments
 (0)