Session table improvements #19708

csthomas · 2018-02-16T16:20:20Z

Summary of Changes

This PR provides an improvement in the performance of high-traffic sites.

Change type of column session_id from varchar to varbinary.
Change type of column time from varchar to integer.

Why I changed time column to integer?
Before this PR the value of time column is stored as text.
Comparing text to text such as "a" <'b' can not use the index key.

When joomla deletes all expired sessions then need to scan whole table because the index (of time column) is useless.

Drawback of varchar:

varchar type use language processing on comparison, it is slower
varchar takes up to 4 bytes per character when it is loaded into RAM
when we compare strings like '21' < '110' then the result returns false (instead true), see Can't login after setting lifetime to 15000000 #13933

Why I changed session_id column to varbinary?
A good explanation is at https://github.com/symfony/http-foundation/blob/master/Session/Storage/Handler/PdoSessionHandler.php#L214

Testing Instructions

Install joomla 3.8.6 or newer
Login and apply this PR 19708, for example by com_patchtester. ~~For PostgreSQL database you should apply additional PR Repair the update of database schema changes on postgreSQL #19707~~ - it is merged.
Go to backend -> extension -> manage -> database -> and click on fix button.
Everything should go without errors. You should be still logged in.
Your session works as before. You can login/logout without any problems.

Expected result

All works as before.

Quy · 2018-02-16T21:48:45Z

libraries/joomla/session/storage/database.php

@@ -76,7 +76,7 @@ public function write($id, $data)
 			$query = $db->getQuery(true)
 				->update($db->quoteName('#__session'))
 				->set($db->quoteName('data') . ' = ' . $db->quote($data))
-				->set($db->quoteName('time') . ' = ' . $db->quote((int) time()))
+				->set($db->quoteName('time') . ' = ' . (int) time())


time() returns an integer. Do we need (int)?

I will remove it.

I'm not sure I should do this as in php5 there is an extension apd, pecl install apd and there is a function override_function(), which can change time() function to something else.
For now, I leave it as it is.

By that argument you can't rely on any of the core PHP functions to do as they document. Should someone be running one of those types of override tools or custom compiling PHP and changing C level internals, it's a problem for them and it's not something we can account for at our level without not using the language features.

alikon · 2018-02-17T10:59:31Z

we should take care of the update path, of course, but a good move

joeforjoomla · 2018-02-17T14:06:42Z

Is this something that may happen again switching to VARBINARY?
#9423

csthomas · 2018-02-17T16:05:00Z

@joeforjoomla

I do not know yet.
If someone use join to session table using session_id column then it would be good to check that.

mbabker · 2018-02-17T16:07:48Z

If someone use join to session table using session_id column then it would be good to check that.

There are people joining the session table on that column for reasons I do not comprehend (I didn't in 2015 and I don't in 2018, I have not found one legitimate use case where my database schema needs a FK to a table storing the session identifier).

joeforjoomla · 2018-02-17T16:11:32Z

A common use case to join the session table is for example when dealing with a chat extension allowing communication between guest users.

csthomas · 2018-02-17T16:46:33Z

I did simple tests with success:

SELECT s.session_id, old.session_id FROM `j38_session` s INNER JOIN `j38_session_old` old ON s.`session_id` = old.`session_id`;
SELECT * FROM `j38_session` WHERE session_id = '4m82kgvd3nr49s3melhnmejqlg';

joeforjoomla · 2018-02-17T18:12:24Z

I did tests too, using MySQL 5.1, 5.5 and 5.7
In all cases with success, no errors are thrown because of the different field type/charset/collation so defintely there should not be any problem.

Quy · 2018-02-17T20:54:08Z

@joeforjoomla Please mark your test at Issue Tracker. Thanks.

csthomas · 2018-02-17T21:40:08Z

It is too early to mark the test as a success.

This PR probably has to wait until #19687 will be merged.

Quy · 2018-02-20T00:03:38Z

installation/sql/mysql/joomla.sql

  `client_id` tinyint(3) unsigned DEFAULT NULL,
-  `guest` tinyint(4) unsigned DEFAULT 1,
-  `time` varchar(14) DEFAULT '',
+  `guest` tinyint(3) unsigned DEFAULT 1,


Should not be changed from 4 to 3 since it is not done during updates?

Quy · 2018-02-23T16:40:48Z

@csthomas #19750 mentioned adding an index on guest column. Would that be another improvement to consider?

csthomas · 2018-02-23T16:52:00Z

