zencart / zencart

Zen Cart® is a full-function e-commerce application for your website.
https://github.com/zencart/zencart/releases
Other
375 stars 233 forks source link

Question: Admin Customers query optimization on Page 1 #4078

Closed boomer196 closed 2 years ago

boomer196 commented 3 years ago

It appears as though this as been like this since at least 1.5.0 according to git history, but I wanted to ask if there was a reason that Page 1 of the results has a page check.

I have over 100k customers and when I go to Customers (/admin/index.php?cmd=customers) and click on each customer row it is executing a query that is causing all 100k rows to be returned. If you do the same thing on page 2, it no longer does it.

Is there a reason this check was written to include $_GET['page'] == '1'?
Would it be safe to rewrite as if ($_GET['page'] == '' && !empty($_GET['cID'])) {

Line 1219 is where the 100k rows are being returned

https://github.com/zencart/zencart/blob/56c55e2664ca57b138ef59812a3e80b40ea0a57a/admin/customers.php#L1216-L1233

Thanks in advance for any insight!

drbyte commented 3 years ago

I was looking at the same thing a few weeks ago.

Haven't arrived yet at a solution I'm satisfied with. :(

boomer196 commented 3 years ago

Thanks for the quick reply! I look forward to what you might be able to come up with.

I made the change to be if ($_GET['page'] == '' && !empty($_GET['cID'])) {, so I will update the issue if I notice any adverse affects.

On that note, I found this because of slowness on my dashboard and orders pages that I was looking into. I also made a change, that seemed less "risky" to not include attributes there and it was a HUGE performance boost.

$includeAttributesInProductDetailRows = false; in orders.php and RecentOrdersDashboardWidget.php

https://github.com/zencart/zencart/blob/1c039c5d2e43558e4ee0b312c2b3816346144ec4/admin/orders.php#L16 https://github.com/zencart/zencart/blob/1c039c5d2e43558e4ee0b312c2b3816346144ec4/admin/includes/modules/dashboard_widgets/RecentOrdersDashboardWidget.php#L16

nick-cambridge commented 3 years ago

As this code is so linked to the internal workings of the splitPageResults class it was much more sensible to push this code into that class as a findPage method. Under certain circumstances the class could jump straight to the correct page but in the default case of just numbered pages there appeared to be no other solution than to loop though all the results and calculate the page if no page is given but at least the page functionality was nicely encapsulated in one class rather than having this code duplicated across multiple files. You can see the same code in many files in the system such as orders.php.

There are admin pages that, although they can take record id parameter such as cid, do not actually highlight the requested record unless it happens to be on the first page. This could be improved by managing this sort of behaviour in a more generic way either using the above find method or have a generic Table handling page handles this sort of thing in a generic (and effective) way.

Just thought I would share that with you.

scottcwilson commented 2 years ago

@boomer196 I have noted these tweaks in the performance documentation. Thanks for the pointer.

Can this ticket be closed?

boomer196 commented 2 years ago

@scottcwilson, I am fine with closing this ticket. Thanks!