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

JTable->reorder is extremely slow #8189

Closed
rysiekpl opened this issue Oct 28, 2015 · 13 comments
Closed

JTable->reorder is extremely slow #8189

rysiekpl opened this issue Oct 28, 2015 · 13 comments

Comments

@rysiekpl
Copy link

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.

@zero-24
Copy link
Contributor

zero-24 commented Oct 29, 2015

Can you send your changes as pull request? So we can test, review and merge it easy? If you need help please see: https://docs.joomla.org/Using_the_Github_UI_to_Make_Pull_Requests


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

@zero-24 zero-24 changed the title JTable->reorder is extremely slow JTable->reorder is extremely slow Oct 29, 2015
@sandrodz
Copy link

I can confirm, we are seeing same issue with 100,000 article DB, saving is extremely slow. Foreach doesn't cut it here.

patch works for me.

rysiekpl added a commit to rysiekpl/joomla-cms that referenced this issue Nov 28, 2015
All credits for the patch should go to Michal Michaláč, who seems to be the author of the original patch.
@rysiekpl
Copy link
Author

Pull request created.

@rysiekpl
Copy link
Author

It occurs to me that this has still not been fixed, after years in the wild, after submitting a patch, creating a pull request, and a long discussion about that very PR.

We are still affected by this, and we're applying the very patch submitted here every single time we upgrade Joomla. This is becoming a huge problem and a strong argument for migrating off of to something else.

Are there any plans to merge the PR or fix the bug in some other way?

@brianteeman
Copy link
Contributor

Every PR needs at least two testers before it can be merged.
I dont see any PR from you only one from @alikon which has no tests

@rysiekpl
Copy link
Author

This is the PR: #8563

I thought it was linked here; there even was a discussion on the PR.

@alikon
Copy link
Contributor

alikon commented Jan 26, 2016

@rysiekpl can you test #8576 wich is not a mysql only solution

@gitcubefree
Copy link

This solution did NOT seem to make a difference in J v3.4.8
We are also using Seblod v3.7.2
12000+ records in the articles
Our Assets table shows everything in sync using ACL Manager
It still takes about 23 seconds to save a new article.
Any other ideas or tests to speed things up?


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

@Fedik
Copy link
Member

Fedik commented Feb 8, 2016

@gitcubefree do test without seblod, because they use some additional call of "reorder" ...

@alikon
Copy link
Contributor

alikon commented Feb 8, 2016

@gitcubefree can you test #8576

@inputsh
Copy link

inputsh commented Feb 9, 2016

@alikon

@rysiekpl and I tested the patch #8576 and we're now able to verify that that patch works.

@wojsmol
Copy link
Contributor

wojsmol commented Feb 9, 2016

@Aleksandar-Todorovic Please mark your test result on https://issues.joomla.org/tracker/joomla-cms/8576

@brianteeman
Copy link
Contributor

Closed as we have a PR #8576


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants