Unlock mtxs before calling osql_bplog_free to avoid crash #2013
Conversation
There was a problem hiding this comment.
Coding style check: Error. Use clang-format or view diff ⚠.
Smoke testing: Error ⚠ .
Failed to compile.
There was a problem hiding this comment.
Coding style check: Error. Use clang-format or view diff ⚠.
Smoke testing: Error ⚠ .
Cbuild submission: Success ✓.
Regression testing: 3/326 tests failed ⚠.
The first 10 failing tests are:
sc_timepart_logicalsc_generated
restart_socksql
enospc
There was a problem hiding this comment.
Sysbench Transactions Per Second (commit: 918534c):
@@ sysbench/bulk_insert @@
-8.16%
@@ sysbench/select_random_ranges @@
-5.77%
@@ sysbench/oltp_read_write @@
-4.74%
There was a problem hiding this comment.
Coding style check: Error. Use clang-format or view diff ⚠.
Smoke testing: Success ✓.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
There was a problem hiding this comment.
Sysbench Transactions Per Second (commit: 918534c):
@@ sysbench/select_random_points @@
-4.44%
There was a problem hiding this comment.
Coding style check: Error. Use clang-format or view diff ⚠.
Smoke testing: Success ✓.
Cbuild submission: Success ✓.
Regression testing: 1/326 tests failed ⚠.
The first 10 failing tests are:
enospc
There was a problem hiding this comment.
Sysbench Transactions Per Second (commit: 918534c):
@@ sysbench/select_random_points @@
-4.45%
@@ sysbench/bulk_insert @@
-7.76%
@@ sysbench/oltp_read_only @@
-5.61%
@@ sysbench/oltp_update_non_index @@
-5.16%
@@ sysbench/oltp_read_write @@
-5.38%
|
|
||
| msglen = OSQLCOMM_DONE_RPL_LEN; |
There was a problem hiding this comment.
This was a bug probably, should have been OSQLCOMM_DONE_UUID_RPL_LEN.
There was a problem hiding this comment.
It most certainly was. I tried to fix it as well (#1868).
| sorese->host); | ||
| } | ||
|
|
||
| } else { |
| __LINE__); /* already have exclusive lock */ | ||
| Pthread_mutex_unlock(&sess->completed_lock); | ||
| Pthread_mutex_unlock(&sess->mtx); | ||
|
|
There was a problem hiding this comment.
This is the real bug, these two should be unlocked before calling osql_repository_rem.
| @@ -5111,7 +5105,7 @@ int osql_comm_signal_sqlthr_rc(osql_sess_t *sorese, struct errstat *xerr, | |||
| max = OSQLCOMM_DONE_XERR_RPL_LEN; | |||
| if (OSQLCOMM_DONE_RPL_LEN > max) | |||
| max = OSQLCOMM_DONE_RPL_LEN; | |||
| buf = alloca(max); | |||
There was a problem hiding this comment.
I know not introduced in this PR and noted in prior code-comment about being kludgy - runtime decision for buf size. Perhaps not even a performance issue after optimization. However, while we are here can use a union to clean this up:
union {
char a[OSQLCOMM_DONE_XERR_UUID_RPL_LEN];
char b[OSQLCOMM_DONE_UUID_RPL_LEN];
char c[OSQLCOMM_DONE_XERR_RPL_LEN];
char d[OSQLCOMM_DONE_RPL_LEN];
} u;
uint8_t *buf = (uint8_t *) &u;
| struct ireq *iq = NULL; | ||
| uint8_t *malcd = malloc(OSQL_BP_MAXLEN); |
There was a problem hiding this comment.
32k - we can probably just allocate on stack This (uint8_t malcd[OSQL_BP_MAXLEN] instead of malloc, free.malcd) needs to survive if successful?
| Let THAT thread free it... | ||
| free(p_buf); | ||
| */ | ||
| Let THAT thread free p_buf (malcd)... */ |
There was a problem hiding this comment.
to answer the above comment, malcd needs to survive...see this line.
There was a problem hiding this comment.
thanks - we can try passing it in so don't have to get from heap
There was a problem hiding this comment.
also simplifies cleanup on error
There was a problem hiding this comment.
good idea, will try in separate PR.
There was a problem hiding this comment.
Coding style check: Error. Use clang-format or view diff ⚠.
Smoke testing: Success ✓.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
|
Thank you Akshat. |
adizaimi
changed the title
R7: unlock mutexes before calling destroy on session obj. Port of PR #2013.
R7: unlock mutexes before calling destroy on session obj. Port of PR bloomberg#2013.
Unlock mtx before calling osql_bplog_free() to avoid crash. Before this PR, we destroy the session with the mutexes held via callstack: osql_bplog_free() -> osql_close_session() -> _destroy_session().
fixes PR #2011