fix(mysql): be explicit that batch queries are parameterless #10095

keynslug · 2023-03-08T14:48:14Z

So that mysql client won't attempt to prepare them automatically, thus trashing the server's prepared statements table and making whole interaction overall heavier. Also add tests to verify that we don't allocate more prepared statements than needed.

This PR also fixes an issue where some strings might have been inserted inconsistently, since escaping logic missed some edge cases.

EEC-782
EMQX-9167

thalesmg · 2023-03-08T19:06:20Z

lib-ee/emqx_ee_connector/src/emqx_ee_connector_tdengine.erl

@@ -160,7 +160,7 @@ status_result(_Status = false) -> connecting.
 %%========================================================================================

 do_batch_insert(InstanceId, BatchReqs, InsertPart, Tokens, State) ->
-    SQL = emqx_plugin_libs_rule:proc_batch_sql(BatchReqs, InsertPart, Tokens),
+    SQL = emqx_plugin_libs_rule:proc_batch_mysql(BatchReqs, InsertPart, Tokens),


Q: since this is also used in TDengine, maybe the function name could be something more generic (perhaps the old name)?

Either this or "fork" the old function so that TDengine uses the old version?

Did that as part of 2544742. I tried to raise some awareness there about pgsql in a comment, would be glad if you take another look.

thalesmg

I'm still digesting the escaping logic, but don't want to block the fix.

apps/emqx_plugin_libs/src/emqx_placeholder.erl

lafirest · 2023-03-09T02:48:26Z

apps/emqx_plugin_libs/src/emqx_placeholder.erl

@@ -28,7 +28,7 @@
    preproc_sql/1,
    preproc_sql/2,
    proc_sql/2,
-    proc_sql_param_str/2,
+    proc_mysql_param_str/2,


I think adding a new function with this name may be better than replacing the generic function, there are many SQL-likes backend.

Did that as part of 2544742. I tried to raise some awareness there about pgsql in a comment, would be glad if you take another look.

lafirest · 2023-03-09T02:56:52Z

@keynslug
awesome PR, could you share what happened and the reason for the performance improvement?
And a change file in English is required, I can translate it into Chinese.

keynslug · 2023-03-09T08:18:04Z

Thanks, @lafirest.

could you share what happened and the reason for the performance improvement?

This is actually a speculation if this will improve performance, but I suspect it will, slightly, because we now won't hit whole layer of caching prepared statements in the client.

And a change file in English is required, I can translate it into Chinese.

Sure, will do shortly, along with other post-review changes.

keynslug · 2023-03-09T08:25:41Z

could you share what happened and the reason for the performance improvement?

Replied too quickly. 😅 What I meant is that while trying to hit some hot codepath and pinpoint where does a performance regression come from, under my workload I actually observed message loss due to prepared statements table exhaustion on the server. So this PR is not actually addressing a root cause for the performance regression, but theoretically, might contribute to it.

Also hit some concerning error from the server (about SQL syntax error) that forced me to reimplement escaping logic.

coveralls · 2023-03-09T12:30:10Z

Pull Request Test Coverage Report for Build 4373849295

35 of 37 (94.59%) changed or added relevant lines in 3 files are covered.
28 unchanged lines in 13 files lost coverage.
Overall coverage increased (+0.01%) to 80.879%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
apps/emqx_connector/src/emqx_connector_mysql.erl	8	9	88.89%
apps/emqx_plugin_libs/src/emqx_placeholder.erl	26	27	96.3%

Files with Coverage Reduction	New Missed Lines	%
apps/emqx_machine/src/emqx_machine.erl	1	80.0%
apps/emqx/src/emqx_channel.erl	1	87.42%
apps/emqx/src/emqx_config.erl	1	86.51%
apps/emqx/src/emqx_connection.erl	1	87.96%
apps/emqx/src/emqx_logger.erl	1	80.49%
apps/emqx/src/emqx_schema.erl	1	90.91%
apps/emqx/src/emqx_session.erl	1	87.2%
lib-ee/emqx_ee_connector/src/emqx_ee_connector_influxdb.erl	1	90.05%
apps/emqx_resource/src/emqx_resource_buffer_worker_sup.erl	2	86.96%
apps/emqx/src/emqx_alarm.erl	2	91.89%

Totals
Change from base Build 4372045048:	0.01%
Covered Lines:	24149
Relevant Lines:	29858

💛 - Coveralls

keynslug · 2023-03-10T08:05:37Z

And a change file in English is required, I can translate it into Chinese.

Hi @lafirest, I've attached a changelog entry with DeepL-ed Chinese translation, much appreciate if you could take a quick read-through.

lafirest · 2023-03-10T09:45:49Z

changes/ee/fix-10095.zh.md

@@ -0,0 +1,3 @@
+阻止 mysql 客户端在每个批处理中用不必要的 `PREPARE` 查询反复轰炸mysql服务器，破坏服务器并耗尽其内部限制。当 mysql 桥处于批处理模式时，这种情况正在发生。


优化 MySQL 桥接在批量模式下能更高效的使用预处理语句 ，减少了对 MySQL 服务器的查询压力，并确保对 SQL 语句进行更安全和谨慎的转义。

I didn't translate it word for word, but the meaning is correct.

lafirest · 2023-03-10T09:48:35Z

changes/ee/fix-10095.en.md

@@ -0,0 +1,3 @@
+Stop mysql client from bombarding mysql server repeatedly with unnecessary `PREPARE` queries on every batch, trashing the server and exhausting its internal limits. This was happening when the mysql bridge was in the batch mode.


nit: mysql -> MySQL