Yes, we can probably use the userid column instead of using guest. Column userid is already indexed.
If there is a relation like: if guest==0 then userid!=0 and if guest==1 then userid==0 then it should be ok.

* Add missing alter table for guest * Remove cast to int for the result of time()

csthomas · 2018-02-26T23:41:16Z

I updated the code. Now this PR is ready for testing.

csthomas · 2018-08-13T19:49:11Z

@joeforjoomla Could you re-test this PR.

csthomas · 2018-08-13T19:52:04Z

@BrainforgeUK Because you know the subject well, could you test it?

joeforjoomla · 2018-08-13T21:55:18Z

@csthomas I re-tested this PR again up to MySQL 8.0 without any issue when joining between #__session.session_id AS varinary(192) and #__customtable.session_id AS varchar(191)

However i can't ensure that everything will work correctly on other RDBMS such as MariaDB, Postgresql, etc

csthomas · 2018-08-14T09:04:59Z

@joeforjoomla thanks,

It is enough, great, now we need one more test and one of the maintainers will decide.

ghost · 2018-08-14T09:07:33Z

@joeforjoomla please mark your Test as successfully:

open Issue Tracker
Login with your github-Account
Click on blue "Test this"-Button above Authors-Picture
mark your Test as successfully
hit "submit test result"

joeforjoomla · 2018-08-14T09:42:26Z

I have tested this item ✅ successfully on 104f824

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19708.}

ggppdk · 2018-08-14T10:10:47Z

administrator/components/com_admin/sql/updates/postgresql/3.8.6-2018-02-27.sql

+ALTER TABLE "#__session" ALTER COLUMN "session_id" TYPE bytea USING "session_id"::bytea;
+ALTER TABLE "#__session" ALTER COLUMN "session_id" SET NOT NULL;
+ALTER TABLE "#__session" ALTER COLUMN "time" DROP DEFAULT,
+                         ALTER COLUMN "time" TYPE integer USING "time"::integer;


The above will work when executing the file on upgrade,
but will it work with the Database "Fix" button ?

The SQL parsing code for every ALTER TABLE statement seems to handle only 1 altering sub-clause case per ALTER TABLE statement
(i have not tested i just reviewed the following file and i think remember similar case for Mysql in the past)

libraries\src\Schema\ChangeItem\PostgresqlChangeItem.php
(same limitation i think applies for MysqlChangeItem too class)

So I think it should become

ALTER TABLE "#__session" ALTER COLUMN "time" DROP DEFAULT; ALTER TABLE "#__session" ALTER COLUMN "time" TYPE integer USING "time"::integer;

If it can handle multiple sub-clauses then please ignore this

The above will work when executing the file on upgrade,
but will it work with the Database "Fix" button ?

Yes

csthomas · 2018-08-14T12:48:22Z

@ggppdk

The SQL parsing code for every ALTER TABLE statement seems to handle only 1 altering sub-clause case per ALTER TABLE statement

In short, joomla will see only the last sub-clause (to compare difference with db), but db did all clauses.

Explanation for PostgreSQL,

ALTER TABLE "#__session" ALTER COLUMN "time" DROP DEFAULT,
                         ALTER COLUMN "time" TYPE integer USING "time"::integer;
ALTER TABLE "#__session" ALTER COLUMN "time" SET DEFAULT 0;

** The first line ** removes the default value of the time column, but joomla did not see it because there is a second line (after the comma) - joomla should not know that the default value is removed, because it back in the third line,
in the second line, we change the column type, and now joomla sees it,
in the third line we set a new default column value and joomla also sees it.

We use two operations in one "ALTER TABLE" so that joomla can not see the change in the first line. This is useful when we can not directly change the default value (or change the INDEX of column), but first we need to remove the default value and then add another value.

I have tested it again a few minutes ago (by update) and in pgAdmin3 now I have:

  "time" integer NOT NULL DEFAULT 0,

csthomas · 2018-08-14T12:52:51Z

Joomla on PostreSQL before click on "Database Fix"

alikon · 2018-08-14T18:14:10Z

I have tested this item ✅ successfully on 104f824

can't say i was able to meter performance gain, but it's ok for me

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19708.}

ghost · 2018-08-14T18:16:43Z

Ready to Commit after two successful tests.

csthomas · 2018-08-14T18:29:59Z

Thank you all for your interest.

mbabker · 2018-08-28T23:13:37Z

Merged to 3.9-dev via 6d1e90f

