? Success
Pull Request for # 8189

User tests: Successful: Unsuccessful:

avatar rysiekpl
rysiekpl
28 Nov 2015

All credits for the patch should go to Michal Michaláč, who seems to be the author of the original patch.

---copied from #8189----

This is an old issue that has never been properly solved. The original issue has been closed due to lack of testing.

Website I am running started having this issue, and we have verified that:

  • the issue exists in the newest Joomla (3.4.5),
  • the patch below solves the issue.

We are using this patch in production on a high-traffic website. All credits for the patch should go to Michal Michaláč, who seems to be the author of the original patch.

GitHub does not allow me to attach a patch, so here it is, pasted:

--- table.php.orig  2015-10-28 17:44:49.199224888 +0100
+++ table.php   2015-10-28 17:46:52.161204224 +0100
@@ -1367,6 +1367,11 @@
            throw new UnexpectedValueException(sprintf('%s does not support ordering.', get_class($this)));
        }

+        // Speedup by SQL optimalization
+        if ($this->_db->name == 'mysql')
+            return $this->reorderMysql($where);
+       
+        // Default (slow) reorder
        $k = $this->_tbl_key;

        // Get the primary keys and ordering values for the selection.
@@ -1408,6 +1413,47 @@
        return true;
    }

+
+    /**
+     * Method to compact the ordering values of rows in a group of rows
+     * defined by an SQL WHERE clause.
+     *
+     * @param   string  $where  WHERE clause to use for limiting the selection of rows to compact the ordering values.
+     *
+     * @return  mixed  Boolean  True on success.
+     *
+     * @link    http://docs.joomla.org/JTable/reorder
+     * @since   11.1
+     */
+    protected function reorderMysql($where = '')
+    {
+        $k = $this->_tbl_key;
+        
+        $this->_db->setQuery('set @num = 0');
+        $this->_db->execute();
+        
+        $query = $this->_db->getQuery(true)
+            ->update($this->_tbl)
+            ->set('ordering = @num := @num + 1')
+            ->where('ordering >= 0')
+            ->order('ordering');
+ 
+        // Setup the extra where and ordering clause data.
+        if ($where)
+        {
+            $query->where($where);
+        }
+        
+        // Warning: Unpatched version of JDatabaseQuery->__toString ignores 'order' to update query.
+        // Then query must be built from string like this:
+        //$query = "update {$this->_tbl} set ordering = @num := @num + 1 where ordering >= 0 " . $where? (" and " . $where): "" . " order by ordering";
+        
+        $this->_db->setQuery($query);
+        $this->_db->execute();
+ 
+        return true;
+    }
+
    /**
     * Method to move a row in the ordering sequence of a group of rows defined by an SQL WHERE clause.
     * Negative numbers move the row up in the sequence and positive numbers move it down.

avatar rysiekpl rysiekpl - open - 28 Nov 2015
avatar rysiekpl rysiekpl - change - 28 Nov 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 28 Nov 2015
Labels Added: ?
avatar bertmert
bertmert - comment - 28 Nov 2015

I don't know how to test it. Couldn't find any test instructions in other issues.

avatar rysiekpl
rysiekpl - comment - 28 Nov 2015

Will fix the formatting issues tonight.

avatar rysiekpl
rysiekpl - comment - 28 Nov 2015

Fixed the formatting issues, hopefully; and added PDO MySQL support.

avatar alikon
alikon - comment - 30 Nov 2015

if I successfully climbed the current, like a salmon, the performance problem arise when you create a new featured article in a category with more than 100 featured articles, more featured items you have on that category more performance degradation you'll experience.
The issue is related to the reorder() of field ordering but for what I can see this pr use a wrong approach it try to solve the reorder method instead to understand why we need to call reorder () when saving a new article. I'll submit a new pr to solve this performance issue later today , which don't call at all the reorder () when saving a new featured article

avatar sovainfo
sovainfo - comment - 5 Dec 2015

Agree with @alikon that there are two sides to the coin. The original author Michal Michaláč of this patch already mentions that in https://developer.joomla.org/joomlacode-archive/issue-32329.html, so he agrees!
Disagree about calling this the wrong approach. Both sides are independent of each other. Obviously, the code used should be optimised (this PR). Obviously, its usage should be analysed and when possible, avoided.

Unfortunately the quality of the patch provided is not such that it can be incorporated into core. Don't mind as much the fact that it only works for MySQL, do mind that the MySQL specific code is proposed at the wrong spot. The patch needs rewriting to only use the more efficient method. The side benefit of that rewrite will be that it remains database agnostic.
The performance benefit of the patch is achieved by providing 1 update to the database that tells it to update x rows instead of x updates for x rows. Suggest to rewrite the patch to use a stored procedure passing table name and where-condition. Instead of having PHP code to generate SQL code to reorder, request each database to execute the stored procedure. Expect that to be even more efficient!

avatar alikon
alikon - comment - 5 Dec 2015

@sovainfo can you look into #8576

avatar mbabker
mbabker - comment - 5 Dec 2015

Instead of having PHP code to generate SQL code to reorder, request each database to execute the stored procedure.

Since Joomla doesn't publish a list of required database permissions, I'd be weary about relying on it. There is one old update query that had to be rewritten to avoid using temporary tables because of a lack of permission in some environments.

avatar sovainfo
sovainfo - comment - 5 Dec 2015

Can only hope that today the DBA will be fired instead of getting away with poor permission management! Sounds to me the same as not giving write access for the filesystem to the webserver.

Take your loss and change provider, there is no medicine against bad infrastructure management.

avatar brianteeman brianteeman - change - 11 Mar 2016
Rel_Number 0 8189
Relation Type Pull Request for
avatar brianteeman brianteeman - change - 12 Mar 2016
The description was changed
Title
Solving #8189
JTable->reorder is extremely slow
avatar brianteeman
brianteeman - comment - 12 Mar 2016

Updated title to meaningful title and copied description from original issue


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

avatar brianteeman brianteeman - change - 12 Mar 2016
Title
Solving #8189
JTable->reorder is extremely slow
avatar brianteeman brianteeman - change - 30 Mar 2016
Title
JTable->reorder is extremely slow
JTable->reorder is extremely slow
avatar brianteeman brianteeman - change - 30 Mar 2016
Category Libraries
avatar mbabker
mbabker - comment - 6 Jun 2016

OK, someone with a large data set, better working knowledge of database engines than me, and access to environments running different engines, validate my thinking here.

Instead of defining a user variable then running the update query using that, why can't it be a more simple UPDATE table SET ordering = ordering + 1 WHERE conditions query? Unless I'm missing something obvious, would that not be the same patch as proposed exclusively for MySQL but in a cross database manner?

// @aschkenasy @alikon @sovainfo

avatar sovainfo
sovainfo - comment - 6 Jun 2016

Unfortunately, that is not doing the same thing. It just increments each rows ordering. So:
1
3
4
becomes
2
4
5
Instead of
1
2
3

avatar mbabker
mbabker - comment - 6 Jun 2016

Considering there's no enforcement of either unique or sequential ordering values, there's a part of me that doesn't see that as an issue. IMO it'd definitely be preferred to keep them unique and sequential within a group, but I don't see it as a hard requirement.

SQL Server supports user variables, PostgreSQL doesn't (you basically need functions there). https://gist.github.com/mbabker/4074f55253c413e43af835551e30ab8d has a version of this with the Microsoft engine support.

avatar csthomas
csthomas - comment - 6 Jun 2016

I know that is not a solution but I want to show a different idea:
sorting in the opposite direction example patch below:

diff --git a/libraries/joomla/table/table.php b/libraries/joomla/table/table.php
index 121dbf2..c317bb0 100644
--- a/libraries/joomla/table/table.php
+++ b/libraries/joomla/table/table.php
@@ -1378,6 +1378,36 @@ abstract class JTable extends JObject implements JObservableInterface, JTableInt
                        throw new UnexpectedValueException(sprintf('%s does not support ordering.', get_class($this)));
                }

+               /* Speed up patch for mysql - start */
+               if (in_array($this->_tbl, ['#__content', '#__content_frontpage'], true))
+               {
+                       $select = 'SELECT @a := @a - 1 FROM (SELECT @a := 2147483647)';
+                       $dir = 'DESC';
+               }
+               else
+               {
+                       $select = 'SELECT @a := @a + 1 FROM (SELECT @a := 0)';
+                       $dir = 'ASC';
+               }
+
+               $query = $this->_db->getQuery(true)
+                       ->update($this->_tbl)
+                       ->set('ordering = (' . $select . ' s)')
+                       ->where('ordering >= 0')
+                       ->order("ordering $dir");
+
+               // Setup the extra where and ordering clause data.
+               if ($where)
+               {
+                       $query->where($where);
+               }
+
+               $this->_db->setQuery($query);
+               $this->_db->execute();
+
+               return true;
+               /* Speed up patch for mysql - end */
+
                $k = $this->_tbl_key;

