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

AMQ-9541 limit expensive queries on the database side #1275

Open
wants to merge 2 commits into
base: activemq-5.18.x
Choose a base branch
from

Conversation

jeanouii
Copy link

…ere there is a setMaxRows on the statement.


PreparedStatement s = null;
ResultSet rs = null;
try {
s = c.getConnection().prepareStatement(this.statements.getFindDurableSubMessagesByPriorityStatement());
s = c.getConnection().prepareStatement(this.limitQuery(this.statements.getFindDurableSubMessagesByPriorityStatement()));
s.setMaxRows(Math.min(maxReturned * 2, maxRows));
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why there is "* 2" for the setMaxRows

Copy link
Contributor

@DressedUpZebra DressedUpZebra Jul 27, 2024

Choose a reason for hiding this comment

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

Not an expert, but I did some git blame digging, the * 2 got introduced in https://issues.apache.org/jira/browse/AMQ-2980, to take into account the messages from low and high priority queues, to ensure that none get lost and to not hog more memory than needed. Down the road this happened java.sql.SQLException: setMaxRows() out of range. 50871918 > 50000000. (https://issues.apache.org/jira/browse/AMQ-1870). The solution to this was to pick the lowest of two values (maxReturner * 2) and maxRows, to ensure such exception does not happen.


PreparedStatement s = null;
ResultSet rs = null;
try {
s = c.getConnection().prepareStatement(this.statements.getFindDurableSubMessagesStatement());
s = c.getConnection().prepareStatement(this.limitQuery(this.statements.getFindDurableSubMessagesStatement()));
s.setMaxRows(Math.min(maxReturned * 2, maxRows));
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why there is "* 2" for the setMaxRows

@jeanouii jeanouii force-pushed the AMQ-9541_limit-queries branch from 743c91c to e66d2a0 Compare July 25, 2024 15:08
@jeanouii jeanouii marked this pull request as ready for review July 25, 2024 17:23
@kenliao94
Copy link
Contributor

kenliao94 commented Dec 2, 2024

@jeanouii i Sorry, not following this logic here. Isn't this.limitQuery a no-ops? It just returns the original query (line 1188)

^ Ah I guess this is the default. It will be over-written in the class that extends it.

@kenliao94
Copy link
Contributor

kenliao94 commented Dec 2, 2024

@jeanouii LGTM. However, this PR should be open against the main branch not a release branch. If you want the fix to only be in a release branch, it can be backported later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants