Merge lp:~danilo/launchpad/bug-408718 into lp:launchpad

= Bug #408717 =

Note: this is a cherrypick candidate so is kept as simple as possible.
It's critical for Launchpad Translations (poimport is blocked on this).

We had super-fast-imports query (which is a huge optimization for our
imports, resulting in 50x faster imports on average) cause Postgres to
eat up diskspace with repeated requests for more temp space. That
happened a month ago (when bug #408717 was filed and considered a
one-off), and it happened again on Friday morning. We all suspect a
Postgres bug, but that doesn't help us out.

However, since this is only an optimization, we can avoid this query
when we do hit a problem. So, how do we know when we'll hit a problem?

Simple: a timeout. 5 minute timeout (300s) should be enough to handle
all real query runs which do not exhibit the bad behaviour. If this
query takes more than 5 minutes, it's probably hitting a PG bug, so
let's kill it. Note that PG was completely operational when this
happened, so it should be able to fulfill this as well.

Note that exactly the same query that failed when it was ran manually
caused no issues: so, it's not a particular query that fails, it's a
particular set of circumstances that's out of our control (i.e.
something like a PG bug) that make the query fail.

= Pre-implementation notes =

Discussed this with Stuart: he thinks it's a great idea :)

He advised me about what I need to do to set the statement timeout and
how to best test it.

= Proposed fix =

So, introduce a statement timeout right around the potentially breaking
query. If it doesn't complete in the allotted time, kill it and go the
slow route for this particular import.

poimport.statement_timeout option allows one to set it to 'timeout'
which will trigger the timeout on purpose. This is used in a test.

= Tests =

bin/test -vvt poimport -t superfastimports

= Demo & QA =

Test poimport thoroughly.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 14.09.2009 11:35, Данило Шеган schrieb:
> = Bug #408717 =
>
> Note: this is a cherrypick candidate so is kept as simple as possible.
> It's critical for Launchpad Translations (poimport is blocked on this).

Thanks for taking this on so quickly and finding a solution. If Stuart
thinks it's a great idea, who am I to argue? ;-)

 review approve

Just two comments below.

> === modified file 'lib/canonical/config/schema-lazr.conf'
> --- lib/canonical/config/schema-lazr.conf 2009-09-08 15:16:33 +0000
> +++ lib/canonical/config/schema-lazr.conf 2009-09-14 09:12:58 +0000
> @@ -1429,6 +1429,10 @@
> storm_cache: generational
> storm_cache_size: 500
>
> +# Statement timeout (in seconds), limited to super-fast-imports query.
> +# Set to 'timeout' to make it timeout every time (for tests).
> +statement_timeout: 300
> +
> [processmail]
> # The database user which will be used by this process.
> # datatype: string
>
> === modified file 'lib/lp/translations/utilities/tests/test_superfastimports.py'
> --- lib/lp/translations/utilities/tests/test_superfastimports.py 2009-07-17 02:25:09 +0000
> +++ lib/lp/translations/utilities/tests/test_superfastimports.py 2009-09-14 09:12:58 +0000
> @@ -5,8 +5,9 @@
>
> import unittest
>
> +from canonical.config import config
> from canonical.testing import ZopelessDatabaseLayer
> -from lp.testing import TestCaseWithFactory, verifyObject

Thanks for cleaning this.

> +from lp.testing import TestCaseWithFactory
> from lp.translations.utilities.translation_import import (
> ExistingPOFileInDatabase)
> from lp.translations.utilities.translation_common_format import (
> @@ -67,6 +68,27 @@
> self.assertFalse(cached_file.isAlreadyImportedTheSame(message_data))
> self.assertFalse(cached_file.isAlreadyTranslatedTheSame(message_data))
>
> + def test_query_timeout(self):
> + # Test that super-fast-imports doesn't cache anything when it hits
> + # a timeout.
> +
> + # Override the config option to force a timeout error.
> + new_config = ("""
> + [poimport]
> + statement_timeout: timeout
> + """)
> + config.push('super_fast_timeout', new_config)
> +
> + # Add a message that would otherwise be cached (see other tests).
> + current_message = self.factory.makeTranslationMessage(
> + pofile=self.pofile, is_imported=False)
> + message_data = self.getTranslationMessageData(current_message)
> + cached_file = ExistingPOFileInDatabase(self.pofile)
> + self.assertFalse(cached_file.isAlreadyTranslatedTheSame(message_data))
> +
> + # Restore the old configuration.
> + config.pop('super_fast_timeout')
> +
>
> def test_suite():
> return unittest.TestLoader().loadTestsFromName(__name__)
>
> === modified file 'lib/lp/translations/utilities/translation_import.py'
> --- lib/lp/translations/utilities/translation_import.py 2009-08-05 13:11:00 +0000
> +++ lib/lp/translations/utilities/translation_import.py 2009-09-14 09:12:58 +0000
> @@ -18,8 +18,13 @@
>
> from operator import attrgetter
>
> +import transaction
> +
> +from...

Hi Henning,

Thanks for the review. For documentation purposes, I am writing up what
we've discussed on IRC as well.

У пон, 14. 09 2009. у 10:24 +0000, Henning Eggers пише:
> > + except TimeoutError:
> > + # Restart the transaction and return empty SelectResults.
> > + transaction.abort()
> > + transaction.begin()
> > + cur.execute("SELECT 1 WHERE 1=0")
>
> As you mentioned on IRC, you should reset the statement_timeout back to
> what it was (if that is not done by abort() ?). Please extend the test
> to check that, too.

I've decided not to (and I don't know if transaction.abort() resets
statement_timeout value): this script run will finish in at most 10
minutes, so having a safeguard while it runs is not too bad (considering
how we just almost killed the DB server).

> Also, could you consider using EmptyResultSet instead of such useless
> queries, please? ;)

As discussed, this is actually using SQLObject, and since I am going to
ask for a CP, I am not converting this query to Storm. With
EmptyResultSet the following would not be possible:

> > rows = cur.fetchall()

And I'd have to worry a bit more about breaking something else.

Cheers,
Danilo