User tests: Successful: Unsuccessful:
This PR provides an improvement in the performance of high-traffic sites.
session_id
from varchar
to varbinary
.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 slowervarchar
takes up to 4 bytes per character when it is loaded into RAMWhy 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
com_patchtester
. All works as before.
Status | New | ⇒ | Pending |
Category | ⇒ | SQL Administration com_admin Postgresql MS SQL Installation Libraries |
I do not know yet.
If someone use join to session table using session_id
column then it would be good to check that.
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).
A common use case to join the session table is for example when dealing with a chat extension allowing communication between guest users.
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.
@joeforjoomla Please mark your test at Issue Tracker. Thanks.
Labels |
Added:
?
|
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.
Title |
|
I updated the code. Now this PR is ready for testing.
@joeforjoomla Could you re-test this PR.
@BrainforgeUK Because you know the subject well, could you test it?
@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
@joeforjoomla thanks,
It is enough, great, now we need one more test and one of the maintainers will decide.
@joeforjoomla please mark your Test as successfully:
I have tested this item
I have tested this item
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,
I have tested this item
can't say i was able to meter performance gain, but it's ok for me
Status | Pending | ⇒ | Ready to Commit |
Ready to Commit after two successful tests.
Thank you all for your interest.
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-08-28 23:13:42 |
Closed_By | ⇒ | mbabker | |
Labels |
Added:
?
|
we should take care of the update path, of course, but a good move