From 41f7ab613d87311442860ccab5b1ce38640b7ddd Mon Sep 17 00:00:00 2001 From: eldenmoon Date: Sun, 16 Feb 2025 22:43:20 +0800 Subject: [PATCH 1/2] [BugFix](PreparedStatement) fix stmtId overflow if stmtId larger than Interger.MAX_VALUE, there is chance to return `Not supported such prepared statement` to the client which indicates the stmtId not exist.In order to fix this bug: 1. change id from original `stmtId` to `preparedStmtId` in ConnectContext 2. rolling the id to fit the range [0, 2147483647) --- .../java/org/apache/doris/common/Config.java | 5 ++++ .../trees/plans/commands/PrepareCommand.java | 2 +- .../org/apache/doris/qe/ConnectContext.java | 12 ++++++++- .../doris/qe/MysqlConnectProcessor.java | 4 +-- .../org/apache/doris/qe/StmtExecutor.java | 5 ++-- .../data/prepared_stmt_p0/prepared_stmt.out | 27 +++++++++++++++++++ .../prepared_stmt_p0/prepared_stmt.groovy | 27 +++++++++++++++++++ 7 files changed, 75 insertions(+), 7 deletions(-) diff --git a/fe/fe-common/src/main/java/org/apache/doris/common/Config.java b/fe/fe-common/src/main/java/org/apache/doris/common/Config.java index 60f59460a0a34b..07a6b03bf15865 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/common/Config.java +++ b/fe/fe-common/src/main/java/org/apache/doris/common/Config.java @@ -140,6 +140,11 @@ public class Config extends ConfigBase { description = {"是否检查table锁泄漏", "Whether to check table lock leaky"}) public static boolean check_table_lock_leaky = false; + @ConfField(mutable = true, masterOnly = false, + description = {"PreparedStatement stmtId 起始位置，仅用于测试", + "PreparedStatement stmtId starting position, used for testing onl"}) + public static long prepared_stmt_start_id = -1; + @ConfField(description = {"插件的安装目录", "The installation directory of the plugin"}) public static String plugin_dir = EnvUtils.getDorisHome() + "/plugins"; diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/PrepareCommand.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/PrepareCommand.java index ec654d7a93dce8..ed201e60c44821 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/PrepareCommand.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/PrepareCommand.java @@ -111,7 +111,7 @@ public void run(ConnectContext ctx, StmtExecutor executor) throws Exception { ctx.addPreparedStatementContext(name, new PreparedStatementContext(this, ctx, ctx.getStatementContext(), name)); if (ctx.getCommand() == MysqlCommand.COM_STMT_PREPARE) { - executor.sendStmtPrepareOK((int) ctx.getStmtId(), labels); + executor.sendStmtPrepareOK(Integer.parseInt(name), labels); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java b/fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java index 56ef149b2ab016..d83d0683d702fc 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java +++ b/fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java @@ -117,6 +117,7 @@ public enum ConnectType { // set for http_stream protected volatile TUniqueId loadId; protected volatile long backendId; + protected int preparedStmtId = 0; protected volatile LoadTaskInfo streamLoadInfo; protected volatile TUniqueId queryId = null; @@ -415,10 +416,11 @@ public void addPreparedStatementContext(String stmtName, PreparedStatementContex } if (this.preparedStatementContextMap.size() > sessionVariable.maxPreparedStmtCount) { throw new UserException("Failed to create a server prepared statement" - + "possibly because there are too many active prepared statements on server already." + + " possibly because there are too many active prepared statements on server already." + "set max_prepared_stmt_count with larger number than " + sessionVariable.maxPreparedStmtCount); } this.preparedStatementContextMap.put(stmtName, ctx); + incPreparedStmtId(); } public void removePrepareStmt(String stmtName) { @@ -445,6 +447,14 @@ public long getStmtId() { return stmtId; } + public long getPreparedStmtId() { + return preparedStmtId; + } + + public void incPreparedStmtId() { + ++preparedStmtId; + } + public long getBackendId() { return backendId; } diff --git a/fe/fe-core/src/main/java/org/apache/doris/qe/MysqlConnectProcessor.java b/fe/fe-core/src/main/java/org/apache/doris/qe/MysqlConnectProcessor.java index b4102e0d0e2152..e7b5791c98c312 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/qe/MysqlConnectProcessor.java +++ b/fe/fe-core/src/main/java/org/apache/doris/qe/MysqlConnectProcessor.java @@ -190,9 +190,7 @@ private void handleExecute() { // nererids PreparedStatementContext preparedStatementContext = ctx.getPreparedStementContext(String.valueOf(stmtId)); if (preparedStatementContext == null) { - if (LOG.isDebugEnabled()) { - LOG.debug("No such statement in context, stmtId:{}", stmtId); - } + LOG.warn("No such statement in context, stmtId:{}", stmtId); ctx.getState().setError(ErrorCode.ERR_UNKNOWN_COM_ERROR, "msg: Not supported such prepared statement"); return; diff --git a/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java b/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java index ed663a1d42f954..cf9e91d6141bcf 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java +++ b/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java @@ -715,9 +715,10 @@ private void executeByNereids(TUniqueId queryId) throws Exception { if (logicalPlan instanceof UnsupportedCommand || logicalPlan instanceof CreatePolicyCommand) { throw new MustFallbackException("cannot prepare command " + logicalPlan.getClass().getSimpleName()); } - logicalPlan = new PrepareCommand(String.valueOf(context.getStmtId()), + long stmtId = Config.prepared_stmt_start_id > 0 + ? Config.prepared_stmt_start_id : context.getPreparedStmtId(); + logicalPlan = new PrepareCommand(String.valueOf(stmtId % Integer.MAX_VALUE), logicalPlan, statementContext.getPlaceholders(), originStmt); - } // when we in transaction mode, we only support insert into command and transaction command if (context.isTxnModel()) { diff --git a/regression-test/data/prepared_stmt_p0/prepared_stmt.out b/regression-test/data/prepared_stmt_p0/prepared_stmt.out index 052094cd7d66c9..15473c1c57124f 100644 --- a/regression-test/data/prepared_stmt_p0/prepared_stmt.out +++ b/regression-test/data/prepared_stmt_p0/prepared_stmt.out @@ -135,3 +135,30 @@ a -- !select24 -- 1 \N [{"id":"1", "name":"doris"}, {"id":"2", "name":"apache"}, null] \N +-- !overflow_1 -- +1 + +-- !overflow_2 -- +2 + +-- !overflow_2 -- +2 + +-- !overflow_3 -- +3 + +-- !overflow_3 -- +3 + +-- !overflow_4 -- +4 + +-- !overflow_4 -- +4 + +-- !overflow_5 -- +5 + +-- !overflow_5 -- +5 + diff --git a/regression-test/suites/prepared_stmt_p0/prepared_stmt.groovy b/regression-test/suites/prepared_stmt_p0/prepared_stmt.groovy index 7b4bf401bf0390..3556879cfc35b5 100644 --- a/regression-test/suites/prepared_stmt_p0/prepared_stmt.groovy +++ b/regression-test/suites/prepared_stmt_p0/prepared_stmt.groovy @@ -302,4 +302,31 @@ suite("test_prepared_stmt", "nonConcurrent") { assertEquals(com.mysql.cj.jdbc.ServerPreparedStatement, stmt_read.class) qe_select24 stmt_read } + + // test stmtId overflow + def result2 = connect(user, password, url) { + def stmt_read1 = prepareStatement "select 1" + qe_overflow_1 stmt_read1 + // int max + sql """admin set frontend config("prepared_stmt_start_id" = "2147483647");""" + def stmt_read2 = prepareStatement "select 2" + qe_overflow_2 stmt_read2 + qe_overflow_2 stmt_read2 + // int max + 1 + sql """admin set frontend config("prepared_stmt_start_id" = "2147483648");""" + def stmt_read3 = prepareStatement "select 3" + qe_overflow_3 stmt_read3 + qe_overflow_3 stmt_read3 + // uint max + 1 + sql """admin set frontend config("prepared_stmt_start_id" = "4294967296");""" + def stmt_read4 = prepareStatement "select 4" + qe_overflow_4 stmt_read4 + qe_overflow_4 stmt_read4 + + // set back + sql """admin set frontend config("prepared_stmt_start_id" = "-1");""" + def stmt_read5 = prepareStatement "select 5" + qe_overflow_5 stmt_read5 + qe_overflow_5 stmt_read5 + } } From 056096014d6361da82ba982ffcc0d37043d21adf Mon Sep 17 00:00:00 2001 From: eldenmoon Date: Mon, 17 Feb 2025 10:55:01 +0800 Subject: [PATCH 2/2] add test --- .../org/apache/doris/qe/ConnectContext.java | 3 +- .../org/apache/doris/qe/StmtExecutor.java | 2 +- .../data/prepared_stmt_p0/prepared_stmt.out | 12 +++++-- .../prepared_stmt_p0/prepared_stmt.groovy | 31 +++++++++++++++---- 4 files changed, 37 insertions(+), 11 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java b/fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java index d83d0683d702fc..99fa9cc73888c8 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java +++ b/fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java @@ -117,7 +117,8 @@ public enum ConnectType { // set for http_stream protected volatile TUniqueId loadId; protected volatile long backendId; - protected int preparedStmtId = 0; + // range [Integer.MIN_VALUE, Integer.MAX_VALUE] + protected int preparedStmtId = Integer.MIN_VALUE; protected volatile LoadTaskInfo streamLoadInfo; protected volatile TUniqueId queryId = null; diff --git a/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java b/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java index cf9e91d6141bcf..53f34613de0ac9 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java +++ b/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java @@ -717,7 +717,7 @@ private void executeByNereids(TUniqueId queryId) throws Exception { } long stmtId = Config.prepared_stmt_start_id > 0 ? Config.prepared_stmt_start_id : context.getPreparedStmtId(); - logicalPlan = new PrepareCommand(String.valueOf(stmtId % Integer.MAX_VALUE), + logicalPlan = new PrepareCommand(String.valueOf(stmtId), logicalPlan, statementContext.getPlaceholders(), originStmt); } // when we in transaction mode, we only support insert into command and transaction command diff --git a/regression-test/data/prepared_stmt_p0/prepared_stmt.out b/regression-test/data/prepared_stmt_p0/prepared_stmt.out index 15473c1c57124f..9cadb98813af02 100644 --- a/regression-test/data/prepared_stmt_p0/prepared_stmt.out +++ b/regression-test/data/prepared_stmt_p0/prepared_stmt.out @@ -135,9 +135,6 @@ a -- !select24 -- 1 \N [{"id":"1", "name":"doris"}, {"id":"2", "name":"apache"}, null] \N --- !overflow_1 -- -1 - -- !overflow_2 -- 2 @@ -162,3 +159,12 @@ a -- !overflow_5 -- 5 +-- !overflow_6 -- +6 + +-- !overflow_6 -- +6 + +-- !overflow_6 -- +6 + diff --git a/regression-test/suites/prepared_stmt_p0/prepared_stmt.groovy b/regression-test/suites/prepared_stmt_p0/prepared_stmt.groovy index 3556879cfc35b5..ced7041e9fb772 100644 --- a/regression-test/suites/prepared_stmt_p0/prepared_stmt.groovy +++ b/regression-test/suites/prepared_stmt_p0/prepared_stmt.groovy @@ -305,28 +305,47 @@ suite("test_prepared_stmt", "nonConcurrent") { // test stmtId overflow def result2 = connect(user, password, url) { - def stmt_read1 = prepareStatement "select 1" - qe_overflow_1 stmt_read1 + // def stmt_read1 = prepareStatement "select 1" + // assertEquals(com.mysql.cj.jdbc.ServerPreparedStatement, stmt_read1.class) + // qe_overflow_1 stmt_read1 + // stmt_read1.close() // int max sql """admin set frontend config("prepared_stmt_start_id" = "2147483647");""" def stmt_read2 = prepareStatement "select 2" + assertEquals(com.mysql.cj.jdbc.ServerPreparedStatement, stmt_read2.class) qe_overflow_2 stmt_read2 qe_overflow_2 stmt_read2 + stmt_read2.close() // int max + 1 sql """admin set frontend config("prepared_stmt_start_id" = "2147483648");""" def stmt_read3 = prepareStatement "select 3" + // overflow throw NumberFormatExceptio and fallback to ClientPreparedStatement + assertEquals(com.mysql.cj.jdbc.ClientPreparedStatement, stmt_read3.class) qe_overflow_3 stmt_read3 qe_overflow_3 stmt_read3 - // uint max + 1 - sql """admin set frontend config("prepared_stmt_start_id" = "4294967296");""" + stmt_read3.close() + // int min + sql """admin set frontend config("prepared_stmt_start_id" = "2147483646");""" def stmt_read4 = prepareStatement "select 4" + assertEquals(com.mysql.cj.jdbc.ServerPreparedStatement, stmt_read4.class) qe_overflow_4 stmt_read4 qe_overflow_4 stmt_read4 + stmt_read4.close() - // set back - sql """admin set frontend config("prepared_stmt_start_id" = "-1");""" + sql """admin set frontend config("prepared_stmt_start_id" = "123");""" def stmt_read5 = prepareStatement "select 5" + assertEquals(com.mysql.cj.jdbc.ServerPreparedStatement, stmt_read5.class) qe_overflow_5 stmt_read5 qe_overflow_5 stmt_read5 + stmt_read5.close() + + // set back + sql """admin set frontend config("prepared_stmt_start_id" = "-1");""" + def stmt_read6 = prepareStatement "select 6" + assertEquals(com.mysql.cj.jdbc.ServerPreparedStatement, stmt_read6.class) + qe_overflow_6 stmt_read6 + qe_overflow_6 stmt_read6 + qe_overflow_6 stmt_read6 + stmt_read6.close() } }