Benefit:
old articles won't be reordering each times when user add a new article.

avatar sovainfo
sovainfo - comment - 6 Jun 2016

@mbabker Doubt very much whether the need for reorder is even considered. Seems like it is considered obligatory. Think that they are two separate issues:

  • Do it only when needed
  • Do it as efficient as possible
avatar csthomas
csthomas - comment - 10 Jun 2016

Some idea

1) ordering column can have max value 2147483647.
Common max value for all back-ends may be different.

2) If you look at my patch from previous comment you can see that I put the last article with ordering=2147483647-1.

3) If I create 3 article I will have:

  • ordering=2147483644 - first article, in old style means 1
  • ordering=2147483645 - first article, in old style means 2
  • ordering=2147483646 - first article, in old style means 3

4) To show ordering to users we have to translate it to more human,
using ordering + 1 - MIN(ordering) = human ordering

  • 2147483644 + 1 - 2147483644 = 1
  • 2147483645 + 1 - 2147483644 = 2
  • 2147483646 + 1 - 2147483644 = 3

5) Delete the last article will change all articles ordering (from the same category), but

6) When do you want to add a new article then you set ordering
as MIN(ordering) - 1, means:

  • 2147483644 - 1 = 2147483643
  • now re-ordering is not required

6) When do you want to delete an article then, ex
If I want to delete third item with ordering 2147483645 then I re-order only 2 items with less ordering value.

