Batch download of submissions returns all submissions on each batch run

Mmmmm, not saying that something isn't broken, but I'm pretty sure this isn't the right fix. It is pretty complicated code, so without stepping through various options, I may be wrong, but...

Thanks for the report.

Looking at webform_download_sids() it appears to use a pager query and $_GET['page'] to communicate which batch of sids should be return. I'm pretty sure that the intention is to never load every sid.

I do see some code in webform_results_download_range_validate() that loads the sids for the specified range into the the $form_state, and then code in the submit handler webform_results_download_form_submit that for all other range_types but 'all' loads the sids too.

This surprises me because it would seem to go against just using the pager to get the right sids for that batch. I don't understand why it is implemented like this. 'range_serial' was added at one point and that may be when the validation handler got added.

What I *think* should be happening is that the list of sids should never be passed in the sandbox, but rather the pager query should be getting the right batch.

What download range options were you using? Let's see if we can fix the issue plus make it work correctly. With very large webforms, downloading all the sids may well be a time or memory problem.

There's lots that I don't like looking at this report code.

1. There seems to be special case code scattered all over the place.
2. The first time through, the sid's are queried for the count, then queried again, then queried again to get the submissions. Subsequent times, the first extra query is skipped.
3. I'm not sure why we are hacking around using a pager query in webform_download_sids(). It would probably be simpler to just get the next batch without having to set and restore $_GET['page'].
4. webform_get_submissions goes to quite a lot of work to query the submissions table, but that work was *just* done in webform_download_sids(). I'm thinking that if we can pass an unexecuted select query around, we can use that to get the same sids. It is insane to retrieve an array of 10,000 sids and then pass them to another query in an "IN" clause.
5. The validation code only runs for one range case, and retrieves every single sid, when the first one would be sufficient.

This is going to take some study and good testing. @pixkat -- maybe you can use your 100,000 submission database for some testing.

I re-worked both the submission and the report_sid code to split the generation of the query from the execution of the query. I then used the optimization in the related "10x speed improvement"* issue when fetching the submission data. However, I'm not getting good performance, so I need to see if it is related to my development machine, the code changes, or something else.

What batch size were you able to use successfully and on what server? The 10,000 "time" limit seems overly optimistic. It may have been picked before all the hooks were added to each submission. I'm thinking something more like 1000 might be more reasonable, and even that might be high.

* The idea is to use an IN condition with a subquery, rather than a huge array of SIDs and an IN clause. When I generalized it to work with any query, rather than all the submissions, it doesn't work because IN doesn't support LIMIT clauses in the SID. So I went to a join, which may not be fast.

If you're willing to test some patches with your big database of submissions and your server, that might be helpful. I made a 20K submission webform for testing.

OK. I figured out my performance issues, which were actually functional issues too.

On my development server, I went from exporting a batch of 10,000 records with the current dev in 41 seconds to 21 seconds with this patch. The time to load the data for 10,000 submissions went from about 16.5 seconds to about 1 second. This is the power of using the join rather than an IN clause with an array of 10,000 sid's.

This patch has two main parts. The following should help with a code review.
webform.submissions.inc

