From 6f6d241bb12f854fce41b60c54e7745165724ca5 Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Wed, 9 Apr 2025 14:04:01 +0500 Subject: [PATCH 1/5] Removed gtid related code --- include/PgSQL_Backend.h | 2 -- lib/PgSQL_Backend.cpp | 2 -- 2 files changed, 4 deletions(-) diff --git a/include/PgSQL_Backend.h b/include/PgSQL_Backend.h index 15d75697f..a4cea94dc 100644 --- a/include/PgSQL_Backend.h +++ b/include/PgSQL_Backend.h @@ -12,8 +12,6 @@ class PgSQL_Backend void * operator new(size_t); void operator delete(void *); int hostgroup_id; //< The ID of the host group this connection belongs to. Set to -1 if uninitialized - char gtid_uuid[128]; //< An array to store a unique identifier for each transaction : for now unused - uint64_t gtid_trxid; //< The ID of the current transaction : for now unused PgSQL_Data_Stream *server_myds; // mysql_cp_entry_t *server_mycpe; bytes_stats_t server_bytes_at_cmd; //< A structure storing the number of bytes received and sent diff --git a/lib/PgSQL_Backend.cpp b/lib/PgSQL_Backend.cpp index 0b926d0c0..d4c7f3327 100644 --- a/lib/PgSQL_Backend.cpp +++ b/lib/PgSQL_Backend.cpp @@ -15,8 +15,6 @@ PgSQL_Backend::PgSQL_Backend() { server_myds=NULL; server_bytes_at_cmd.bytes_recv=0; server_bytes_at_cmd.bytes_sent=0; - memset(gtid_uuid,0,sizeof(gtid_uuid)); - gtid_trxid=0; } PgSQL_Backend::~PgSQL_Backend() { From 857e501ca3ca13fa6ecc729b0b7d1deed9be9d34 Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Thu, 24 Apr 2025 19:39:42 +0500 Subject: [PATCH 2/5] Added update_fd_at_index method --- include/ProxySQL_Poll.h | 1 + lib/ProxySQL_Poll.cpp | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/include/ProxySQL_Poll.h b/include/ProxySQL_Poll.h index 406c0e993..8dbcfc921 100644 --- a/include/ProxySQL_Poll.h +++ b/include/ProxySQL_Poll.h @@ -44,6 +44,7 @@ class ProxySQL_Poll { ProxySQL_Poll(); ~ProxySQL_Poll(); void add(uint32_t _events, int _fd, T *_myds, unsigned long long sent_time); + void update_fd_at_index(unsigned int idx, int _fd); void remove_index_fast(unsigned int i); int find_index(int fd); }; diff --git a/lib/ProxySQL_Poll.cpp b/lib/ProxySQL_Poll.cpp index 425bca51d..637a28879 100644 --- a/lib/ProxySQL_Poll.cpp +++ b/lib/ProxySQL_Poll.cpp @@ -126,6 +126,21 @@ void ProxySQL_Poll::add(uint32_t _events, int _fd, T *_myds, unsigned long lo len++; } +/** + * @brief Updates the file descriptor (FD) at a specific index in the ProxySQL_Poll object. + * + * This function updates the file descriptor (FD) at a specific index in the ProxySQL_Poll object. + * It does not modify any other associated data or metadata. + * + * @param idx The index of the file descriptor (FD) to update. + * @param _fd The new file descriptor (FD) value. + */ +template +void ProxySQL_Poll::update_fd_at_index(unsigned int idx, int _fd) { + if ((int)idx == -1 || idx >= len) return; + fds[idx].fd = _fd; +} + /** * @brief Removes a file descriptor (FD) and its associated MySQL_Data_Stream from the ProxySQL_Poll object. * From 4969bd8301af17bec21b718cd8364beaedfb8ccd Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Thu, 24 Apr 2025 19:42:23 +0500 Subject: [PATCH 3/5] PQconnectPoll can file descriptor (FD) during the connection process. We need to check whether FD has changed and, if so, update it in the thread's poll array. --- lib/PgSQL_Connection.cpp | 10 ++++++++-- lib/PgSQL_Session.cpp | 13 ++++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/lib/PgSQL_Connection.cpp b/lib/PgSQL_Connection.cpp index 68baf89da..cb1517f9e 100644 --- a/lib/PgSQL_Connection.cpp +++ b/lib/PgSQL_Connection.cpp @@ -884,7 +884,7 @@ void PgSQL_Connection::connect_cont(short event) { case PGRES_POLLING_WRITING: async_exit_status = PG_EVENT_WRITE; break; - case PGRES_POLLING_ACTIVE: + case PGRES_POLLING_ACTIVE: // Not used case PGRES_POLLING_READING: async_exit_status = PG_EVENT_READ; break; @@ -897,7 +897,13 @@ void PgSQL_Connection::connect_cont(short event) { const PGresult* result = PQgetResultFromPGconn(pgsql_conn); set_error_from_result(result); proxy_error("Connect failed. %s\n", get_error_code_with_message().c_str()); - return; + } + int current_fd = PQsocket(pgsql_conn); + if (current_fd != fd) { + proxy_warning("PgSQL Connection FD has been changed by PQconnectPoll(). oldFD:%d newFD:%d\n", fd, current_fd); + proxy_debug(PROXY_DEBUG_MYSQL_CONNECTION, 5, "PgSQL Connection FD has been changed by PQconnectPoll()" + "Session=%p, Conn=%p, myds=%p, oldFD=%d, newFD=%d\n", myds->sess, this, myds, fd, current_fd); + fd = current_fd; } } diff --git a/lib/PgSQL_Session.cpp b/lib/PgSQL_Session.cpp index c09dce42c..5426a49ce 100644 --- a/lib/PgSQL_Session.cpp +++ b/lib/PgSQL_Session.cpp @@ -1744,10 +1744,21 @@ bool PgSQL_Session::handler_again___status_CONNECTING_SERVER(int* _rc) { if (myds->mypolls == NULL) { // connection yet not in mypolls myds->assign_fd_from_mysql_conn(); - thread->mypolls.add(POLLIN | POLLOUT, mybe->server_myds->fd, mybe->server_myds, curtime); + thread->mypolls.add(POLLIN | POLLOUT, myds->fd, myds, curtime); if (mirror) { PROXY_TRACE(); } + } else { + // See Issue#4919 (https://github.com/sysown/proxysql/issues/4919) + // File descriptor was already set previously. Let's verify if it has changed + if (myds->fd != myconn->fd) + { + // PQconnectPoll has changed the file descriptor (FD) during the connection process. + // We need to update the new FD in mypolls, replacing the old one, + // Note: previous FD is closed by PQconnectPoll + myds->assign_fd_from_mysql_conn(); + thread->mypolls.update_fd_at_index(myds->poll_fds_idx, myds->fd); + } } switch (rc) { case 0: From d113bcb19375897c9437262ff50cd00779a71b35 Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Thu, 24 Apr 2025 19:46:33 +0500 Subject: [PATCH 4/5] Use PG_EVENT_WRITE status for PgSQL --- lib/Base_Thread.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/Base_Thread.cpp b/lib/Base_Thread.cpp index a70d119e4..d6ee97fe7 100644 --- a/lib/Base_Thread.cpp +++ b/lib/Base_Thread.cpp @@ -349,8 +349,13 @@ void Base_Thread::configure_pollout(DS * myds, unsigned int n) { } else { if (myds->DSS > STATE_MARIADB_BEGIN && myds->DSS < STATE_MARIADB_END) { thr->mypolls.fds[n].events = POLLIN; - if (thr->mypolls.myds[n]->myconn->async_exit_status & MYSQL_WAIT_WRITE) - thr->mypolls.fds[n].events |= POLLOUT; + if constexpr (std::is_same_v) { + if (thr->mypolls.myds[n]->myconn->async_exit_status & PG_EVENT_WRITE) + thr->mypolls.fds[n].events |= POLLOUT; + } else if constexpr (std::is_same_v) { + if (thr->mypolls.myds[n]->myconn->async_exit_status & MYSQL_WAIT_WRITE) + thr->mypolls.fds[n].events |= POLLOUT; + } } else { myds->set_pollout(); } From dd589508f4919ee071bdaa74a06ab519d2d07e1e Mon Sep 17 00:00:00 2001 From: Rahim Kanji Date: Thu, 24 Apr 2025 19:47:34 +0500 Subject: [PATCH 5/5] Added regression test --- .../pgsql-connection_parameters_test-t.cpp | 73 +++++++++++++++++-- 1 file changed, 66 insertions(+), 7 deletions(-) diff --git a/test/tap/tests/pgsql-connection_parameters_test-t.cpp b/test/tap/tests/pgsql-connection_parameters_test-t.cpp index 063a75e77..077d80ff4 100644 --- a/test/tap/tests/pgsql-connection_parameters_test-t.cpp +++ b/test/tap/tests/pgsql-connection_parameters_test-t.cpp @@ -620,14 +620,14 @@ bool test_parameters(PGconn* admin_conn, const parameter_test& test) { snprintf(buffer, sizeof(buffer), "SET %s='%s'", parameter.name.c_str(), parameter.value.c_str()); if (executeQueries(admin_conn, { buffer, "LOAD PGSQL VARIABLES TO RUNTIME" }) == false) { - BAIL_OUT("Error: failed to set admin variable in file %s, line %d", __FILE__, __LINE__); + diag("Error: failed to set admin variable in file %s, line %d", __FILE__, __LINE__); return false; } } int sock = connect_server(cl.pgsql_host, cl.pgsql_port); if (sock == -1) { - BAIL_OUT("Error: failed to connect to the server in file %s, line %d", __FILE__, __LINE__); + diag("Error: failed to connect to the server in file %s, line %d", __FILE__, __LINE__); return false; } @@ -671,7 +671,7 @@ bool test_parameters(PGconn* admin_conn, const parameter_test& test) { // Send StartupMessage if (send_data(sock, startup_msg.data(), startup_msg.size()) == false) { - BAIL_OUT("Error: failed to send startup message in file %s, line %d", __FILE__, __LINE__); + diag("Error: failed to send startup message in file %s, line %d", __FILE__, __LINE__); goto cleanup; } @@ -692,7 +692,7 @@ bool test_parameters(PGconn* admin_conn, const parameter_test& test) { auto result = execute_query(sock, buffer); if (result == nullptr) { - BAIL_OUT("Error: failed to execute query in file %s, line %d", __FILE__, __LINE__); + diag("Error: failed to execute query in file %s, line %d", __FILE__, __LINE__); goto cleanup; } @@ -707,7 +707,7 @@ bool test_parameters(PGconn* admin_conn, const parameter_test& test) { diag("Executing: %s\n", reset_cmd.c_str()); auto result = execute_query(sock, reset_cmd); if (result == nullptr) { - BAIL_OUT("Error: failed to reset parameter in file %s, line %d", __FILE__, __LINE__); + diag("Error: failed to reset parameter in file %s, line %d", __FILE__, __LINE__); goto cleanup; } if (result->error.empty() == false) { @@ -722,7 +722,7 @@ bool test_parameters(PGconn* admin_conn, const parameter_test& test) { diag("Executing: %s\n", show_cmd.c_str()); auto result = execute_query(sock, show_cmd); if (result == nullptr) { - BAIL_OUT("Error: failed to execute query in file %s, line %d", __FILE__, __LINE__); + diag("Error: failed to execute query in file %s, line %d", __FILE__, __LINE__); goto cleanup; } if (test.expect_failure == false && result->error.empty()) { @@ -859,6 +859,26 @@ std::vector test_cases = { } }; +constexpr int MAX_REG_ITERATION_PER_THREAD = 5; +constexpr int MAX_REG_THREAD = 2; + +void test_invalid_param_reg_4919_thread() { + auto admin_conn = createNewConnection(ConnType::ADMIN, "", false); + + if (!admin_conn || PQstatus(admin_conn.get()) != CONNECTION_OK) { + diag("Error: failed to connect to the database in file %s, line %d", __FILE__, __LINE__); + return; + } + + parameter_test invalid_param_test = test_cases.back(); + + for (int i = 0; i < MAX_REG_ITERATION_PER_THREAD; i++) { + if (test_parameters(admin_conn.get(), invalid_param_test) == false) { + diag("Error: failed to test parameters in file %s, line %d", __FILE__, __LINE__); + return; + } + } +} int main(int argc, char** argv) { @@ -879,6 +899,29 @@ int main(int argc, char** argv) { test_count += test_case.conn_params.size() * 2; } + // Regression test for Issue#4919 (https://github.com/sysown/proxysql/issues/4919) + int test_count_regression = 0; + + const auto& test_case = test_cases.back(); + + if (test_case.expect_failure) { + int case_count = 1; + + if (test_case.set_commands.empty() == false) + case_count++; + if (test_case.reset_after) + case_count++; + + test_count_regression += test_case.conn_params.size() * case_count; + } + else + test_count_regression += test_case.conn_params.size() * 2; + + test_count_regression *= MAX_REG_ITERATION_PER_THREAD * MAX_REG_THREAD; + test_count_regression += 1; // execute "select 1" to check if proxysql is alive + // Regression test for Issue#4919 + test_count += test_count_regression; + plan(test_count); if (cl.getEnv()) @@ -900,10 +943,26 @@ int main(int argc, char** argv) { for (const auto& test_case : test_cases) { if (test_parameters(admin_conn.get(), test_case) == false) { - BAIL_OUT("Error: failed to test parameters in file %s, line %d", __FILE__, __LINE__); + diag("Error: failed to test parameters in file %s, line %d", __FILE__, __LINE__); return exit_status(); } } + // Regression test for Issue#4919 (https://github.com/sysown/proxysql/issues/4919) + std::vector threads; + + for (int i = 0; i < MAX_REG_THREAD; ++i) { + threads.emplace_back(test_invalid_param_reg_4919_thread); + } + + for (auto& t : threads) { + t.join(); + } + + auto result = executeQueries(admin_conn.get(), {"SELECT 1"}); + + ok(result, "ProxySQL should be alive"); + // Regression test for Issue#4919 + return exit_status(); }