PR-staging RTC

Pending

User tests: Successful: Unsuccessful:

avatar csthomas
csthomas
16 Feb 2018

Summary of Changes

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

  1. Change type of column session_id from varchar to varbinary.
  2. 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 #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

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

Expected result

All works as before.

avatar csthomas csthomas - open - 16 Feb 2018
avatar csthomas csthomas - change - 16 Feb 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 16 Feb 2018
Category SQL Administration com_admin Postgresql MS SQL Installation Libraries
avatar csthomas csthomas - change - 16 Feb 2018
The description was changed
avatar csthomas csthomas - edited - 16 Feb 2018
avatar alikon
alikon - comment - 17 Feb 2018

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

avatar joeforjoomla
joeforjoomla - comment - 17 Feb 2018

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

avatar csthomas
csthomas - comment - 17 Feb 2018

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

avatar mbabker
mbabker - comment - 17 Feb 2018

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

avatar joeforjoomla
joeforjoomla - comment - 17 Feb 2018

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

avatar joeforjoomla
joeforjoomla - comment - 17 Feb 2018

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.

avatar Quy
Quy - comment - 17 Feb 2018

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

avatar csthomas
csthomas - comment - 17 Feb 2018

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

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

avatar csthomas csthomas - change - 20 Feb 2018
Labels Added: PR-staging
avatar Quy
Quy - comment - 23 Feb 2018

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

avatar csthomas
csthomas - comment - 23 Feb 2018

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.

avatar csthomas csthomas - change - 26 Feb 2018
The description was changed
avatar csthomas csthomas - edited - 26 Feb 2018
avatar csthomas csthomas - change - 26 Feb 2018
Title
[WIP] Session table improvements
Session table improvements
avatar csthomas csthomas - edited - 26 Feb 2018
avatar csthomas
csthomas - comment - 26 Feb 2018

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

avatar csthomas csthomas - change - 8 Mar 2018
The description was changed
avatar csthomas csthomas - edited - 8 Mar 2018
avatar csthomas csthomas - change - 14 Mar 2018
The description was changed
avatar csthomas csthomas - edited - 14 Mar 2018
avatar csthomas csthomas - change - 10 May 2018
The description was changed
avatar csthomas csthomas - edited - 10 May 2018
avatar csthomas csthomas - change - 13 Aug 2018
The description was changed
avatar csthomas csthomas - edited - 13 Aug 2018
avatar csthomas
csthomas - comment - 13 Aug 2018

@joeforjoomla Could you re-test this PR.

avatar csthomas
csthomas - comment - 13 Aug 2018

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

avatar joeforjoomla
joeforjoomla - comment - 13 Aug 2018

@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

avatar csthomas
csthomas - comment - 14 Aug 2018

@joeforjoomla thanks,

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

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 14 Aug 2018

@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"
avatar joeforjoomla
joeforjoomla - comment - 14 Aug 2018

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.

avatar joeforjoomla
joeforjoomla - comment - 14 Aug 2018

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.

avatar joeforjoomla joeforjoomla - test_item - 14 Aug 2018 - Tested successfully
avatar csthomas
csthomas - comment - 14 Aug 2018

@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,
avatar csthomas
csthomas - comment - 14 Aug 2018

Joomla on PostreSQL before click on "Database Fix"

screenshot_20180814_145204

avatar alikon alikon - test_item - 14 Aug 2018 - Tested successfully
avatar alikon
alikon - comment - 14 Aug 2018

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.

avatar franz-wohlkoenig franz-wohlkoenig - change - 14 Aug 2018
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 14 Aug 2018

Ready to Commit after two successful tests.

avatar csthomas
csthomas - comment - 14 Aug 2018

Thank you all for your interest.

avatar mbabker
mbabker - comment - 28 Aug 2018

Merged to 3.9-dev via 6d1e90f

avatar mbabker mbabker - change - 28 Aug 2018
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2018-08-28 23:13:42
Closed_By mbabker
Labels Added: RTC
avatar mbabker mbabker - close - 28 Aug 2018

Add a Comment

Login with GitHub to post a comment