Skip to content

Commit 50243ef

Browse files
rahim-kanjirahim-kanji
committed
Corrected RESET/DEFAULT logic
* Now will actually reset value to server default (represented by nullptr value), if that parameter is not set in startup parameter. * Default parameter values (pgsql_default_*) will now be set only for critical parameters.
1 parent 1bcd090 commit 50243ef

File tree

5 files changed

+56
-62
lines changed

5 files changed

+56
-62
lines changed

‎include/PgSQL_Thread.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -957,7 +957,7 @@ class PgSQL_Threads_Handler
957957
char* ldap_user_variable;
958958
char* add_ldap_user_comment;
959959
char* default_session_track_gtids;
960-
char* default_variables[PGSQL_NAME_LAST_HIGH_WM];
960+
char* default_variables[PGSQL_NAME_LAST_LOW_WM];
961961
char* firewall_whitelist_errormsg;
962962
#ifdef DEBUG
963963
bool session_debug;

‎include/proxysql_structs.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1192,7 +1192,7 @@ __thread int pgsql_thread___query_cache_soft_ttl_pct;
11921192
__thread int pgsql_thread___query_cache_handle_warnings;
11931193

11941194
__thread bool pgsql_thread___session_idle_show_processlist;
1195-
__thread char* pgsql_thread___default_variables[PGSQL_NAME_LAST_HIGH_WM];
1195+
__thread char* pgsql_thread___default_variables[PGSQL_NAME_LAST_LOW_WM];
11961196
__thread int pgsql_thread___handle_unknown_charset;
11971197
//---------------------------
11981198

@@ -1497,7 +1497,7 @@ extern __thread int pgsql_thread___query_cache_soft_ttl_pct;
14971497
extern __thread int pgsql_thread___query_cache_handle_warnings;
14981498

14991499
extern __thread bool pgsql_thread___session_idle_show_processlist;
1500-
extern __thread char* pgsql_thread___default_variables[PGSQL_NAME_LAST_HIGH_WM];
1500+
extern __thread char* pgsql_thread___default_variables[PGSQL_NAME_LAST_LOW_WM];
15011501
extern __thread int pgsql_thread___handle_unknown_charset;
15021502
//---------------------------
15031503

‎lib/PgSQL_Connection.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2208,8 +2208,8 @@ std::pair<const char*, uint32_t> PgSQL_Connection::get_startup_parameter_and_has
22082208
assert(startup_parameters[idx]);
22092209
return { startup_parameters[idx], startup_parameters_hash[idx] };
22102210
}
2211-
// fall back to thread-specific default
2212-
return { pgsql_thread___default_variables[idx], 0 };
2211+
assert(!(idx < PGSQL_NAME_LAST_LOW_WM));
2212+
return { "", 0};
22132213
}
22142214

