PostgreSQL string quoting type detection spams error logs

[Stuart Bishop]

With the PostgreSQL backend, Storm detects what sort of string quoting mechanism to use by issuing a test query that possibly fails and issuing a rollback. This generates false error messages in PostgreSQL logs.

2009-01-28 14:43:27 ICT LOG: statement: SHOW server_version
2009-01-28 14:43:27 ICT LOG: statement: SELECT EE''
2009-01-28 14:43:27 ICT ERROR: type "ee" does not exist at character 8
2009-01-28 14:43:27 ICT STATEMENT: SELECT EE''
2009-01-28 14:43:27 ICT LOG: statement: ROLLBACK

We should be able to detect the quoting mechanism to use better, and make DBAs and their log analysis and fault monitoring systems happier.

[James Henstridge (jamesh)]

I think current versions of psycopg2 handle this properly, so one fix would be to add a minimum version requirement for psycopg2 to Storm and remove the workaround code.

Given the number of bugs in ancient versions of psycopg2, this is probably a good idea anyway.

[Gustavo Niemeyer (niemeyer)]

Considering that this change is somwhat recent, having a minimum version of psycopg2 could potentially exclude most of Storm's users. I'm not entirely sure even if we have a recent enough version.

That said, if we know the exact version in which this changed, we can use the version to conditionally introduce the fix based on the server version and on the psycopg2 version.

[Stuart Bishop (stub)]

On Thu, Jan 29, 2009 at 8:28 PM, Gustavo Niemeyer <email address hidden> wrote:
> Considering that this change is somwhat recent, having a minimum version
> of psycopg2 could potentially exclude most of Storm's users. I'm not
> entirely sure even if we have a recent enough version.
>
> That said, if we know the exact version in which this changed, we can
> use the version to conditionally introduce the fix based on the server
> version and on the psycopg2 version.

We know it works correctly with 2.0.8, so we can just do 2.0.8 or greater. It can be adjusted down later if people learn more and care enough.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

[James Henstridge (jamesh)]

The E'' quote stuff was fixed in psycopg2 2.0.7 (released a bit over 9 months ago). If you are using an older version than that, then you also have broken error handling for commit. Not knowing whether commits succeed or fail is a pretty big problem if you care about your data ...

If the E'' quoting was the only important fix in the later versions of psycopg2 I might agree with you, but the other fixes are important enough that I'd be worried about anyone running on older versions.

If Landscape is still running on an ancient version, you should definitely look at upgrading. The IS team already has 2.0.8 installed for Launchpad and Online Services, so it shouldn't be a big deal installing it for Landscape too.

[Stuart Bishop (stub)]

On Sat, Jan 31, 2009 at 9:50 AM, James Henstridge
<email address hidden> wrote:
> The E'' quote stuff was fixed in psycopg2 2.0.7 (released a bit over 9
> months ago). If you are using an older version than that, then you also
> have broken error handling for commit. Not knowing whether commits
> succeed or fail is a pretty big problem if you care about your data ...
>
> If the E'' quoting was the only important fix in the later versions of
> psycopg2 I might agree with you, but the other fixes are important
> enough that I'd be worried about anyone running on older versions.
>
> If Landscape is still running on an ancient version, you should
> definitely look at upgrading. The IS team already has 2.0.8 installed
> for Launchpad and Online Services, so it shouldn't be a big deal
> installing it for Landscape too.

So the fix for Storm is to require psycopg2 2.0.7+ and remove the
detection code? That suites me fine, and protects Storm and its users
from known data loss bugs.

I'm happy to submit the patch if this is the way forward.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

[Gustavo Niemeyer (niemeyer)]

Considering what James just explained, sounds like a good idea.

[Stuart Bishop (stub)]

Merge proposal up and ready for review.

[James Henstridge (jamesh)]

Fix merged in r311