Also make them look as recommended in the documentation.

So that mysql client won't attempt to prepare them automatically, thus trashing the server's prepared statements table and making interaction overall heavier.

Also hexencode non-utf8 binaries. This is essentially an heuristic. We don't know column types in runtime, and there's no simple way to find them out. Since we're already doing full binary scan during escaping it should be cheap to bail out on non-utf8 strings and hexencode them instead. Also introduce separate function to highlight that this escaping is MySQL-specific.

Bump `emqx_plugin_libs` app version to 4.3.7.

Similar to what we have in mysql and pgqsl testsuites.

keynslug requested review from a team, lafirest and JimMoen as code owners March 8, 2023 14:48

keynslug force-pushed the fix/EEC-782/mysql-prepstmt-exhaustion branch 2 times, most recently from ab7841d to db2cfbc Compare March 8, 2023 18:05

thalesmg reviewed Mar 8, 2023

View reviewed changes

thalesmg previously approved these changes Mar 8, 2023

View reviewed changes

apps/emqx_plugin_libs/src/emqx_placeholder.erl Show resolved Hide resolved

lafirest reviewed Mar 9, 2023

View reviewed changes

keynslug dismissed thalesmg’s stale review via 428921a March 9, 2023 11:07

keynslug force-pushed the fix/EEC-782/mysql-prepstmt-exhaustion branch 3 times, most recently from 37d1433 to 870f820 Compare March 9, 2023 11:13

keynslug requested review from lafirest and thalesmg March 9, 2023 16:05

thalesmg previously approved these changes Mar 9, 2023

View reviewed changes

lafirest reviewed Mar 10, 2023

View reviewed changes

keynslug dismissed thalesmg’s stale review via 49321c7 March 10, 2023 09:53

keynslug force-pushed the fix/EEC-782/mysql-prepstmt-exhaustion branch 3 times, most recently from 2757d46 to ac2b4a5 Compare March 10, 2023 10:23

keynslug requested review from Rory-Z and savonarola as code owners March 10, 2023 10:23

keynslug changed the base branch from release-50 to master March 10, 2023 10:23

ci(mysql): drop outdated config options

225cbab

Also make them look as recommended in the documentation.

keynslug added 5 commits March 10, 2023 18:42

fix(mysql): be explicit that batch queries are parameterless

fc37d9b

So that mysql client won't attempt to prepare them automatically, thus trashing the server's prepared statements table and making interaction overall heavier.

chore: drop unused macrodefs

781942c

chore: bump app versions + add changelog

e571b60

Bump `emqx_plugin_libs` app version to 4.3.7.

test(tde): add testcase for a nasty string in SQL query

f7c0d29

Similar to what we have in mysql and pgqsl testsuites.

keynslug force-pushed the fix/EEC-782/mysql-prepstmt-exhaustion branch from ac2b4a5 to f7c0d29 Compare March 10, 2023 15:43

Rory-Z approved these changes Mar 13, 2023

View reviewed changes

keynslug requested a review from lafirest March 13, 2023 17:17

zmstone approved these changes Mar 13, 2023

View reviewed changes

keynslug merged commit a530ccb into emqx:master Mar 14, 2023

keynslug deleted the fix/EEC-782/mysql-prepstmt-exhaustion branch March 14, 2023 07:21

sstrigler mentioned this pull request Mar 14, 2023

Release e5.0.2 alpha.1 #10134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mysql): be explicit that batch queries are parameterless #10095

fix(mysql): be explicit that batch queries are parameterless #10095

keynslug commented Mar 8, 2023 •

edited

thalesmg Mar 8, 2023

keynslug Mar 9, 2023

thalesmg left a comment

lafirest Mar 9, 2023

keynslug Mar 9, 2023

lafirest commented Mar 9, 2023

keynslug commented Mar 9, 2023

keynslug commented Mar 9, 2023

coveralls commented Mar 9, 2023

keynslug commented Mar 10, 2023

lafirest Mar 10, 2023

lafirest Mar 10, 2023

lafirest Mar 10, 2023

		@@ -0,0 +1,3 @@
		阻止 mysql 客户端在每个批处理中用不必要的 `PREPARE` 查询反复轰炸mysql服务器，破坏服务器并耗尽其内部限制。当 mysql 桥处于批处理模式时，这种情况正在发生。

		@@ -0,0 +1,3 @@
		Stop mysql client from bombarding mysql server repeatedly with unnecessary `PREPARE` queries on every batch, trashing the server and exhausting its internal limits. This was happening when the mysql bridge was in the batch mode.

fix(mysql): be explicit that batch queries are parameterless #10095

fix(mysql): be explicit that batch queries are parameterless #10095

Conversation

keynslug commented Mar 8, 2023 • edited

thalesmg Mar 8, 2023

Choose a reason for hiding this comment

keynslug Mar 9, 2023

Choose a reason for hiding this comment

thalesmg left a comment

Choose a reason for hiding this comment

lafirest Mar 9, 2023

Choose a reason for hiding this comment

keynslug Mar 9, 2023

Choose a reason for hiding this comment

lafirest commented Mar 9, 2023

keynslug commented Mar 9, 2023

keynslug commented Mar 9, 2023

coveralls commented Mar 9, 2023

Pull Request Test Coverage Report for Build 4373849295

💛 - Coveralls

keynslug commented Mar 10, 2023

lafirest Mar 10, 2023

Choose a reason for hiding this comment

lafirest Mar 10, 2023

Choose a reason for hiding this comment

lafirest Mar 10, 2023

Choose a reason for hiding this comment

keynslug commented Mar 8, 2023 •

edited