1. webform_get_submissions() is refactored into webform_get_submissions_query(), which takes the same arguments as webform_get_submissions() but returns an unexecuted query or extended query (pager, tablesort).
2. webform_get_submissions_query required table-qualified column names so that the join can work. There was some work to do this, including inferring the table from the column (in the case of uid, which is users, not webform_submissions).
3. The second part of webform_get_submissions() is now webform_submissions_load, which takes the query created above. webform_get_submissions is a trivial function which calls _query and then _load.
4. webform_get_submissions_load() needs to make a subquery for the join. It can only do this if it is given an unextended query (i.e. this optimization doesn't work when paging or sorting the submissions table). Like SelectQuery->countQuery(), it removes unneeded fields.
5. Because _load no longer knows the specified filters, it infers the nid by looking through the conditionals for it. This is used in optimizing the webform_submissions_data query because the index is on nid_sid.
6. The array if sid's no longer needs to be build in PHP as it either comes in as an argument or isn't needed because of the join.

webform.report.inc

1. The draft form element is now always added, but hidden with #access when drafts aren't enabled. This simplifies the code because the value (all, but default) is always in the form.
2. The form validation code now gets a count of the submissions satisfying the validation for all cases, not just range_serial. For efficiency, it limits the query to 1 response. Should always be fast.
3. When building the form, rather than fetching all the matching sid's, it uses a count query instead. This is about twice as fast in my testing, plus avoid potential memory problems (with, say 100,000 submissions).
4. The submit handler never uses a passed list of submissions. It always generated them for the particular batch run.
5. webform_results_export(), which is deprecated, create the array of submissions with webform_get_submissions() and then passes it to webform_results_download_rows, which now takes a list of submissions. This won't use the optimized join, but you the batched version should be used instead anyhow.
6. webform_results_download_rows receives the submissions as an argument, rather than creating them itself.
7. webform_results_batch_rows, uses a count query for counting the total number of submissions to be exported. It then creates an appropriate query for that one batch and downloads them.
8. webform_download_sids() is deprecated. It just calls the new webform_download_sids_query() and executes it.
9. webform_download_sids_count() calls the new webform_download_sids_query(), makes it into a count query, and executes it.
10. webform_download_sids_query create the actual query. It no longer uses a pager query, which I think was a remnant from some older code because I find no place where paging is used with downloading. Instead, it just adds a range clause. This removes a hack involving $_GET['page'].
11. The code to update the last-downloaded sid was corrected for range_serial (which may not be the latest).

Patch forthcoming after some additional testing. Yikes.

Here's the patch. It also cleans up the writing of the last download sid. Needs testing and review.

Some substantial changes here, but the performance gains look good.

At this stage I've not tested the patch so this is just a code review. Overall the theory of using the JOIN rather than IN makes a lot of sense. Splitting webform_get_submissions() into _query and _load seems ok, especially for BC.

Following are mostly nits:

1. +++ b/includes/webform.report.inc
   @@ -943,24 +952,13 @@ function webform_results_download_headers($node, $options) {
   -function webform_results_download_rows($node, $options, $serial_start = 0) {
   +function webform_results_download_rows($node, $options, $submissions, $serial_start = 0) {
   

   While this isn't exactly a public facing function changing its signature in the stable 7-x-4.x branch probably isn't a good idea. $submissions as the last param perhaps?

2. +++ b/includes/webform.report.inc
   @@ -1236,19 +1231,28 @@ function webform_results_batch_headers($node, $format = 'delimited', $options =
   +  // Initialize the sandbox if this is the first execution of the batch operation.
   

   > 80 chars. Quite a few instances of this.

3. +++ b/includes/webform.report.inc
   @@ -1236,19 +1231,28 @@ function webform_results_batch_headers($node, $format = 'delimited', $options =
   +  ¶
   

   Whitespace.

4. +++ b/includes/webform.report.inc
   @@ -1603,8 +1608,42 @@ function theme_webform_results_analysis($variables) {
   + * Given a set of range options, retrieve a count the number of matching
   + * submissions.
   + *
   

   Should be on one line, no trailing line.

5. +++ b/includes/webform.submissions.inc
   @@ -701,22 +701,33 @@ function webform_submission_render($node, $submission, $email, $format, $exclude
   + * Return an unexecuted query on the webform_submissions table for the
   + * specified webform_get_submssions arguments.
   

   Again, one line.

6. +++ b/includes/webform.report.inc
   @@ -1603,8 +1608,42 @@ function theme_webform_results_analysis($variables) {
   + * @deprecated This function is scheduled to be removed int webform 7.x-5.x.
   + * Use webform_download_sids_query() instead.
   

   int > in
   7.x-5.x hey, good on you Dan!

As for the batch size: I usually go for 100 for entities (which obviously are still a lot slower in D7). As a general rule of thumb each batch should take a few seconds (2-5). Then the time spent with querying the database for the data of the next batch is negligible compared with the time spent doing the actual calculations - but it still allows for a fine control wrt to the max execution time. The filter query seems to be simple enough that it's feasible to run it again for every batch. We have to look out for that once we allow for more complex filters though (>8 joins or GROUP BYs).

Also it depends a bit on the size of one single submission. If we store large text-areas with KBytes of text 1000 submissions already use up MBytes of RAM. Essentially we have to limit it to (memory_limit - rest_of_drupal)/(average memory size of a submission).

I've also found two other suggestions for the code:

1. +++ b/includes/webform.submissions.inc
   @@ -776,35 +792,83 @@ function webform_get_submissions($filters = array(), $header = NULL, $pager_coun
   +    foreach ($submission_query_fields as $alias => $field) {
   +      unset($submission_query_fields[$alias]);
   +    }
   

   You could also do a $submission_query_fields = array(); instead.

2. +++ b/includes/webform.submissions.inc
   @@ -776,35 +792,83 @@ function webform_get_submissions($filters = array(), $header = NULL, $pager_coun
   +    unset($submission_query_fields); // Drop PHP reference.
   

   That's duplicated and perhaps should unset $submission_query_tables the second time.

Good work!

Updated patch based on comments.

#13.1 - I suppose, although it would seem very unlikely that anyone would call it. I can't just change the parameter order because the functionality is actually split into two function; $submissions isn't optional. I moved the obsolete submission retrieval code back out of webform_results_export and back into webform_results_download_rows. I then moved the body of webform_results_download_rows into a new function webform_results_download_rows_process. I put the submissions as the last parameter to harmonize the parameter order of the two, and made the $start_serial not optional (which is fine since this is a new function. A bit messier, but does support someone calling webform_ressults_download_rows(). This begs the issue of which functions in a module are public and which ones aren't. Just because it doesn't start with an underscore doesn't mean someone should be calling it.

#13.2 - I was astonished to re-read the coding standards and find that all comments are supposed to be <= 80 columns. I could have sworn it was just docblocks. This is, of course, a very stupid standard but I will abide by it when there is no reason not to. I quickly re-wrapped these two source files, rather than try to find the new comments and just wrap those.

#13.5 - Ha. I also didn't know that the first line summary had to be both 1 line and <= 80 columns. Ya know, because it's more important to be brief than informative. Someone might be browsing code on a VT-100 terminal.

#14.1 - D'oh. Of course. I copy/pasted code from SelectQuery::countQuery and didn't notice what I was doing.

#14.2 - Good catch! PHP references that aren't formal arguments are timebombs waiting to create bugs.

@pixlkat - "this patch" -- do you #12 or #15 (which I mistakenly called -14.patch)?

Did you use the default batch size of 10000, or something smaller? If so, what?

@torotil -- The batch size guestimate is based upon both memory and time. The time limit is 10000. I want to hear what pixlkat has to say, but on my relatively slow windows machine, a 10K batch took about 21 seconds. I'm thinking maybe of changing the 10K to 5K. The drush limit can stay 10K since time isn't a factor with drush.

Re #20-1 -- That's not what that unset does. $submission_query_tables is a reference. You should always unset a reference after you're done with it because if you mistakenly reuse the variable (easy to do if it's called, say, $node) then you end up setting what the variable is referred to, rather than it.

Re #20-2 -- Wow, it's worse than that. I didn't read SelectQuery::conditions() closely enough. It does return a '#conjunction' => 'AND' (or 'OR') element, which needs to be skipped. But worse, my "test" for $condition['field'] was actually an assignment. I think it just happened to work because the nid was always the first condition. Yikes, that could have been a nasty bug to find.

Thanks for the review! This patch fixes #20-2.

Based on your numbers and my testing which showed that I used 2/3rd of the 30 second timeout, I think lowering the default batch size to 5000 for the time limit is prudent. A 3% time penalty seems worthwhile for dramatically increasing the margin. My testing of a 5000 batch was about 10 seconds, giving an ample margin.

PS. I'm using PHP 5.3.13, which casts the 'operator' to 0 and returns the first character of '#conjunction', a '#' which is not equal to '='. So that's why it worked for me and I got no warnings.

You must be using a different version. And I'm using error_reporting of E_ALL | E_STRICT. Sheesh.

Just quickly Re: coding standards, all comments <= 80 chars, however function descriptions can be > 80 but must be on one line. Also, there's still whitespace in the patch, not sure what your editor is Dan but try setting it to get rid of it for you. Good work here people!

Coding standards:

All summaries (first lines of docblocks) must be under 80 characters, start with a capital letter, and end with a period (.). They must provide a brief description of what a function does, what a class does, what a file contains, etc.

There are exceptions -- hook_update_N comments are used in the UI, for example.

What whitespace are you seeing? I use Komodo IDE and it is set to remove trailing whitespace and fix EOL markers, but only for changed lines. I think this is the best/correct setting.

Great job on this Dan. Passing along a select query looks like a much better solution than the $sids array all over the place. FYI, we were using SIDs previously because we were coming from a place (D6) where there wasn't such a thing as a query object. Additionally, older versions of MySQL we used to support did not support subqueries. We still don't use them frequently in Drupal, it would be prudent to test this set of changes on PostGres to ensure we're not breaking support there.

-    $context['sandbox']['sid_count'] = count($sids);
+    $context['sandbox']['sid_count'] = webform_download_sids_count($node->nid, $options['range']);;

Double semicolon at the end of the line.

+  // If the "$pager_query" is actually an unextended select query, then instead
+  // of querying the webform_submissions_data table with a pontentially huge
+  // array of sids in an IN clause, use the select query directy as this will
+  // be much faster.
+  if ($pager_query instanceof SelectQuery) {
+    $submissions_query = clone $pager_query;
+    // Convert the submissions query into a query suitable for a join.
+    // @see SelectQuery::countQuery().
+    $submission_query_fields = &$submissions_query->getFields();
+    $submission_query_fields = array();
+    unset($submission_query_fields); // Drop PHP reference.
+    $submissions_query->addField('ws', 'sid');
+    $submission_query_tables = &$submissions_query->getTables();
+    foreach ($submission_query_tables as $alias => $table) {
+      unset($submission_query_tables[$alias]['all_fields']);
+    }
+    unset($submission_query_tables); // Drop PHP reference.
+  }

This conversion from one query type to another is really difficult to follow and is deeply tied to DBTNG's internals. Is there a way we could avoid this conversion and pass in an adequate query in the first place? Or if this optimization is only for backwards-compatibility, it would be fine to leave the IN (:sid) situation unoptimized. Given the general utility of this function, I expect it will start getting used to load submissions from manually-assembled queries that may not match the input this conversion expects. The less modification we apply to the incoming query the better.

+  // Extract any filter on node id to use in an optimization below
+  foreach ($pager_query->conditions() as $index => $condition) {
+    if ($index !== '#conjunction' && $condition['operator'] === '=' && ($condition['field'] === 'nid' || $condition['field'] === 'ws.nid')) {
+      $nid = $condition['value'];
+      break;
+    }
+  }

Could we just pass in the NID as an argument to webform_get_submissions_load() instead of doing this kind of messy extraction?

Overall I think this looks great.

Re #28.

You can never have enough semi-colons. ;)

I'll ask Liam to test with Postgresql. I think he uses it. From googling, it looks like joins on subqueries are okay.

I'll add some comments to the subquery maker. The only way around this that I can imagine would be to create a subclass of SelectQuery that uses an interface that adds fields and the subquery has been created. We'd make one for webform_get_submissions and one for download. Doesn't seem much better. And I'd venture that if it doesn't work with our current code, it wouldn't work with the OOP version either. This isn't a general purpose function. We are the only ones calling it, and the query needs to be of the right sort. My goal wasn't to extend webform, but rather to solve the problem of querying on big arrays of sids with an IN clause.

I think it is better and safer to infer the nid from the query. This way, the caller never gets it wrong and creates a silent performance problem.

Re: Coding Standards, huh, I stand corrected. It appears function summaries used to be allowed to go over 80 chars, not any more.

Re: White space, probably the easiest way to see them is to review the patch in Dreditor. Git also complains about whitespace errors when applying the patch (as noted by @pixlkat), though that probably depends on your whitespace settings in .gitignore.

@quicksketch -- I think I misunderstood the gist of your comment re the join. I did some testing and, at least with the queries we are generating, there isn't any harm in leaving the fields in the query. This was actually left over from when I was using an IN clause (as originally suggested in the related performance issue). Accordingly, I have deleted the field-deleting code. In my testing, it adds about 10% execution time to the query, but that time is swamped by the processing of the results of the query (e.g. downloading the data).

I'm pretty confident that any SQL can implement a join on a subquery. That's pretty basic stuff. Not too worried about postgres and ms sql, but if we have to, we can always add an engine test to using the join.

@fenstrat -- Thanks. I added a couple of whitespace-related options to my .gitconfig. Still, it's odd that KomodoIDE didn't clean up the changed lines. I cleaned the two related files, and re-wrapped comments.

I found a bug due to confusion over fields qualified with table aliases and not. I changed webform_get_submissions_query() to start by qualifying all the filter column names.

@LiamMorland -- please review this patch for postgres compatibility.

Committed #30 to 7.x-4.x and 8.x.

Note that currently d.o is broken and doesn't generate commit comments for the 7.x-4.x branch. Rest assured, it was committed and pushed.

If further testing reveals database engine regression, please re-open with details and the name of the offending database engine.

Great work here Dan, thanks.

When I tried exporting form results with a PostgreSQL backend, I got the following error:

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /~lkmorlan/drupal-7/?q=batch&id=4&op=do StatusText: Service unavailable (with message) ResponseText: PDOException: SQLSTATE[42702]: Ambiguous column: 7 ERROR: ORDER BY "sid" is ambiguous LINE 6: ORDER BY sid ASC ^: SELECT ws.*, sid AS sid, u.name AS name FROM {webform_submissions} ws LEFT OUTER JOIN {users} u ON u.uid = ws.uid WHERE (nid = :db_condition_placeholder_0) ORDER BY sid ASC LIMIT 5000 OFFSET 0; Array ( [:db_condition_placeholder_0] => 1 ) in webform_get_submissions_load() (line 836 of .../sites/all/modules/webform/includes/webform.submissions.inc).

The attached patch fixed the issue.

Tested on MySQL. Committed to 7.x-4.x and 8.x. Thanks, Liam Morland.