[HttpFoundation] Precalculate expiry timestamp #14625

dosten · 2015-05-13T02:51:45Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	#13955
License	MIT
Doc PR	-

xabbuh · 2015-05-14T09:25:08Z

src/Symfony/Component/HttpFoundation/CHANGELOG.md

+3.0.0
+-----
+
+ * PdoSessionHandler: removed the `lifetime` and `timestamp` columns in favor of `expiry` column


This would need to be added to the update file.

I've added more info in the UPGRADE-3.0.md file, see above.

Tobion · 2015-05-14T10:42:12Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

                );
                $insertStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR);
                $insertStmt->bindValue(':data', '', \PDO::PARAM_LOB);
-                $insertStmt->bindValue(':lifetime', 0, \PDO::PARAM_INT);
-                $insertStmt->bindValue(':time', time(), \PDO::PARAM_INT);
+                $insertStmt->bindValue(':expiry', time(), \PDO::PARAM_INT);


you can just put 0 here since this is only dummy data that will be updated by the write operation later anyway

dosten · 2015-05-14T14:39:40Z

@Tobion done

Tobion · 2015-05-14T22:54:28Z

UPGRADE-3.0.md

+
+   - Rename the `sess_time` to `sess_expiry`. In MySQL this would be:
+      ```
+      ALTER TABLE `session` CHANGE `sess_time` `sess_expiry` INTEGER UNSIGNED NOT NULL;


This will result in wrong expiry time.

Yes, maybe the steps must be:

Create a new sess_expiry column.

Migrate the old data: UPDATE session SET sess_expiry = sess_time + sess_lifetime;

Remove the sess_time and sess_lifetime columns.

What do you think?

ping @Tobion

dosten · 2015-05-23T03:02:53Z

@Tobion updated the UPGRADE-3.0.md file.

Tobion · 2015-05-23T12:54:35Z

👍

Tobion · 2015-05-29T12:21:43Z

ping @symfony/deciders

xabbuh · 2015-05-30T21:33:14Z

Can we in a way provide a smoother upgrade path (ie. by offering a deprecation message in Symfony 2.8)?

weaverryan · 2015-05-30T22:39:49Z

I agree. I think we should create a new class for this and deprecate the old class. We already messed this up in 2.6 with the new PdoSessionHandler and then had to add the LegacyPdoSessionHandler quickly after to help ease the upgrade path. That - like everything else we do - will give them at least on version where they can migrate from the old to the new. If we did that, this would be merged into 2.8, which again, is consistent with all our other features (no new features in only 3.0).

Tobion · 2015-05-30T22:45:24Z

I agree is would be nicer. But how would we name the new PdoSessionHandler? If the name sucks, we have to change the name again in 3.0 which will not make the upgrade easier.

weaverryan · 2015-05-30T23:11:48Z

@Tobion ha, so true - we don't want something like TheNewPdoSessionHandler :).

And I'm really not sure - I think about this question a lot, but I don't know a good, global answer. How about PdoExpirySessionHandler in this case? I realize it's a slippery slope (would future changes make an ever-longer name?), but I don't think we can plan for that, and we need some way to introduce this improvement in 2.8 without the BC break.

GromNaN · 2015-05-31T08:10:55Z

Is there a necessity of removing the sess_time and sess_lifetime columns ? I don't think so.

Since this sess_expiry column needs to be added for performance reasons on "large production websites", it's a pity that we have to break session databases this way.

For a smoother migration path, I would create a feature flag to enable the extra sess_expiry column.
sess_time and sess_lifetime columns get always filled and you can still enable/disable the feature anytime and re-run the SQL query to populate the sess_expiry column.

dosten · 2015-06-17T01:34:18Z

IMO the only factible way to introduce this in 2.8 is the idea proposed by @GromNaN. What do you think? If all agree, i can make the necessary changes.

weaverryan · 2015-06-19T16:31:02Z

I think @GromNaN's idea would certainly work, though we'd need to have a lot of if statements inside the class to use different queries. But we could deprecate the old way and remove it in 3.0 (meaning only 1 version would need to have all these ugly if statements). But yea, I would like to fix this - other than the BC part, it sounds like a great win!

dosten · 2015-06-19T16:40:45Z

Ok, i will work on this.

weaverryan · 2015-06-19T16:48:15Z

@dosten you rock :)

Tobion · 2015-06-19T17:58:53Z

I think the index on the expiry column is really necessary for high loads: #15020 (comment)

dosten · 2015-06-19T18:04:53Z

I'm not a database expert, if someone knows how to create an index in mysql, sqlite, pgsql, oci or sqlsrv.. help is welcome :)

GromNaN · 2015-06-19T18:39:27Z

@dosten the syntax is SQL standard : https://dev.mysql.com/doc/refman/5.0/en/create-index.html

CREATE INDEX idx_sess_expiry ON session (sess_expiry);

dosten · 2015-06-28T02:09:24Z

Closing this because i'm going to create another PR over the 2.8 branch.

GuilhemN · 2016-02-01T17:35:03Z

no news about this?

Removed timestamp and lifetime columns in favor of expiry column. Based on the work done in symfony#14625. Fix symfony#13955.

Removed timestamp and lifetime columns in favor of expiry column. Based on the work done in symfony#14625. Fix symfony#13955. Implemented CR suggestions from nicolas-grekas

… (azjezz) This PR was merged into the 4.4 branch. Discussion ---------- [HttpFoundation] Precalculate session expiry timestamp | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #13955, #21423, #14625 | License | MIT Continued work from the original PR #21423 / #21857 Commits ------- 066000a [HttpFoundation] Precalculate session expiry timestamp

xabbuh reviewed May 14, 2015
View reviewed changes

xabbuh added the HttpFoundation label May 14, 2015

Tobion reviewed May 14, 2015
View reviewed changes

Removed timestamp and lifetime columns in favor of expiry column

9b64cb9

Tobion added the Ready label May 23, 2015

dosten closed this Jun 28, 2015

dosten deleted the precalculate-expiry-timestamp branch November 7, 2015 03:20

bcremer mentioned this pull request Jan 25, 2017

[Session][3.0] PdoSessionHandler: precalculate expiry timestamp #13955

Closed

bcremer added a commit to bcremer/symfony that referenced this pull request Jan 26, 2017

[HttpFoundation] Precalculate expiry timestamp

5780201

Removed timestamp and lifetime columns in favor of expiry column. Based on the work done in symfony#14625. Fix symfony#13955.

bcremer mentioned this pull request Jan 26, 2017

[HttpFoundation] Precalculate expiry timestamp #21423

Closed

robfrawley mentioned this pull request Mar 3, 2017

[HttpFoundation] Precalculate session expiry timestamp #21857

Closed

6 tasks

azjezz mentioned this pull request Aug 14, 2019

[HttpFoundation] Precalculate session expiry timestamp #33169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpFoundation] Precalculate expiry timestamp #14625

[HttpFoundation] Precalculate expiry timestamp #14625

dosten commented May 13, 2015

xabbuh May 14, 2015

dosten May 14, 2015

Tobion May 14, 2015

dosten commented May 14, 2015

Tobion May 14, 2015

dosten May 14, 2015

dosten May 22, 2015

Tobion May 22, 2015

dosten commented May 23, 2015

Tobion commented May 23, 2015

Tobion commented May 29, 2015

xabbuh commented May 30, 2015

weaverryan commented May 30, 2015

Tobion commented May 30, 2015

weaverryan commented May 30, 2015

GromNaN commented May 31, 2015

dosten commented Jun 17, 2015

weaverryan commented Jun 19, 2015

dosten commented Jun 19, 2015

weaverryan commented Jun 19, 2015

Tobion commented Jun 19, 2015

dosten commented Jun 19, 2015

GromNaN commented Jun 19, 2015

dosten commented Jun 28, 2015

GuilhemN commented Feb 1, 2016

[HttpFoundation] Precalculate expiry timestamp #14625

[HttpFoundation] Precalculate expiry timestamp #14625

Conversation

dosten commented May 13, 2015

xabbuh May 14, 2015

Choose a reason for hiding this comment

dosten May 14, 2015

Choose a reason for hiding this comment

Tobion May 14, 2015

Choose a reason for hiding this comment

dosten commented May 14, 2015

Tobion May 14, 2015

Choose a reason for hiding this comment

dosten May 14, 2015

Choose a reason for hiding this comment

dosten May 22, 2015

Choose a reason for hiding this comment

Tobion May 22, 2015

Choose a reason for hiding this comment

dosten commented May 23, 2015

Tobion commented May 23, 2015

Tobion commented May 29, 2015

xabbuh commented May 30, 2015

weaverryan commented May 30, 2015

Tobion commented May 30, 2015

weaverryan commented May 30, 2015

GromNaN commented May 31, 2015

dosten commented Jun 17, 2015

weaverryan commented Jun 19, 2015

dosten commented Jun 19, 2015

weaverryan commented Jun 19, 2015

Tobion commented Jun 19, 2015

dosten commented Jun 19, 2015

GromNaN commented Jun 19, 2015

dosten commented Jun 28, 2015

GuilhemN commented Feb 1, 2016