[HttpFoundation] Precalculate session expiry timestamp #21857

robfrawley · 2017-03-03T17:27:13Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#13955, #21423, #14625
License	MIT
Doc PR	TBA

Continued work from the original PR #21423.

Todo:

Provide legacy tests
Fix code style (such as yoda style)
Add indexes to time column (only done for MySQL, SQLite, and PgSQL)
Re-implement some removed code for BC
Add docs to UPGRADE*.md files including SQL for changing to new behavior
Add deprecation/removal info to UPGRADE*.md files

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

robfrawley · 2017-03-04T18:39:59Z

I've added indexes to the new time column for MySQL, SQLite, and PgSQL, but I'm not sure how to do so with the oci or sqlsrv drivers. Here are the "create table" statements I have updated so far:

MySQL

$sql = "CREATE TABLE $this->table ($this->idCol VARBINARY(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->timeCol INTEGER UNSIGNED NOT NULL, KEY {$this->table}_{$this->timeCol}_idx ($this->timeCol)) COLLATE utf8_bin, ENGINE = InnoDB";

CREATE TABLE `sessions` (`sess_id` varbinary(128) NOT NULL, `sess_data` blob NOT NULL, `sess_time` int(10) unsigned NOT NULL, PRIMARY KEY (`sess_id`), KEY `sessions_sess_time_idx` (`sess_time`)) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin

SQLite

$sql = "BEGIN; CREATE TABLE $this->table ($this->idCol TEXT NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->timeCol INTEGER NOT NULL); CREATE INDEX {$this->table}_{$this->timeCol}_idx ON $this->table  ($this->timeCol); COMMIT";

BEGIN; CREATE TABLE sessions (sess_id TEXT NOT NULL PRIMARY KEY, sess_data BLOB NOT NULL, sess_time INTEGER NOT NULL); CREATE INDEX sessions_sess_time_idx ON sessions (sess_time); COMMIT;

PgSQL

$sql = "BEGIN; CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BYTEA NOT NULL, $this->timeCol INTEGER NOT NULL); CREATE INDEX {$this->table}_{$this->timeCol}_idx ON $this->table ($this->timeCol); COMMIT"

BEGIN; CREATE TABLE sessions (sess_id VARCHAR(128) NOT NULL PRIMARY KEY, sess_data BYTEA NOT NULL, sess_time INTEGER NOT NULL); CREATE INDEX sessions_sess_time_idx ON sessions (sess_time); COMMIT

Anyone have any insight into the proper syntax for the oci and sqlsrv drivers?

…indexes/revert removed code

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

xabbuh · 2017-03-04T19:15:33Z

UPGRADE-3.3.md

+HttpFoundation
+--------------
+
+ * The `PdoSessionHandler` option `db_lifetime_col` has been deprecated.


You should add an entry to the UPGRADE-4.0.md file talking about the removal of the column.

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

…_col to db_time_col

stof · 2017-03-30T15:30:01Z

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

            } else {
                $mergeStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR);
                $mergeStmt->bindParam(':data', $data, \PDO::PARAM_LOB);
-                $mergeStmt->bindParam(':lifetime', $maxlifetime, \PDO::PARAM_INT);


this removal looks wrong to me. You need to handle the legacy way too

@stof The legacy tests pass as-is. Should it still be added back with the same assignment as before (the current Unix time)?

same than @stof to me also

robfrawley · 2017-03-30T18:31:34Z

Calling out again to see if anyone has knowledge of and/or access to an Oracle (oci driver) or MSSQL Server (sqlsrv driver); we still need to add indexes to the CREATE TABLE statements for these drivers, as has been done for the mysql, sqlite, and pgsql drivers already.

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

nicolas-grekas · 2017-07-06T13:39:18Z

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