From:

  • 2147483643 + 1 - 2147483643 = 1
  • 2147483644 + 1 - 2147483643 = 2
  • 2147483646 + 1 - 2147483643 = 4

To:

  • 2147483644 + 1 - 2147483644 = 1
  • 2147483645 + 1 - 2147483644 = 2
  • 2147483646 + 1 - 2147483644 = 3 <- grater ordering won't have to be changed

7) Why I force that numbers?

  • Because it allows me to not reorder all items (from the same category) when adding a new item.
  • In Mysql Innodb (with replication enabled) changing ordering by adding a new articles is very ineffective.
  • When replication is enabled mysql does not send (to slave database) only query which change rows (as it was on myIsam), but it sends binary block of each changed row (or something similar but it is a big data).

Above I could make a few language mistakes.
I added a few examples, numbers to make the text more readable.

avatar rysiekpl
rysiekpl - comment - 11 Jun 2016

I don't know, the longer I look at this the more obvious it becomes -- at least to me -- that perhaps the ordering should be inverted, so that the higher the ordering, the closer to the "top" a given article is (currently it's the opposite). This way there is no need for reordering anything when adding articles.

All the problems in this comment thread stem from the fact that it's the opposite in Joomla. Perhaps it's time to fix that?

avatar euismod2336
euismod2336 - comment - 4 Nov 2016

Hello @rysiekpl,

The last comment here was on 11 Jun 2016. So the question is, Is this issue/pull request still valid?
If no reply is received within 4 weeks we will close this issue.

Thanks for understanding!

Personally, I agree that ordering should be reversed to save everyone this "headache".


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

avatar rysiekpl
rysiekpl - comment - 7 Nov 2016

This issue is indeed still valid. We have just upgraded to Joomla 3.6.4 and still experience the slowness. Applying the patch fixes it.

It's amazing to me that this known bug with a known and production-tested fix has not been fixed yet in mainline.

avatar mbabker
mbabker - comment - 7 Nov 2016

I still advocate that as long as Joomla claims multi database support a
MySQL specific fix shouldn't be accepted. Yes I realize over 98% of users
are using MySQL but we should not be making moves that favor specific
server configurations unless those are the only supported configurations.

IIRC there is another patch proposed that tries to fix this in an agnostic
manner.

On Monday, November 7, 2016, rysiekpl notifications@github.com wrote:

This issue is indeed still valid. We have just upgraded to Joomla 3.6.4
and still experience the slowness. Applying the patch fixes it.

It's amazing to me that this known bug with a known and
production-tested fix
has not been fixed yet in mainline.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8563 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoS2Mo3zt2R8zsXGuoQme9XpMcpc1ks5q7znHgaJpZM4Gq1AN
.

avatar andrepereiradasilva
andrepereiradasilva - comment - 7 Nov 2016

yeah i think there is one from @ggppdk

avatar rysiekpl
rysiekpl - comment - 7 Nov 2016

@mbabker and I will reiterate that this is a BS argument, since it will not make other database servers users lives harder, but it will make 98% of Joomla's users lives much easier.

Otherwise, I don't really care. We're migrating off of Joomla in the long run anyway, and this bug is one of the reasons.

avatar andrepereiradasilva
andrepereiradasilva - comment - 7 Nov 2016

here it is #11184

avatar mbabker
mbabker - comment - 7 Nov 2016

Well, reverse the argument. If someone proposed to fix this performance issue for only PostgreSQL or SQL Server, would it be as acceptable as allowing a MySQL specific fix? I don't have an issue at all with this patch except for the fact it applies to only one of several supported platforms. If Joomla would've stayed MySQL only I would've merged this last year but alas a decision was made to claim additional support and I don't think it is fair to code performance benefits into our platform for only MySQL.

avatar rysiekpl
rysiekpl - comment - 7 Nov 2016

If someone proposed to fix this performance issue for only PostgreSQL or SQL Server, would it be as acceptable as allowing a MySQL specific fix?

Yes, absolutely. It doesn't make my (MySQL user) life any harder, and it makes some users lives easier. It improves the software overall without degrading my experience even a tiny bit.

The other possibility is that a bug that has been found and fixed years ago is still biting users in their different bodyparts because the idea is that the experience should be exactly as shitty for everyone. Which is where Joomla is now.

avatar csthomas
csthomas - comment - 9 Nov 2016

Take a look at PR #12839

avatar Sieger66
Sieger66 - comment - 12 Feb 2017

PR #12839 is closed a new version is at PR #13505

avatar csthomas
csthomas - comment - 12 Feb 2017

IMHO we can close this PR.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 11 Mar 2017

close this PR @Bakual?

avatar zero-24 zero-24 - change - 11 Mar 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-03-11 15:47:57
Closed_By zero-24
avatar zero-24 zero-24 - close - 11 Mar 2017

Add a Comment

Login with GitHub to post a comment