22152215
void PgSQL_Connection::copy_pgsql_variables_to_startup_parameters(bool copy_only_critical_param) {

‎lib/PgSQL_Session.cpp‎

Lines changed: 40 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -4210,15 +4210,32 @@ bool PgSQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
42104210

42114211
int idx = PGSQL_NAME_LAST_HIGH_WM;
42124212
for (int i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
4213+
// skip low water mark
4214+
if (i == PGSQL_NAME_LAST_LOW_WM) continue;
4215+
42134216
if (variable_name_exists(pgsql_tracked_variables[i], var.c_str()) == true) {
42144217
idx = i;
42154218
break;
42164219
}
42174220
}
42184221
if (idx != PGSQL_NAME_LAST_HIGH_WM) {
4219-
4222+
uint32_t current_hash = pgsql_variables.client_get_hash(this, idx);
42204223
if ((value1.size() == sizeof("DEFAULT") - 1) && strncasecmp(value1.c_str(), (char*)"DEFAULT",sizeof("DEFAULT")-1) == 0) {
4221-
std::tie(value1, std::ignore) = client_myds->myconn->get_startup_parameter_and_hash((enum pgsql_variable_name)idx);
4224+
auto [value, hash] = client_myds->myconn->get_startup_parameter_and_hash((enum pgsql_variable_name)idx);
4225+
if (hash == 0) {
4226+
if (current_hash != 0) {
4227+
proxy_debug(PROXY_DEBUG_MYSQL_COM, 8, "Resetting connection variable %s to DEFAULT\n", var.c_str());
4228+
pgsql_variables.client_reset_value(this, idx, true);
4229+
}
4230+
client_myds->DSS = STATE_QUERY_SENT_NET;
4231+
unsigned int nTrx = NumActiveTransactions();
4232+
const char trx_state = (nTrx ? 'T' : 'I');
4233+
client_myds->myprot.generate_ok_packet(true, true, NULL, 0, dig, trx_state, NULL, param_status);
4234+
RequestEnd(NULL);
4235+
l_free(pkt->size, pkt->ptr);
4236+
return true;
4237+
}
4238+
value1 = value;
42224239
}
42234240

42244241
char* transformed_value = nullptr;
@@ -4259,10 +4276,10 @@ bool PgSQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
42594276
return true;
42604277
}
42614278
}
4262-
4263-
proxy_debug(PROXY_DEBUG_MYSQL_COM, 8, "Changing connection %s to %s\n", var.c_str(), value1.c_str());
4279+
42644280
uint32_t var_hash_int = SpookyHash::Hash32(value1.c_str(), value1.length(), 10);
4265-
if (pgsql_variables.client_get_hash(this, idx) != var_hash_int) {
4281+
if (current_hash != var_hash_int) {
4282+
proxy_debug(PROXY_DEBUG_MYSQL_COM, 8, "Changing connection %s to %s\n", var.c_str(), value1.c_str());
42664283
if (!pgsql_variables.client_set_value(this, idx, value1.c_str(), true)) {
42674284
return false;
42684285
}
@@ -4445,8 +4462,7 @@ bool PgSQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
44454462
std::string value1 = *values;
44464463
proxy_debug(PROXY_DEBUG_MYSQL_COM, 5, "Processing SET %s value %s\n", var.c_str(), value1.c_str());
44474464
#endif // DEBUG
4448-
}
4449-
else {
4465+
} else {
44504466
// At this point the variable is unknown to us, or it's a user variable
44514467
// prefixed by '@', in both cases, we should fail to parse. We don't
44524468
// fail inmediately so we can anyway keep track of the other variables
@@ -4638,10 +4654,10 @@ bool PgSQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
46384654

46394655
const char* name = pgsql_tracked_variables[idx].set_variable_name;
46404656
auto [value, hash] = client_myds->myconn->get_startup_parameter_and_hash((enum pgsql_variable_name)idx);
4641-
4642-
proxy_debug(PROXY_DEBUG_MYSQL_COM, 8, "Changing connection %s to %s\n", name, value);
4643-
uint32_t var_hash_int = ((hash != 0) ? hash : SpookyHash::Hash32(value, strlen(value), 10));
4644-
if (pgsql_variables.client_get_hash(this, idx) != var_hash_int) {
4657+
// hash can never be 0 for critical variables
4658+
uint32_t current_hash = pgsql_variables.client_get_hash(this, idx);
4659+
if (current_hash != hash) {
4660+
proxy_debug(PROXY_DEBUG_MYSQL_COM, 8, "Changing connection %s to %s\n", name, value);
46454661
if (!pgsql_variables.client_set_value(this, idx, value, false)) {
46464662
return false;
46474663
}
@@ -4652,21 +4668,19 @@ bool PgSQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
46524668
}
46534669

46544670
for (int idx : client_myds->myconn->dynamic_variables_idx) {
4655-
assert(idx < PGSQL_NAME_LAST_HIGH_WM);
46564671
const char* name = pgsql_tracked_variables[idx].set_variable_name;
46574672
auto [value, hash] = client_myds->myconn->get_startup_parameter_and_hash((enum pgsql_variable_name)idx);
4658-
proxy_debug(PROXY_DEBUG_MYSQL_COM, 8, "Changing connection %s to %s\n", name, value);
4659-
uint32_t var_hash_int = ((hash != 0) ? hash : SpookyHash::Hash32(value, strlen(value), 10));
4660-
if (pgsql_variables.client_get_hash(this, idx) != var_hash_int) {
4673+
uint32_t current_hash = pgsql_variables.client_get_hash(this, idx);
4674+
if (hash == 0 && current_hash != 0) {
4675+
proxy_debug(PROXY_DEBUG_MYSQL_COM, 8, "Resetting connection variable %s to DEFAULT\n", name);
4676+
pgsql_variables.client_reset_value(this, idx, false);
4677+
} else if (hash != 0 && current_hash != hash) {
4678+
proxy_debug(PROXY_DEBUG_MYSQL_COM, 8, "Changing connection %s to %s\n", name, value);
46614679
if (!pgsql_variables.client_set_value(this, idx, value, false)) {
46624680
return false;
46634681
}
4664-
if (IS_PGTRACKED_VAR_OPTION_SET_PARAM_STATUS(pgsql_tracked_variables[idx])) {
4665-
param_status.emplace_back(name, value);
4666-
}
46674682
}
46684683
}
4669-
46704684
client_myds->myconn->reorder_dynamic_variables_idx();
46714685

46724686
} else if (std::find(pgsql_variables.ignore_vars.begin(), pgsql_variables.ignore_vars.end(), nq) != pgsql_variables.ignore_vars.end()) {
@@ -4675,10 +4689,8 @@ bool PgSQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
46754689
proxy_debug(PROXY_DEBUG_MYSQL_COM, 5, "Processing RESET %s\n", nq.c_str());
46764690
#endif // DEBUG
46774691
} else {
4678-
46794692
int idx = PGSQL_NAME_LAST_HIGH_WM;
46804693
for (int i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
4681-
46824694
if (i == PGSQL_NAME_LAST_LOW_WM)
46834695
continue;
46844696

@@ -4687,18 +4699,20 @@ bool PgSQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
46874699
break;
46884700
}
46894701
}
4690-
46914702
if (idx != PGSQL_NAME_LAST_HIGH_WM) {
46924703
const char* name = pgsql_tracked_variables[idx].set_variable_name;
46934704
auto [value, hash] = client_myds->myconn->get_startup_parameter_and_hash((enum pgsql_variable_name)idx);
4694-
4695-
uint32_t var_hash_int = ((hash != 0) ? hash : SpookyHash::Hash32(value, strlen(value), 10));
4696-
if (pgsql_variables.client_get_hash(this, idx) != var_hash_int) {
4705+
uint32_t current_hash = pgsql_variables.client_get_hash(this, idx);
4706+
// Reset to default if hash is zero, means startup parameter is not set
4707+
if (hash == 0 && current_hash != 0) {
4708+
proxy_debug(PROXY_DEBUG_MYSQL_COM, 8, "Resetting connection variable %s to DEFAULT\n", name);
4709+
pgsql_variables.client_reset_value(this, idx, true);
4710+
} else if (hash != 0 && current_hash != hash) {
4711+
proxy_debug(PROXY_DEBUG_MYSQL_COM, 8, "Changing connection %s to %s\n", name, value);
46974712
if (!pgsql_variables.client_set_value(this, idx, value, true)) {
46984713
return false;
46994714
}
47004715
if (IS_PGTRACKED_VAR_OPTION_SET_PARAM_STATUS(pgsql_tracked_variables[idx])) {
4701-
proxy_debug(PROXY_DEBUG_MYSQL_COM, 8, "Changing connection %s to %s\n", name, value);
47024716
param_status.emplace_back(name, value);
47034717
}
47044718
}

‎lib/PgSQL_Thread.cpp‎

Lines changed: 11 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1020,7 +1020,7 @@ PgSQL_Threads_Handler::PgSQL_Threads_Handler() {
10201020
variables.init_connect = NULL;
10211021
variables.ldap_user_variable = NULL;
10221022
variables.add_ldap_user_comment = NULL;
1023-
for (int i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
1023+
for (int i = 0; i < PGSQL_NAME_LAST_LOW_WM; i++) {
10241024
variables.default_variables[i] = strdup(pgsql_tracked_variables[i].default_value);
10251025
}
10261026
variables.default_session_track_gtids = strdup((char*)MYSQL_DEFAULT_SESSION_TRACK_GTIDS);
@@ -1289,9 +1289,7 @@ char* PgSQL_Threads_Handler::get_variable_string(char* name) {
12891289
}
12901290
}
12911291
if (!strncmp(name, "default_", 8)) {
1292-
for (int i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
1293-
if (i == PGSQL_NAME_LAST_LOW_WM)
1294-
continue;
1292+
for (int i = 0; i < PGSQL_NAME_LAST_LOW_WM; i++) {
12951293
char buf[128];
12961294
sprintf(buf, "default_%s", pgsql_tracked_variables[i].internal_variable_name);
12971295
if (!strcmp(name, buf)) {
@@ -1422,9 +1420,7 @@ char* PgSQL_Threads_Handler::get_variable(char* name) { // this is the public fu
14221420
}
14231421
if (strlen(name) > 8) {
14241422
if (strncmp(name, "default_", 8) == 0) {
1425-
for (unsigned int i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
1426-
if (i == PGSQL_NAME_LAST_LOW_WM)
1427-
continue;
1423+
for (int i = 0; i < PGSQL_NAME_LAST_LOW_WM; i++) {
14281424
size_t var_len = strlen(pgsql_tracked_variables[i].internal_variable_name);
14291425
if (strlen(name) == (var_len + 8)) {
14301426
if (!strncmp(name + 8, pgsql_tracked_variables[i].internal_variable_name, var_len)) {
@@ -1802,11 +1798,7 @@ bool PgSQL_Threads_Handler::set_variable(char* name, const char* value) { // thi
18021798
}
18031799

18041800
if (!strncmp(name, "default_", 8)) {
1805-
for (int i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
1806-
1807-
if (i == PGSQL_NAME_LAST_LOW_WM)
1808-
continue;
1809-
1801+
for (int i = 0; i < PGSQL_NAME_LAST_LOW_WM; i++) {
18101802
char buf[128];
18111803
sprintf(buf, "default_%s", pgsql_tracked_variables[i].internal_variable_name);
18121804
if (!strcmp(name, buf)) {
@@ -1842,7 +1834,6 @@ bool PgSQL_Threads_Handler::set_variable(char* name, const char* value) { // thi
18421834
}
18431835
}
18441836

1845-
18461837
if (!strcasecmp(name, "keep_multiplexing_variables")) {
18471838
if (vallen) {
18481839
free(variables.keep_multiplexing_variables);
@@ -2249,17 +2240,10 @@ char** PgSQL_Threads_Handler::get_variables_list() {
22492240

22502241
const size_t l = sizeof(pgsql_thread_variables_names) / sizeof(char*);
22512242
unsigned int i;
2252-
size_t ltv = 0;
2253-
for (i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
2254-
if (i == PGSQL_NAME_LAST_LOW_WM)
2255-
continue;
2256-
ltv++;
2257-
}
2243+
const size_t ltv = PGSQL_NAME_LAST_LOW_WM;
22582244
char** ret = (char**)malloc(sizeof(char*) * (l + ltv)); // not adding + 1 because pgsql_thread_variables_names is already NULL terminated
22592245
size_t fv = 0;
2260-
for (i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
2261-
if (i == PGSQL_NAME_LAST_LOW_WM)
2262-
continue;
2246+
for (i = 0; i < PGSQL_NAME_LAST_LOW_WM; i++) {
22632247
char* m = (char*)malloc(strlen(pgsql_tracked_variables[i].internal_variable_name) + 1 + strlen((char*)"default_"));
22642248
sprintf(m, "default_%s", pgsql_tracked_variables[i].internal_variable_name);
22652249
ret[fv] = m;
@@ -2279,9 +2263,7 @@ char** PgSQL_Threads_Handler::get_variables_list() {
22792263
bool PgSQL_Threads_Handler::has_variable(const char* name) {
22802264
if (strlen(name) > 8) {
22812265
if (strncmp(name, "default_", 8) == 0) {
2282-
for (unsigned int i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
2283-
if (i == PGSQL_NAME_LAST_LOW_WM)
2284-
continue;
2266+
for (unsigned int i = 0; i < PGSQL_NAME_LAST_LOW_WM; i++) {
22852267
size_t var_len = strlen(pgsql_tracked_variables[i].internal_variable_name);
22862268
if (strlen(name) == (var_len + 8)) {
22872269
if (!strncmp(name + 8, pgsql_tracked_variables[i].internal_variable_name, var_len)) {
@@ -2672,7 +2654,7 @@ PgSQL_Threads_Handler::~PgSQL_Threads_Handler() {
26722654
if (variables.ssl_p2s_crl) free(variables.ssl_p2s_crl);
26732655
if (variables.ssl_p2s_crlpath) free(variables.ssl_p2s_crlpath);
26742656

2675-
for (int i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
2657+
for (int i = 0; i < PGSQL_NAME_LAST_LOW_WM; i++) {
26762658
if (variables.default_variables[i]) {
26772659
free(variables.default_variables[i]);
26782660
variables.default_variables[i] = NULL;
@@ -2801,7 +2783,7 @@ PgSQL_Thread::~PgSQL_Thread() {
28012783
if (pgsql_thread___server_version) { free(pgsql_thread___server_version); pgsql_thread___server_version = NULL; }
28022784
if (pgsql_thread___server_encoding) { free(pgsql_thread___server_encoding); pgsql_thread___server_encoding = NULL; }
28032785

2804-
for (int i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
2786+
for (int i = 0; i < PGSQL_NAME_LAST_LOW_WM; i++) {
28052787
if (pgsql_thread___default_variables[i]) {
28062788
free(pgsql_thread___default_variables[i]);
28072789
pgsql_thread___default_variables[i] = NULL;
@@ -3926,9 +3908,7 @@ void PgSQL_Thread::refresh_variables() {
39263908
mysql_thread___default_session_track_gtids = GloPTH->get_variable_string((char*)"default_session_track_gtids");
39273909
*/
39283910

3929-
for (int i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
3930-
if (i == PGSQL_NAME_LAST_LOW_WM)
3931-
continue;
3911+
for (int i = 0; i < PGSQL_NAME_LAST_LOW_WM; i++) {
39323912
if (pgsql_thread___default_variables[i]) {
39333913
free(pgsql_thread___default_variables[i]);
39343914
pgsql_thread___default_variables[i] = NULL;
@@ -4091,7 +4071,7 @@ PgSQL_Thread::PgSQL_Thread() {
40914071
variables.stats_time_query_processor = false;
40924072
variables.query_cache_stores_empty_result = true;
40934073

4094-
for (int i = 0; i < PGSQL_NAME_LAST_HIGH_WM; i++) {
4074+
for (int i = 0; i < PGSQL_NAME_LAST_LOW_WM; i++) {
40954075
pgsql_thread___default_variables[i] = NULL;
40964076
}
40974077
shutdown = 0;

0 commit comments

Comments
 (0)