@@ -510,7 +546,7 @@ private function doRead($sessionId)
            $sessionRows = $selectStmt->fetchAll(\PDO::FETCH_NUM);

            if ($sessionRows) {
-                if ($sessionRows[0][1] + $sessionRows[0][2] < time()) {
+                if ($sessionRows[0][1] < time() || (isset($sessionRows[0][2]) && $sessionRows[0][1] + $sessionRows[0][2] < time())) {


I would base this check on a ternary check for false === $this->lifetimeCol, so that it's clearer and easier to spot that this needs to be changed in 4.0

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

nicolas-grekas · 2017-07-06T13:42:40Z

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);


should be conditional

nicolas-grekas · 2017-07-06T13:43:10Z

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

@@ -629,9 +671,9 @@ private function getSelectSql()
                case 'mysql':
                case 'oci':
                case 'pgsql':
-                    return "SELECT $this->dataCol, $this->lifetimeCol, $this->timeCol FROM $this->table WHERE $this->idCol = :id FOR UPDATE";
+                    return "SELECT $this->dataCol, $this->timeCol FROM $this->table WHERE $this->idCol = :id FOR UPDATE";


conditional also (we should fetch the column when false !== $this->lifetimeCol (same below)

nicolas-grekas · 2017-08-07T07:32:21Z

ping @robfrawley

robfrawley · 2017-08-10T05:01:01Z

#21857 (comment):

Calling out again to see if anyone has knowledge of and/or access to an Oracle (oci driver) or MSSQL Server (sqlsrv driver); we still need to add indexes to the CREATE TABLE statements for these drivers, as has been done for the mysql, sqlite, and pgsql drivers already.

Otherwise, I need to amend a few things per some comments/reviews, but the above is a blocker for me ATM, as I have no experience using the oci and sqlsrv drivers, and no access to these databases to test, either.

nicolas-grekas · 2017-09-26T12:44:53Z

@robfrawley is this really a blocker? I mean: if we are index-less in this iteration, it's fine to me, the code would still be better than today for many ppl, and not worse for oci/sqlsrv, isn't it? WOuld you have time to rebase/finish by the end of the week?

robfrawley · 2017-09-26T19:09:00Z

@nicolas-grekas If you don't consider it a blocker to only support indexes on select drivers, I'm happy to rebase and address any remaining comments. Doing so by the end of the week shouldn't be an issue, either.

nicolas-grekas · 2017-09-28T09:05:18Z

@robfrawley looking forward to it thanks :)

nicolas-grekas · 2017-10-08T15:39:13Z

ping @robfrawley, this might be moved to 4.1 very soon :)

nicolas-grekas · 2017-10-12T16:05:21Z

I'm moving this PR to 4.1, we're already late in feat freeze.

fabpot · 2018-03-31T05:33:16Z

@robfrawley Still interested in finishing this one? For the record, I agree with @nicolas-grekas about support for OCI and mssql.

robfrawley · 2018-04-04T16:38:19Z

@nicolas-grekas @fabpot Fair enough. I can definitely finish this up without index support for oci/sqlsrv. I'll take a look at the end of next week. Likely won't get to it sooner.

fabpot · 2018-10-10T12:21:37Z

Anyone willing to take over this one? It's almost finished AFAICR. /cc @symfony/deciders

fabpot · 2019-03-24T19:34:03Z

If nobody is willing to take over this one, I'm going to close it. Might be something we can work on in the upcoming Hackaton.

vargedo · 2019-07-22T06:39:54Z

+1 this would be great to be merged, we have some issues with this as well.

azjezz · 2019-08-14T13:11:27Z

i will take over this one, thanks for your work @robfrawley

nicolas-grekas · 2019-08-15T17:45:34Z

Continued in #33169
Thank you @robfrawley!

… (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

[HttpFoundation] Precalculate expiry timestamp to allow indexed lookups

a6a83b7

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

carsonbot added Status: Needs Review Bug Feature labels Mar 3, 2017

robfrawley changed the title ~~[WIP] Precalculate session expiry timestamp~~ [WIP] [HttpFoundation] Precalculate session expiry timestamp Mar 3, 2017

robfrawley force-pushed the feature-precalculate-session-expiry-timestamp branch from de28661 to 50e01b1 Compare March 3, 2017 17:28

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Mar 3, 2017

nicolas-grekas changed the title ~~[WIP] [HttpFoundation] Precalculate session expiry timestamp~~ [HttpFoundation] Precalculate session expiry timestamp Mar 3, 2017

nicolas-grekas added this to the 3.x milestone Mar 4, 2017

[HttpFoundation] introduce PDO session legacy tests/style fixes/time …

0fa0c5a

…indexes/revert removed code

robfrawley force-pushed the feature-precalculate-session-expiry-timestamp branch from 50e01b1 to 0fa0c5a Compare March 4, 2017 18:48

robfrawley commented Mar 4, 2017

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php Show resolved Hide resolved

robfrawley commented Mar 4, 2017

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php Show resolved Hide resolved

robfrawley mentioned this pull request Mar 4, 2017

[HttpFoundation] Precalculate expiry timestamp #21423

Closed

robfrawley commented Mar 4, 2017

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php Show resolved Hide resolved

xabbuh reviewed Mar 4, 2017

View reviewed changes

robfrawley commented Mar 4, 2017

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php Show resolved Hide resolved

[HttpFoundation] updated changelogs descibing change from db_lifetime…

d1af40a

…_col to db_time_col

nicolas-grekas added Deprecation and removed Bug labels Mar 7, 2017

stof requested changes Mar 30, 2017

View reviewed changes

nicolas-grekas self-requested a review April 8, 2017 18:53

robfrawley mentioned this pull request Jun 21, 2017

MySQL: Very Slow Session Handling #23243

Closed

nicolas-grekas reviewed Jul 6, 2017

View reviewed changes