Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't use a client query parameter for ORDER BY field #1106

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

derklaro
Copy link
Member

Motivation

The current SQL query to read chunks of data from the database uses a query parameter for the field to order by. To understand the issue that this causes, a small understanding how a query is executed is required:

  1. The call the prepareStatement is made
  2. Some internal pre-parsing happens, then the query is submitted to the mysql server which does some preprocessing and responds with information about the query. This information contains:
    1. The number of query fields that the client is supposed to send
    2. The number of response fields that the server will respond with
    3. The information about the query fields
    4. The information about the response fields
  3. The client driver reads the counts from the response and then the information about the response fields based on the supplied counts. Here comes the issue: while the MySQL server understands that a query field should be send for the order by field, the server is not sending any information about the field to the client. This causes the client to read the (as the server responsed with) 3 query fields, but the server only sends 2 query field information (for limit and offset). That means that the first response field is parsed as a query field, and when the client tries to read the information about the response fields, it will wait forever for the fifth field information, as the server only send four.

Modification

Instead of using a query parameter for the key the name of the field is directly inserted within the string.format call, removing the need to the query parameter.

Result

The MySQL database no longer wait infinitely for a field information packet to arrive, which never gets send by the server.

@derklaro derklaro added v: 4.X This pull should be included in the 4.0 release t: fix A pull request introducing a fix for a bug. in: module An issue/pull request releated to one of the internal modules labels Jan 31, 2023
@derklaro derklaro added this to the 4.0.0-RC8 milestone Jan 31, 2023
@derklaro derklaro requested a review from 0utplay January 31, 2023 14:23
@derklaro derklaro self-assigned this Jan 31, 2023
@0utplay 0utplay merged commit b6ca9d0 into nightly Jan 31, 2023
@0utplay 0utplay deleted the mysql-invalid-query-parameter branch January 31, 2023 16:10
@@ -224,7 +224,7 @@ public void iterate(@NonNull BiConsumer<String, JsonDocument> consumer) {
@Override
public @Nullable Map<String, JsonDocument> readChunk(long beginIndex, int chunkSize) {
return this.databaseProvider.executeQuery(
String.format("SELECT * FROM `%s` ORDER BY ? LIMIT ? OFFSET ?;", this.name),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did this ever work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, MariaDB responds with information for the field, so this was not noticed in the (unit) tests either 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: module An issue/pull request releated to one of the internal modules t: fix A pull request introducing a fix for a bug. v: 4.X This pull should be included in the 4.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants