Code review comment for lp:~stub/launchpad/garbo

The query to search for orphans takes 10 minutes cold. I've opened Bug #723596, cited in an XXX comment stating that this should be run less frequently.

Yes, it sucks needing the procedure to use FETCH. It seems impossible to use FETCH inside a SELECT statement without it, and WITH clauses don't help as they contain SELECT statements. It is interesting that the only way of declaring a CURSOR WITH HOLD is using the DECLARE SQLstatement, as the cursors declared using the plpgsql DECLARE statement implicitly close at the end of the transaction. So as far as I can tell, this is the only possible structure to do what we are doing. Thankfully, this is all in the base class so if we work out a better approach there is just one piece of code to change :)

I agree CLOSE ALL doesn't belong in the constructor. I didn't want to add a cleanup method to ILoopTuner (and all the implementations) though, esp as it will be rare to need the cleanup method. I considered a check adapting the looptuner instance to ICleanup (does not yet exist) and invoking that, but thought that was overkill for this case. From IRC discussion, I've added an optional cleanUp method to ITunableLoop and used it.

The DELETE could be rewritten to use USING, but it is less readable (table alias needed and 4 substitutions vs. the current 3) and have nearly identical query plans (IN has a 'Hash Semi Join' as the top node, USING has a 'Hash Join' as the top node. USING actually has an infinitesimally larger cost. Go figure :)

The BulkPruner tests no longer use the Launchpad database. They now use the ZopelessDatabaseLayer.

I don't think your suggested test structure is an improvement. It seems redundant, as the existing test would still be needed to ensure things like commit-per-iteration, only removing chunk items per iteration. We are testing the object implements the loop tuner interface correctly, and the existing loop tuner tests confirm that this interface is invoked correctly (which is lacking in the tests for the other garbo tasks - bogus implementations that don't deal with the chunk size correctly can still pass).