Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up of saving new articles #13505

Merged
merged 2 commits into from
Jan 27, 2017
Merged

Speed up of saving new articles #13505

merged 2 commits into from
Jan 27, 2017

Conversation

csthomas
Copy link
Contributor

@csthomas csthomas commented Jan 8, 2017

Pull Request for Issue #10567.

Related to older implementation at #12839 which requires postgreSQL >= 8.4.2.

Summary of Changes

  • Big performance improvement in JTable::reorder() method with B/C.
  • It works on sqlite, mysql, postgresql and mssql, (on oracle probably too).
  • Do not load any tables to php on "reordering".
  • New public method JDatabaseQuery::selectRowNumber($orderBy, $orderColumnAlias) which add an additional incremental column to SELECT type query.
  • You can use selectRowNumber only once per query.
  • to reorder articles use multi table UPDATE query with SELECT in subquery.

How it works.

  • create SELECT sub-query with row_number column.
  • create UPDATE query with the same where clause.
  • join above queries by primary key or keys if exists.
  • update ordering to new values from SELECT sub-query.

Details about selectRowNumber()

  • on MySQL PostgreSQL >= 8.4.0 and SQL Server SELECT query preserves position of row_number column - you can use safely $db->loadRowList(), examples:
    • ...select('id')->selectRowNumber('ordering ASC', 'row_number') generates result row with keys as 'id, row_number'
    • ...selectRowNumber('ordering ASC', 'row_number')->select('id') generates result row with keys as 'row_number,id'
  • on postgreSQL less that 8.4.0 and SQLite row_number is always on the last column, This is related to index column at loadRow() and loadRowList().

Other changes

  • PostgreSQL:
  • SQL Server:
    • fix multi table UPDATE to generate correct query
    • In SELECT query move HAVING before ORDER BY
  • SQLite:
    • add support for multi table UPDATE query by INSERT OR REPLACE INTO
    • add SQLite function ROW_NUMBER(init=null, partitionBy=null) after establish a connection.
  • Add unit tests for selectRowNumber and multi table UPDATE query.
  • Fix a few other unit test files that generated error after applied my changes.

Testing Instruction

  • go to back end to list of articles (com_content).
  • set order list by "Ordering asceding"
  • select any category and set level 1
  • add 3 example articles (articleA, articleB, articleC) in the same category as featured.
  • back to articles list and you should see articles on the top of list (articleC, articleB, articleA, ...olders...)
  • go to Featured Articles list and set order list by "Ordering asceding"
  • you should see on the top of list (articleC, articleB, articleA, ...olders...)

If something went wrong then test it first without PR.
After this PR result should be the same.

[UPDATED]
How to test on postgreSQL
PostgreSQL Query has 2 versions of code which depend on postgresql server version (< 8.4 or >= 8.4).
So it would be good to test 2 options but as usual there is probably nobody with postgresql >= 8.3.18 and < 8.4.

Documentation Changes Required

Probably.

Introduce a new public method JDatabaseQuery::selectRowNumber().
Main database drivers (mysql, postgresql, sqlsrv and sqlite) can use Update with innerJoin.
SQLite driver has a new sqlite function ROW_NUMBER(init=null, partitionBy=null).

Benchmark (old)

I tested this on MariaDB 10.0.27, php 7.0.8 on my laptop.
reorder_test_on_4000

After patch reorder is almost 100x faster for 4000 articles in one category. Total articles ~233k.

@waader
Copy link
Contributor

waader commented Jan 8, 2017

sorry wrong pull

@photodude
Copy link
Contributor

@csthomas would you consider also adding the JDatabaseQuery selectRowNumber() method and related changes to the Framework database repo

@csthomas
Copy link
Contributor Author

csthomas commented Jan 8, 2017

I will but now I want to add it to joomla 3.7.

@csthomas csthomas changed the title Fast table reorder for mysql, postgresql, mssql and sqlite Speed up of saving new articles Jan 12, 2017
@csthomas csthomas force-pushed the row_number_37 branch 2 times, most recently from 72733e6 to 9d7736e Compare January 15, 2017 09:02
@csthomas
Copy link
Contributor Author

I have added two versions of selectRowNumber(), for postgresql >= 8.4.0 and other for less than 8.4.0.

@ghost
Copy link

ghost commented Jan 22, 2017

I have tested this item ✅ successfully on be84faa

With and without PR Result is same ordering like in "Testing instruction" described.

Tested on:

Joomla! 3.7.0-beta1-nightly
macOS Sierra, 10.12.2
Firefox 50.1.0
PHP 7.0.4
MySQLi 5.5.53-0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13505.

@csthomas
Copy link
Contributor Author

@ggppdk Can I ask you for test mainly on postgreSQL (two versions).
If you have more time then on mysql and on mssql too.

@ggppdk
Copy link
Contributor

ggppdk commented Jan 24, 2017

@csthomas, I moved to new apartment, trying to keep up with existing obligations now, little time till 10 of February, i hope someone can step up to test this

@csthomas
Copy link
Contributor Author

@ggppdk Thanks for response.

@marrouchi
Copy link
Contributor

marrouchi commented Jan 24, 2017

I have tested this item ✅ successfully on be84faa

Tested on mysql version 5.5.53-0ubuntu0.14.04.1

Before patch
COUNT :1116
TIME DIFF : 5.6671

After patch
COUNT :1120
TIME DIFF : 0.0319

It was successful following the Testing instruction.

But when i do the following, with or without the PR :

  1. go to back end to list of articles (com_content).
  2. set order list by "Ordering asceding"
  3. select any category and set level 1
  4. add 3 example articles (articleA, articleB, articleC) in the same category as not featured.
  5. back to articles list and we see articles on the top of list (articleC, articleB, articleA, ...olders...)
  6. check the articles (articleA, articleB, articleC) and hit "Feature" and we still see the same ordering
  7. go to Featured Articles list and set order list by "Ordering asceding" we see on the top of list (articleB, articleC, articleA, ...olders...).

Any ideas on this anomaly ?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13505.

@csthomas
Copy link
Contributor Author

check the articles (articleA, articleB, articleC) and hit "Feature" and we still see the same ordering

this is ok, you add articles to featured but list order is by article ordering (not featured ordering)

go to Featured Articles list and set order list by "Ordering asceding" we see on the top of list (articleB, articleC, articleA, ...olders...).

You add in one time 3 articles to featured, sql will deliver 3 featured with ordering=0 to reorder. IMO It won't preserve any order. it is a set of 3 articles, not ordered list of 3 items.

@csthomas
Copy link
Contributor Author

@waader @alikon @photodude Can you help with test? (mainly postgresql, but it would be nice to test mssql too)

@photodude
Copy link
Contributor

photodude commented Jan 25, 2017

@csthomas I'm currently primarily working on the Windows CI testing with Appveyor
See
joomla-framework/database#72
joomla-framework/database#74
joomla-framework/database#75
#13690
and the Alpha of the PHPCS 2 coding standards autofixers release see
joomla/coding-standards#143

So I haven't had time to test CMS specific PRs (my current local needs to be rebuilt too, especially for any PostgreSQL or MSSQL testing).

If you would be willing to pitch in on those projects, I could allocate time to setup a local for testing here.

There is some additional consideration with PostgreSQL or MSSQL testing, we often only merge those by code review especially when MySQL passes, or if there are no MySQL related changes. Since the change will be held up if try to find someone with a system configuration that meets those edge case requirements and time to test. Final call on a Merge by review would need to be made by the release leader or someone else with commit privileges.

We have two successful tests on MySQL, so technically we just need two reviewers to mark approved and this is RTC

Copy link
Contributor

@photodude photodude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good by code review. Includes Unit tests, so we should be good overall.

@marrouchi
Copy link
Contributor

I have tested this item ✅ successfully on be84faa

Tested on psql (PostgreSQL) 9.3.15

Before the patch
COUNT : 4044
TIME DIFF : 36.7341 sec

After the patch

COUNT : 4044
TIME DIFF : 0.1496 sec


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13505.

@jeckodevelopment
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13505.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 26, 2017
@jeckodevelopment jeckodevelopment added this to the Joomla 3.7.0 milestone Jan 26, 2017
@rdeutz rdeutz merged commit 1b036c6 into joomla:staging Jan 27, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 27, 2017
@csthomas csthomas deleted the row_number_37 branch January 27, 2017 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants