#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)
Change History (17)
comment:1 by , 13 years ago
| Cc: | added |
|---|
comment:2 by , 13 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:3 by , 13 years ago
| Has patch: | set |
|---|
comment:4 by , 13 years ago
| Owner: | changed from to |
|---|
comment:5 by , 13 years ago
This patch is conflicting with my time zone support branch -- I'll deal with this ticket just after I merge the branch.
comment:6 by , 13 years ago
Interestingly, #3459 mentions this problem and contains a patch to fix it, but that patch wasn't committed.
comment:7 by , 13 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 , 13 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 , 13 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 :)
comment:10 by , 13 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 , 13 years ago
| Attachment: | 17062.diff added |
|---|
comment:11 by , 13 years ago
The ticket about get_parameter_status is #17266. This ticket will need to be resolved before working on that ticket.
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...