joomla-cms-bot added the PR-staging label Feb 16, 2018

Quy reviewed Feb 16, 2018

View reviewed changes

alikon approved these changes Feb 17, 2018

View reviewed changes

Quy reviewed Feb 20, 2018

View reviewed changes

csthomas force-pushed the session_id_and_time branch from 5eda21f to 6b58c8a Compare February 20, 2018 13:49

Session table improvements

104f824

* Add missing alter table for guest * Remove cast to int for the result of time()

csthomas force-pushed the session_id_and_time branch from 6b58c8a to 104f824 Compare February 26, 2018 23:17

csthomas changed the title ~~[WIP] Session table improvements~~ Session table improvements Feb 26, 2018

ghost mentioned this pull request Mar 1, 2018

Backend extremely slow on "large" sites due to User Status backend module #19750

Closed

mbabker approved these changes May 10, 2018

View reviewed changes

csthomas mentioned this pull request Jun 26, 2018

Can't login after setting lifetime to 15000000 #13933

Closed

csthomas referenced this pull request Jul 16, 2018

Use prepared statements in metadata manager

6bc0710

Quy mentioned this pull request Aug 11, 2018

Performance of mod_logged #21555

Closed

ggppdk reviewed Aug 14, 2018

View reviewed changes

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 14, 2018

mbabker added this to the Joomla 3.9.0 milestone Aug 21, 2018

mbabker pushed a commit that referenced this pull request Aug 28, 2018

Session table improvements (#19708)

6d1e90f

mbabker closed this Aug 28, 2018

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 28, 2018

csthomas deleted the session_id_and_time branch August 29, 2018 10:00

alikon mentioned this pull request May 11, 2021

[4][Release Blocker] Unable to cleanly logout of Joomla when using pgsql #33740

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Session table improvements #19708

Session table improvements #19708

csthomas commented Feb 16, 2018 •

edited

Quy Feb 16, 2018

csthomas Feb 19, 2018

csthomas Feb 20, 2018

mbabker Feb 20, 2018

alikon commented Feb 17, 2018

joeforjoomla commented Feb 17, 2018

csthomas commented Feb 17, 2018

mbabker commented Feb 17, 2018

joeforjoomla commented Feb 17, 2018

csthomas commented Feb 17, 2018

joeforjoomla commented Feb 17, 2018 •

edited

Quy commented Feb 17, 2018

csthomas commented Feb 17, 2018

Quy Feb 20, 2018

csthomas Feb 20, 2018

Quy commented Feb 23, 2018

csthomas commented Feb 23, 2018

csthomas commented Feb 26, 2018 •

edited

csthomas commented Aug 13, 2018

csthomas commented Aug 13, 2018

joeforjoomla commented Aug 13, 2018

csthomas commented Aug 14, 2018

ghost commented Aug 14, 2018

joeforjoomla commented Aug 14, 2018

ggppdk Aug 14, 2018

csthomas Aug 14, 2018

csthomas commented Aug 14, 2018

csthomas commented Aug 14, 2018

alikon commented Aug 14, 2018

ghost commented Aug 14, 2018

csthomas commented Aug 14, 2018

mbabker commented Aug 28, 2018

Session table improvements #19708

Session table improvements #19708

Conversation

csthomas commented Feb 16, 2018 • edited

Summary of Changes

Testing Instructions

Expected result

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alikon commented Feb 17, 2018

joeforjoomla commented Feb 17, 2018

csthomas commented Feb 17, 2018

mbabker commented Feb 17, 2018

joeforjoomla commented Feb 17, 2018

csthomas commented Feb 17, 2018

joeforjoomla commented Feb 17, 2018 • edited

Quy commented Feb 17, 2018

csthomas commented Feb 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quy commented Feb 23, 2018

csthomas commented Feb 23, 2018

csthomas commented Feb 26, 2018 • edited

csthomas commented Aug 13, 2018

csthomas commented Aug 13, 2018

joeforjoomla commented Aug 13, 2018

csthomas commented Aug 14, 2018

ghost commented Aug 14, 2018

joeforjoomla commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csthomas commented Aug 14, 2018

csthomas commented Aug 14, 2018

alikon commented Aug 14, 2018

ghost commented Aug 14, 2018

csthomas commented Aug 14, 2018

mbabker commented Aug 28, 2018

csthomas commented Feb 16, 2018 •

edited

joeforjoomla commented Feb 17, 2018 •

edited

csthomas commented Feb 26, 2018 •

edited