JTable->reorder is extremely slow

[rysiekpl]

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.

[ghost]

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

[mbabker]

Why is this allowed for only the deprecated JDatabaseDriverMysql object? There are additional MySQL object types which do not match this conditional.

[rysiekpl]

Will fix the formatting issues tonight.

[rysiekpl]

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

[alikon]

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

[sovainfo]

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!

[alikon]

@sovainfo can you look into #8576

[mbabker]

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.

[sovainfo]

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.

[brianteeman]

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

[mbabker]

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

[sovainfo]

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

[mbabker]

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.

[csthomas]

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.

[sovainfo]

@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

[csthomas]

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.

[rysiekpl]

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?

[euismod2336]

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

[rysiekpl]

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.

[mbabker]

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 [email protected] 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
.

[andrepereiradasilva]

yeah i think there is one from @ggppdk

[rysiekpl]

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

[andrepereiradasilva]

here it is #11184

[mbabker]

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.

[rysiekpl]

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.

[csthomas]

Take a look at PR #12839

[Sieger66]

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

[csthomas]

IMHO we can close this PR.

[ghost]

close this PR @Bakual?