Code

Opened 3 years ago

Closed 2 years ago

Last modified 3 months 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: mwhudson Owned by: aaugustin
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: mwhudson 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 akaariai 2 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 3 years ago by mwhudson

  • Cc mwhudson added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 2 years ago by akaariai

  • 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 Changed 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:5 Changed 2 years ago by aaugustin

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

Last edited 2 years ago by aaugustin (previous) (diff)

comment:6 Changed 2 years ago by aaugustin

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

comment:7 Changed 2 years ago by akaariai

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 Changed 2 years ago by aaugustin

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 Changed 2 years ago by akaariai

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 2 years ago by akaariai (previous) (diff)

comment:10 Changed 2 years ago by akaariai

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...

Changed 2 years ago by akaariai

comment:11 Changed 2 years ago by akaariai

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

comment:12 Changed 2 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from new to closed

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 Changed 3 months ago by Aymeric Augustin <aymeric.augustin@…>

In 1c24096f1e860960fa44df937713528065b5f79a:

Fixed a test isolation issue. Refs #17062.

This test could change settings.DATABASESdefault?TIME_ZONE? and
didn't restore the previous value.

comment:14 Changed 3 months ago by Aymeric Augustin <aymeric.augustin@…>

In 1afe7488322f9b53ea00f47fb07c4374a5f2150e:

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

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

Thanks Anssi for analysing this issue.

Refs #17062.

comment:15 Changed 3 months ago by Aymeric Augustin <aymeric.augustin@…>

In b79bf9c7a9c1bc0e3cf0c5c4b0d8191a3033530b:

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

This test could change settings.DATABASESdefault?TIME_ZONE? and
didn't restore the previous value.

Backport of 1c24096f from master.

comment:16 Changed 3 months ago by Aymeric Augustin <aymeric.augustin@…>

In cb4a000adbb750da62daab318a9f4da8e3d4fdcd:

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

When settings.DATABASESdefault?AUTOCOMMIT? = 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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.