-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(8.0) PXC-4498: PXC node "stalls" and causes the cluster to stop responding #2021
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang-Tidy
found issue(s) with the introduced code (1/1)
@@ -387,19 +389,142 @@ page_no_t trx_sysf_rseg_find_page_no(ulint rseg_id) { | |||
|
|||
#ifdef WITH_WSREP | |||
|
|||
#define WSREP_UUID_SIZE 16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
macro WSREP_UUID_SIZE
used to declare a constant; consider using a constexpr
constant
unsigned char xid_uuid[WSREP_UUID_SIZE]; | ||
long long xid_seqno = read_wsrep_xid_seqno(xid); | ||
read_wsrep_xid_uuid(xid, xid_uuid); | ||
if (!memcmp(xid_uuid, trx_sys_cur_xid_uuid, WSREP_UUID_SIZE) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implicit conversion int
-> bool
if (!memcmp(xid_uuid, trx_sys_cur_xid_uuid, WSREP_UUID_SIZE) && | |
if ((memcmp(xid_uuid, trx_sys_cur_xid_uuid, WSREP_UUID_SIZE) == 0) && |
// ut_ad((current_thd && wsrep_thd_is_in_nbo(current_thd)) || | ||
// xid_seqno >= trx_sys_cur_xid_seqno); | ||
} else { | ||
memcpy(trx_sys_cur_xid_uuid, xid_uuid, 16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
16 is a magic number; consider replacing it with a named constant
storage/innobase/trx/trx0sys.cc
Outdated
} | ||
trx_sys_cur_xid_seqno = xid_seqno; | ||
|
||
char uuid_str[40] = { 0, }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
40 is a magic number; consider replacing it with a named constant
storage/innobase/trx/trx0sys.cc
Outdated
trx_sys_cur_xid_seqno = xid_seqno; | ||
|
||
char uuid_str[40] = { 0, }; | ||
wsrep_uuid_print((const wsrep_uuid_t *)xid_uuid, uuid_str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not use C-style cast to convert between unrelated types
storage/innobase/trx/trx0sys.cc
Outdated
trx_sys_cur_xid_seqno = xid_seqno; | ||
|
||
char uuid_str[40] = { 0, }; | ||
wsrep_uuid_print((const wsrep_uuid_t *)xid_uuid, uuid_str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C-style casts are discouraged; use reinterpret_cast
wsrep_uuid_print((const wsrep_uuid_t *)xid_uuid, uuid_str, | |
wsrep_uuid_print(reinterpret_cast<const wsrep_uuid_t *>(xid_uuid), uuid_str, |
} else { | ||
#ifdef UNIV_DEBUG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not use else
after return
} else { | |
#ifdef UNIV_DEBUG | |
} #ifdef UNIV_DEBUG |
} else { | ||
#ifdef UNIV_DEBUG | ||
wsrep_xid_sanity_check(xid); | ||
#endif /* UNIV_DEBUG */ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not use else
after return
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please make sure Jenkins is clean and don't forget to squash the commits.
6640832
to
e542383
Compare
https://pxc.cd.percona.com/view/8.0%20parallel%20MTR/job/pxc-8.0-pipeline-parallel-mtr/3058/ All failures are instabilities or timeouts (long tests) |
https://perconadev.atlassian.net/browse/PXC-4498 https://perconadev.atlassian.net/browse/PXC-4390 Removed wsrep_group_commit_queue. The queue works only for variables used for debug assertions. The actual write to sys_header is done without the protection of the queue. As the queue is not enforcing proper order of xid storing in sys_header, but was in the past and still is the source of deadlocks - removing.
e542383
to
c65c69d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang-Tidy
found issue(s) with the introduced code (1/1)
} | ||
trx_sys_cur_xid_seqno = xid_seqno; | ||
|
||
char uuid_str[40] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
40 is a magic number; consider replacing it with a named constant
char uuid_str[40] = { | ||
0, | ||
}; | ||
wsrep_uuid_print((const wsrep_uuid_t *)xid_uuid, uuid_str, sizeof(uuid_str)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not use C-style cast to convert between unrelated types
char uuid_str[40] = { | ||
0, | ||
}; | ||
wsrep_uuid_print((const wsrep_uuid_t *)xid_uuid, uuid_str, sizeof(uuid_str)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C-style casts are discouraged; use reinterpret_cast
wsrep_uuid_print((const wsrep_uuid_t *)xid_uuid, uuid_str, sizeof(uuid_str)); | |
wsrep_uuid_print(reinterpret_cast<const wsrep_uuid_t *>(xid_uuid), uuid_str, sizeof(uuid_str)); |
https://perconadev.atlassian.net/browse/PXC-4498
https://perconadev.atlassian.net/browse/PXC-4390
Removed wsrep_group_commit_queue.