Opened 12 years ago

Closed 12 years ago

Last modified 10 years ago

#17062 closed Bug (fixed)

Using postgres, if you rollback the first transaction on a connection, the effect of settings.TIME_ZONE is lost

Reported by: Michael Hudson-Doyle Owned by: Aymeric Augustin
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: Michael Hudson-Doyle Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The fix for #3459 means that SET TIME ZONE is only called once per connection. But as per http://www.postgresql.org/docs/8.3/static/sql-set.html:

If SET (or equivalently SET SESSION) is issued within a transaction that is later aborted, the effects of the SET command disappear when the transaction is rolled back.

So if you're unlucky enough for the first transaction on a connect to get rolled back, the effect of settings.TIME_ZONE is lost for the life time of that connection -- which could quite possibly be lifetime of the process.

A simple fix would be to always COMMIT after setting the timezone -- I don't know if that would cause other problems though.

Attachments (1)

17062.diff (4.6 KB ) - added by Anssi Kääriäinen 12 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 by Michael Hudson-Doyle, 12 years ago

Cc: Michael Hudson-Doyle added

comment:2 by Alex Gaynor, 12 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Anssi Kääriäinen, 12 years ago

Has patch: set

Attached my attempt at fixing this issue. The fix I used is some code restructuring in postgresql_psycopg2.DatabaseWrapper. I am running the SET TIME ZONE in autocommit mode and setting the real requested isolation level only after that. connection_created is send only after the SET TIME ZONE is ran.

There is a test demonstrating the problem. It might be a little ugly, though...

comment:4 by Aymeric Augustin, 12 years ago

Owner: changed from nobody to Aymeric Augustin

comment:5 by Aymeric Augustin, 12 years ago

This patch is conflicting with my time zone support branch -- I'll deal with this ticket just after I merge the branch.

Last edited 12 years ago by Aymeric Augustin (previous) (diff)

comment:6 by Aymeric Augustin, 12 years ago

Interestingly, #3459 mentions this problem and contains a patch to fix it, but that patch wasn't committed.

comment:7 by Anssi Kääriäinen, 12 years ago

That patch in #3459 reminds me of another point: why do we use the integers 0 and 1 in set_isolation_level? It would be much better to use the constants mentioned in psycopg2 documentation: http://initd.org/psycopg/docs/extensions.html#transaction-status-constants. Maybe not this tickets problem, though.

I found it very useful to set log_statements to all in postgresql.conf (you can probably set that per user, also). That way I could see what queries psycopg2 actually generated, making it easier to minimize the amount of statements created in connect.

There might be some point in memoizing the databases default time zone (in the same way we memoize the db version) and not doing the set for every connection if the default is already the correct one. Or perhaps you could pass the default time zone in connection parameters? Again, maybe not this ticket's problem, and maybe over-optimization, too.

comment:8 by Aymeric Augustin, 12 years ago

akaariai, is there a reason why your patch changes the isolation level rather than just calling transaction.commit() after the SET TIME ZONE query?

FYI, I just committed a patch to use the constants for isolation levels (r17112).

Optimizing the case when settings.TIME_ZONE is PostgreSQL's default time zone could save 1 query per request (if I understand correctly). I suppose we'd check the server timezone with get_parameter_status. Would you mind opening a separate ticket for this?

comment:9 by Anssi Kääriäinen, 12 years ago

The reason of not doing a commit, but instead doing the autocommit -> real isolation level dance is saving a db roundtrip. I use my patch attached to this ticket (which doesn't apply any more) as an example. The patched one shows this in postgresql logs:

26639: LOG:  connection authorized: user=akaj database=testdb1
26639: LOG:  statement: SHOW default_transaction_isolation
26639: LOG:  statement: SET default_transaction_isolation TO DEFAULT
26639: LOG:  statement: SET TIME ZONE 'America/Chicago'
26639: LOG:  statement: SET default_transaction_isolation TO 'read uncommitted'
26639: LOG:  statement: BEGIN
26639: LOG:  statement: first real query...

And if you change it so that isolation level is set to real isolation level before the set, and commit is done after the SET TIME ZONE, you get this:

26731: LOG:  connection authorized: user=akaj database=testdb1
26731: LOG:  statement: SHOW default_transaction_isolation
26731: LOG:  statement: SET default_transaction_isolation TO 'read uncommitted'
26731: LOG:  statement: BEGIN
26731: LOG:  statement: SET TIME ZONE 'America/Chicago'
26731: LOG:  statement: COMMIT
26731: LOG:  statement: BEGIN
26731: LOG:  statement: first real query...

Which is one roundtrip more. Also, BEGIN / COMMIT are probably more expensive than the SHOW and SET commands (not tested). This is not really important, but on the other hand the cost in code isn't that great, either.

That get_parameter_status idea is a good one. Didn't know it existed. With get_parameter_status you can save actually two queries, resetting the isolation level, and the setting of time zone. I will create a ticket for this... But, if you have correct settings in the postgresql.conf (or for the user), get_parameter_status + the isolation level constants reduce the connection creation queries to this:

27163: LOG:  connection authorized: user=akaj database=testdb1
27163: LOG:  statement: SHOW default_transaction_isolation
27163: LOG:  statement: BEGIN
27163: LOG:  statement: first real query

Not bad :)

Last edited 12 years ago by Anssi Kääriäinen (previous) (diff)

comment:10 by Anssi Kääriäinen, 12 years ago

I figured it would be useful to post the work I did for the examples above into this ticket. I know this is not assigned to me, but as I have the code lying around it seems a bit silly not to post it here...

by Anssi Kääriäinen, 12 years ago

Attachment: 17062.diff added

comment:11 by Anssi Kääriäinen, 12 years ago

The ticket about get_parameter_status is #17266. This ticket will need to be resolved before working on that ticket.

comment:12 by Aymeric Augustin, 12 years ago

Resolution: fixed
Status: newclosed

In [17128]:

Fixed #17062 -- Ensured that the effect of SET TIME ZONE isn't lost when the first transation is rolled back under PostgreSQL. Thanks Anssi for the patch.

comment:13 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

In 1c24096f1e860960fa44df937713528065b5f79a:

Fixed a test isolation issue. Refs #17062.

This test could change settings.DATABASESdefaultTIME_ZONE and
didn't restore the previous value.

comment:14 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

In 1afe7488322f9b53ea00f47fb07c4374a5f2150e:

Fixed #21452 -- Non-autocommit connections to PostgreSQL.

When settings.DATABASESdefaultAUTOCOMMIT = False, the connection
wasn't in autocommit mode but Django pretended it was.

Thanks Anssi for analysing this issue.

Refs #17062.

comment:15 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

In b79bf9c7a9c1bc0e3cf0c5c4b0d8191a3033530b:

[1.6.x] Fixed a test isolation issue. Refs #17062.

This test could change settings.DATABASESdefaultTIME_ZONE and
didn't restore the previous value.

Backport of 1c24096f from master.

comment:16 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

In cb4a000adbb750da62daab318a9f4da8e3d4fdcd:

[1.6.x] Fixed #21452 -- Non-autocommit connections to PostgreSQL.

When settings.DATABASESdefaultAUTOCOMMIT = False, the connection
wasn't in autocommit mode but Django pretended it was.

Thanks Anssi for analysing this issue.

Refs #17062.

Backport of 1afe7488 from master

Note: See TracTickets for help on using tickets.
Back to Top