From a577491f428796a925e5ee35a6b363f87c7a7e6e Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Fri, 28 Nov 2025 01:26:02 +0000 Subject: [PATCH] Refactor issue 4855 fix: Use sqlite3_total_changes64 difference approach PROBLEM: The initial fix used a DDL detection approach which required maintaining a list of query types that should return 0 affected rows. This approach was brittle and could miss edge cases like commented queries or complex statements. SOLUTION: Instead of detecting DDL queries, use sqlite3_total_changes64() to measure the actual change count before and after each query execution. The difference between total_changes before and after represents the true affected rows count for the current query, regardless of query type. CHANGES: - Added proxy_sqlite3_total_changes64 function pointer and initialization - Rewrote execute_statement() and execute_statement_raw() to use total_changes difference approach - This automatically handles all query types (DDL, DML, comments, etc.) - Added comprehensive TAP test covering INSERT, CREATE, DROP, VACUUM, UPDATE, and BEGIN operations BENEFITS: - More robust and accurate than DDL detection approach - Handles edge cases like commented queries automatically - No maintenance overhead for new query types - Simpler and cleaner implementation - Still fixes both Admin interface and SQLite3 Server This approach is mathematically sound: affected_rows = total_changes_after - total_changes_before, which gives the exact number of rows changed by the current query execution. Fixes #4855 --- include/sqlite3db.h | 2 + lib/sqlite3db.cpp | 71 +++--------- .../reg_test_4855_affected_rows_ddl-t.cpp | 105 ++++++++++++++++++ 3 files changed, 120 insertions(+), 58 deletions(-) create mode 100644 test/tap/tests/reg_test_4855_affected_rows_ddl-t.cpp diff --git a/include/sqlite3db.h b/include/sqlite3db.h index ad9d0107b..bdd01fc9b 100644 --- a/include/sqlite3db.h +++ b/include/sqlite3db.h @@ -44,6 +44,7 @@ extern void (*proxy_sqlite3_free)(void*); extern int (*proxy_sqlite3_status)(int op, int *pCurrent, int *pHighwater, int resetFlag); extern int (*proxy_sqlite3_status64)(int op, long long *pCurrent, long long *pHighwater, int resetFlag); extern int (*proxy_sqlite3_changes)(sqlite3*); +extern long long (*proxy_sqlite3_total_changes64)(sqlite3*); extern int (*proxy_sqlite3_step)(sqlite3_stmt*); extern int (*proxy_sqlite3_config)(int, ...); extern int (*proxy_sqlite3_shutdown)(void); @@ -93,6 +94,7 @@ int (*proxy_sqlite3_status)(int op, int *pCurrent, int *pHighwater, int resetFla int (*proxy_sqlite3_status64)(int op, long long *pCurrent, long long *pHighwater, int resetFlag); int (*proxy_sqlite3_changes)(sqlite3*); +long long (*proxy_sqlite3_total_changes64)(sqlite3*); int (*proxy_sqlite3_step)(sqlite3_stmt*); int (*proxy_sqlite3_config)(int, ...); int (*proxy_sqlite3_shutdown)(void); diff --git a/lib/sqlite3db.cpp b/lib/sqlite3db.cpp index 7ba83cc8c..37d7f3cb1 100644 --- a/lib/sqlite3db.cpp +++ b/lib/sqlite3db.cpp @@ -17,14 +17,6 @@ #define USLEEP_SQLITE_LOCKED 100 -/** - * @brief Check if a SQL statement is a DDL query that doesn't affect rows - * - * @param query The SQL statement to check - * @return True if the statement is a DDL query that doesn't affect rows - */ -static bool is_ddl_query_without_row_changes(const char *query); - /** * @brief Constructor for the SQLite3_column class. * @@ -349,6 +341,8 @@ bool SQLite3DB::execute_statement(const char *str, char **error, int *cols, int *cols = (*proxy_sqlite3_column_count)(statement); if (*cols==0) { // not a SELECT *resultset=NULL; + // Get total changes before executing the statement + long long total_changes_before = (*proxy_sqlite3_total_changes64)(db); do { rc=(*proxy_sqlite3_step)(statement); if (rc==SQLITE_LOCKED || rc==SQLITE_BUSY) { // the execution of the prepared statement failed because locked @@ -360,14 +354,9 @@ bool SQLite3DB::execute_statement(const char *str, char **error, int *cols, int } } while (rc==SQLITE_LOCKED || rc==SQLITE_BUSY); if (rc==SQLITE_DONE) { - int changes = (*proxy_sqlite3_changes)(db); - // For DDL queries that don't affect rows, reset affected_rows to 0 - // to prevent incorrect reporting of previous DML affected rows - if (is_ddl_query_without_row_changes(str)) { - *affected_rows = 0; - } else { - *affected_rows = changes; - } + // Calculate affected rows as the difference in total changes + long long total_changes_after = (*proxy_sqlite3_total_changes64)(db); + *affected_rows = (int)(total_changes_after - total_changes_before); ret=true; } else { *error=strdup((*proxy_sqlite3_errmsg)(db)); @@ -384,40 +373,6 @@ __exit_execute_statement: return ret; } -/** - * @brief Check if a SQL statement is a DDL query that doesn't affect rows - * - * @param query The SQL statement to check - * @return True if the statement is a DDL query that doesn't affect rows - */ -static bool is_ddl_query_without_row_changes(const char *query) { - if (!query) return false; - - // Skip leading whitespace - while (isspace(*query)) { - query++; - } - - // Check for DDL statements that don't affect row counts - return ( - (strncasecmp(query, "CREATE", 6) == 0) || - (strncasecmp(query, "DROP", 4) == 0) || - (strncasecmp(query, "ALTER", 5) == 0) || - (strncasecmp(query, "TRUNCATE", 8) == 0) || - (strncasecmp(query, "VACUUM", 6) == 0) || - (strncasecmp(query, "REINDEX", 7) == 0) || - (strncasecmp(query, "ANALYZE", 7) == 0) || - (strncasecmp(query, "CHECKPOINT", 10) == 0) || - (strncasecmp(query, "PRAGMA", 6) == 0) || - (strncasecmp(query, "BEGIN", 5) == 0) || - (strncasecmp(query, "COMMIT", 6) == 0) || - (strncasecmp(query, "ROLLBACK", 8) == 0) || - (strncasecmp(query, "SAVEPOINT", 9) == 0) || - (strncasecmp(query, "RELEASE", 7) == 0) || - (strncasecmp(query, "EXPLAIN", 7) == 0) - ); -} - /** * @brief Executes a SQL statement and returns the result set without parsing it. * @@ -442,6 +397,8 @@ bool SQLite3DB::execute_statement_raw(const char *str, char **error, int *cols, *cols = (*proxy_sqlite3_column_count)(*statement); if (*cols==0) { // not a SELECT //*resultset=NULL; + // Get total changes before executing the statement + long long total_changes_before = (*proxy_sqlite3_total_changes64)(db); do { rc=(*proxy_sqlite3_step)(*statement); if (rc==SQLITE_LOCKED || rc==SQLITE_BUSY) { // the execution of the prepared statement failed because locked @@ -449,14 +406,9 @@ bool SQLite3DB::execute_statement_raw(const char *str, char **error, int *cols, } } while (rc==SQLITE_LOCKED || rc==SQLITE_BUSY); if (rc==SQLITE_DONE) { - int changes = (*proxy_sqlite3_changes)(db); - // For DDL queries that don't affect rows, reset affected_rows to 0 - // to prevent incorrect reporting of previous DML affected rows - if (is_ddl_query_without_row_changes(str)) { - *affected_rows = 0; - } else { - *affected_rows = changes; - } + // Calculate affected rows as the difference in total changes + long long total_changes_after = (*proxy_sqlite3_total_changes64)(db); + *affected_rows = (int)(total_changes_after - total_changes_before); ret=true; } else { *error=strdup((*proxy_sqlite3_errmsg)(db)); @@ -1065,6 +1017,7 @@ void SQLite3DB::LoadPlugin(const char *plugin_name) { proxy_sqlite3_status = NULL; proxy_sqlite3_status64 = NULL; proxy_sqlite3_changes = NULL; + proxy_sqlite3_total_changes64 = NULL; proxy_sqlite3_step = NULL; proxy_sqlite3_shutdown = NULL; proxy_sqlite3_prepare_v2 = NULL; @@ -1144,6 +1097,7 @@ void SQLite3DB::LoadPlugin(const char *plugin_name) { proxy_sqlite3_status = sqlite3_status; proxy_sqlite3_status64 = sqlite3_status64; proxy_sqlite3_changes = sqlite3_changes; + proxy_sqlite3_total_changes64 = sqlite3_total_changes64; proxy_sqlite3_step = sqlite3_step; proxy_sqlite3_shutdown = sqlite3_shutdown; proxy_sqlite3_prepare_v2 = sqlite3_prepare_v2; @@ -1173,6 +1127,7 @@ void SQLite3DB::LoadPlugin(const char *plugin_name) { assert(proxy_sqlite3_status); assert(proxy_sqlite3_status64); assert(proxy_sqlite3_changes); + assert(proxy_sqlite3_total_changes64); assert(proxy_sqlite3_step); assert(proxy_sqlite3_shutdown); assert(proxy_sqlite3_prepare_v2); diff --git a/test/tap/tests/reg_test_4855_affected_rows_ddl-t.cpp b/test/tap/tests/reg_test_4855_affected_rows_ddl-t.cpp new file mode 100644 index 000000000..bbd7e923a --- /dev/null +++ b/test/tap/tests/reg_test_4855_affected_rows_ddl-t.cpp @@ -0,0 +1,105 @@ +/* + Copyright (c) 2025 ProxySQL + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA */ + +#define PROXYSQL_EXTERN +#include +#include "tap.h" +#include +#include +#include + +#include "openssl/ssl.h" + +#include "mysql.h" +#include "proxysql_structs.h" +#include "sqlite3db.h" +#include "MySQL_LDAP_Authentication.hpp" + +MySQL_LDAP_Authentication* GloMyLdapAuth = nullptr; + +int main() { + SQLite3DB::LoadPlugin(NULL); + plan(15); + + { + int i=sqlite3_config(SQLITE_CONFIG_URI, 1); + if (i!=SQLITE_OK) { + fprintf(stderr,"SQLITE: Error on sqlite3_config(SQLITE_CONFIG_URI,1)\n"); + } + ok(i==SQLITE_OK, "Setting SQLITE_CONFIG_URI"); + } + + SQLite3DB *db; + db = new SQLite3DB(); + db->open((char *)"file:mem_db?mode=memory&cache=shared", SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_FULLMUTEX); + + char *error = NULL; + int cols = 0; + int affected_rows = 0; + SQLite3_result *resultset = NULL; + + // Test the fix: DDL queries should reset affected_rows to 0 + + // 1. First execute a DML that affects rows + bool rc = db->execute_statement("CREATE TABLE test_table (id INTEGER PRIMARY KEY, value TEXT)", &error, &cols, &affected_rows, &resultset); + ok(rc && error == nullptr, "CREATE TABLE test_table succeeds"); + ok(affected_rows == 0, "CREATE TABLE returns 0 affected rows"); + + // 2. Insert some data + error = NULL; cols = 0; affected_rows = 0; resultset = NULL; + rc = db->execute_statement("INSERT INTO test_table (value) VALUES ('test1')", &error, &cols, &affected_rows, &resultset); + ok(rc && error == nullptr, "INSERT first row succeeds"); + ok(affected_rows == 1, "INSERT returns 1 affected row"); + + // 3. Insert more data + error = NULL; cols = 0; affected_rows = 0; resultset = NULL; + rc = db->execute_statement("INSERT INTO test_table (value) VALUES ('test2')", &error, &cols, &affected_rows, &resultset); + ok(rc && error == nullptr, "INSERT second row succeeds"); + ok(affected_rows == 1, "INSERT returns 1 affected row"); + + // 4. Now execute a DDL - this should reset affected_rows to 0 (this was the bug) + error = NULL; cols = 0; affected_rows = 0; resultset = NULL; + rc = db->execute_statement("CREATE TABLE test_table2 (id INTEGER PRIMARY KEY)", &error, &cols, &affected_rows, &resultset); + ok(rc && error == nullptr, "CREATE TABLE test_table2 succeeds"); + ok(affected_rows == 0, "CREATE TABLE after DML returns 0 affected rows (bug fix verified)"); + + // 5. Test DROP TABLE + error = NULL; cols = 0; affected_rows = 0; resultset = NULL; + rc = db->execute_statement("DROP TABLE test_table2", &error, &cols, &affected_rows, &resultset); + ok(rc && error == nullptr, "DROP TABLE succeeds"); + ok(affected_rows == 0, "DROP TABLE returns 0 affected rows"); + + // 6. Test VACUUM + error = NULL; cols = 0; affected_rows = 0; resultset = NULL; + rc = db->execute_statement("VACUUM", &error, &cols, &affected_rows, &resultset); + ok(rc && error == nullptr, "VACUUM succeeds"); + ok(affected_rows == 0, "VACUUM returns 0 affected rows"); + + // 7. Test that DML still works correctly after DDL + error = NULL; cols = 0; affected_rows = 0; resultset = NULL; + rc = db->execute_statement("UPDATE test_table SET value = 'updated' WHERE id = 1", &error, &cols, &affected_rows, &resultset); + ok(rc && error == nullptr, "UPDATE after DDL succeeds"); + ok(affected_rows == 1, "UPDATE returns 1 affected row"); + + // 8. Test transaction commands + error = NULL; cols = 0; affected_rows = 0; resultset = NULL; + rc = db->execute_statement("BEGIN", &error, &cols, &affected_rows, &resultset); + ok(rc && error == nullptr, "BEGIN transaction succeeds"); + ok(affected_rows == 0, "BEGIN returns 0 affected rows"); + + delete db; + return exit_status(); +} \ No newline at end of file