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

Enabled PHP Syntax Highlighting #1

Merged
merged 1 commit into from
Apr 26, 2017
Merged

Enabled PHP Syntax Highlighting #1

merged 1 commit into from
Apr 26, 2017

Conversation

DavidLambauer
Copy link
Contributor

No description provided.

@brobie brobie merged commit 1e29c52 into degdigital:master Apr 26, 2017
jantzenw added a commit that referenced this pull request Oct 4, 2022
…records

We could think of two possible ways to resolve this:
1) Remove the query's LIMIT
2) Fix the code to correctly use the query's LIMIT + OFFSET filters, which is what the code was trying to do before this change.

Both have significant downsides. #1 can drastically increase the memory footprint of the export logic, because now the full results are loaded into memory. #2 can result in exceptionally slow exports, because LIMIT + OFFSET causes a large query to be run multiple times, PLUS the fact that OFFSET can have extremely bad performance with large values.

We went with option #1. We found that the performance impact of the export and the load on the database of #2 can be too significant with large, heavy queries. With #1, a report CSV would have to be in somewhere over 100MB before the risk of hitting the PHP memory limit is significant. We feel this is a much smaller likelihood of happening, and for clients that would need to be exporting that much data would have the technical expertise to implement #2 if necessary, or shrink the query results somehow. #2 affects a greater number of users and causes a frustrating performance hit.
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.

2 participants