3
\$\begingroup\$

I am creating a sort of Sales Report for an Invoicing System.

I have an extremely complex MySQL Query that takes a very long time to execute (usually about 10 - 15 seconds). It involves multiple tables (containing hundreds or even thousands of rows) in a database that an even more complex system is built upon.

At this stage in development, it doesn't seem to me that it would be beneficial to change the structure of the database tables for this one query since everything else is working very fast.

If you care to review my query and give me advice on how to optimize my code to achieve the quickest possible time in terms of speed, I would be more than grateful.

I am using PHP with MySQLi.

SELECT `u`.`first`, `u`.`last`,
    `i`.`id`, IFNULL(`i`.`vendor`, `d`.`unattached_vendor`) AS `vendor`, IFNULL(`i`.`originalskunumber`, `d`.`skunumber`) AS `skunumber`,
    `d`.`date_sent`, `d`.`document_type`, `d`.`document_id`, `d`.`description`, `d`.`qty`, `d`.`triage_notes`, `d`.`status`, `d`.`arrival_date`, `d`.`id` AS `document_row`
    FROM `documents` AS `d`
        LEFT JOIN `inventory` AS `i`
            ON (CASE
                WHEN `d`.`document_type` = 'Invoice'
                    THEN `i`.`skunumber` = `d`.`skunumber`
                WHEN `d`.`document_type` = 'Purchase Order'
                    THEN `i`.`originalskunumber` = `d`.`skunumber`
            END)
        INNER JOIN `users` AS `u`
            ON `u`.`uid` = `d`.`customer_id`
        WHERE `d`.`document_type` <> 'Quote'
            AND `d`.`header_item` = '0'
            AND `d`.`active_document` = '1'
            AND `d`.`active_item` = '1'
            AND (CASE
                WHEN `d`.`document_type` = 'Invoice' AND `d`.`skunumber` IS NOT NULL
                    THEN (`d`.`document_id`, LEFT(`d`.`skunumber`, 3)) NOT IN (SELECT `trailing_doc`, LPAD(`vendor_code`, 3, '0') AS `vendor_code` FROM `documents` WHERE `active_document` = '1' AND `active_item` = '1' AND `trailing_doc` IS NOT NULL AND `vendor_code` IS NOT NULL)
                WHEN `d`.`document_type` = 'Purchase Order'
                    THEN 1
                ELSE 1
            END)
            AND (CASE
                WHEN `d`.`document_type` = 'Invoice'
                    THEN `d`.`document_id` IN (SELECT DISTINCT `document_id` FROM `payments` WHERE `active_payment` = '1' AND `payment_amount` > 0)
                WHEN `d`.`document_type` = 'Purchase Order'
                    THEN 1
            END)
            AND (CASE
                WHEN `i`.`id` IS NULL AND `document_id` IN (SELECT DISTINCT `trailing_doc` FROM `documents` WHERE `document_type` = 'Purchase Order' AND `active_document` = '1' AND `active_item` = '1' AND `trailing_doc` > '')
                    THEN 0
                ELSE 1
            END)
    GROUP BY `d`.`document_id`, `i`.`originalskunumber`

As requested, here's my EXPLAIN:

id  select_type         table       type    possible_keys   key     key_len     ref                     rows    Extra   
1   PRIMARY             d           ALL     NULL            NULL    NULL        NULL                    2298    Using where; Using temporary; Using filesort
1   PRIMARY             u           eq_ref  PRIMARY         PRIMARY 4           example.d.customer_id   1       Using where
1   PRIMARY             i           ALL     NULL            NULL    NULL        NULL                    77418   Using where
4   DEPENDENT SUBQUERY  documents   ALL     NULL            NULL    NULL        NULL                    2298    Using where; Using temporary
3   DEPENDENT SUBQUERY  payments    ALL     NULL            NULL    NULL        NULL                    14      Using where; Using temporary
2   DEPENDENT SUBQUERY  documents   ALL     NULL            NULL    NULL        NULL                    2298    Using where
\$\endgroup\$
  • 2
    \$\begingroup\$ Post your EXPLAIN and the table definitions \$\endgroup\$ – Mihai May 12 '16 at 21:58
  • \$\begingroup\$ Welcome to Code Review. I removed the PHP related tags, as there is no PHP specific language to be reviewed, this is all valid SQL code on its own. Hope you get some good answers! \$\endgroup\$ – Phrancis May 12 '16 at 22:31
  • \$\begingroup\$ Could you edit your question to add an explanation, in English, of exactly what your code is supposed to do? \$\endgroup\$ – Nic Hartley May 13 '16 at 4:05
6
\$\begingroup\$

There are some formatting and code clarity points I will mention a bit further down, but for now there are some places I think the performance could be improved, at a glance. You should definitively consider posting your EXPLAIN and table definitions, as was suggested in a comment, for this and future SQL questions, as those can make a huge difference, often more so than the queries themselves.


Bottlenecks

This JOIN is perplexing:

FROM documents AS d
    LEFT JOIN inventory AS i
        ON (CASE
            WHEN d.document_type = 'Invoice'
                THEN i.skunumber = d.skunumber
            WHEN d.document_type = 'Purchase Order'
                THEN i.originalskunumber = d.skunumber
        END)
  1. There is a fundamental problem if you are using a case expression like this as a joining condition. Why are your Invoice and Purchase Order documents not normalized into their own tables? I would suggest to take another look at the architecture there, especially if you find yourself having to do this kind of join often.

  2. Parsing strings in SQL is expensive. At the very least, I hope your document_type column is indexed. If it isn't, then that's a choke point, because it has to scan the documents table (instead of an index) just to be able to join. If you find yourself making joins on non-index columns more than just very seldom, you need to look to do some serious indexing, and consider normalizing.

  3. You are using a LEFT JOIN so I am assuming you are expecting null values... but the way you wrote the expression does not make that clear. Consider at the very least adding a comment to explain where you are expecting nulls and how it handles it.

    I had to notice further down what the left join is actually for, with this bit...

    WHEN d.document_type = 'Invoice' AND d.skunumber IS NOT NULL
    
  4. How many other kinds of documents are stored like this? I see at least one more, Quote. Good question to ask yourself, I think. It's much easier to re-architect databases earlier on, if you have that luxury, than after you've written all your application around a sub-par architecture.


This is a bit weird:

WHERE d.document_type <> 'Quote'
    AND d.header_item = '0'
    AND d.active_document = '1'
    AND d.active_item = '1'

What are those '0' and '1'supposed to mean? Why are they characters instead of say, a BIT if they are meant to be true/false, or a TINYINT or INT if that is just a key that represents some value?


This part (and the 2 after it) in your where clause is formatted in such a way that as a one-liner you don't really see what is going on. Once you format it like the rest of your query is, it becomes clear that it's a very inefficient operation:

WHERE ...
...
AND (CASE
    WHEN d.document_type = 'Invoice' AND d.skunumber IS NOT NULL
        THEN (d.document_id, LEFT(d.skunumber, 3)) NOT IN (
            SELECT 
                trailing_doc, 
                LPAD(vendor_code, 3, '0') AS vendor_code 
            FROM documents 
            WHERE active_document = '1' 
                AND active_item = '1' 
                AND trailing_doc IS NOT NULL 
                AND vendor_code IS NOT NULL
        )
    WHEN d.document_type = 'Purchase Order'
        THEN 1
    ELSE 1
END)

Wow, this is complex.

  1. This condition is redundant: WHEN d.document_type = 'Purchase Order' THEN 1 ELSE 1. If a value doesn't fall in your first case, it will be 1 either way, so just keep the ELSE 1 by itself.

  2. Again, I hope this document_type column is indexed, otherwise here we go again parsing those strings!

  3. Now at first glance, this subquery seems really odd. You are verifying that your document_id (primary key, I'm assuming) is not contained in trailing_doc column (why?) and, perhaps even more strangely to an outsider, that the left-most 3 characters of your skunumber (why is it called a number if it has characters?) are not the same as the vendor_code padded to 3 characters with 0s to the left. So presumably, a skunumber could look something like 011999999 where 011 is the vendor code, and 999999 is the actual unit number. Did I guess right? That, to me, would be deserving of at the very least a small explanation as code comment, so the next maintainer doesn't have to do this kind of mental gymnastics as well.

  4. Again, if performance is a concern, I really hope you have indexes on skunumber, vendor_code and trailing_doc. If you don't, then there's another bottleneck.

All the same points apply on your other subqueries to payments table. (except maybe point 3 that's pretty specific to this one).


Split into multiple, more specialized queries?

Consider splitting this logic into at least 2 queries, one for invoices and one for purchase orders, since they are really not the same thing. If need be, you could always bring them together using UNION, or combine them some other way (maybe in your application?), or altogether just keep them separated.


Code style

A few things:

  1. All those back-ticks are really distracting, and really not needed. I realize they are added there by a PHP tool, but a quick find & delete in a text editor, or using a regex, is all that's needed to remove them.

  2. Some line breaks make the first part of a SELECT easier to read, like

    SELECT u.first, u.last, i.id, IFNULL(i.vendor, d.unattached_vendor) AS vendor, IFNULL(i.originalskunumber, d.skunumber) AS skunumber, ....

  3. It's tempting to use 1-letter aliases for tables, but in bigger queries it can make things very hard to follow, once you start joining in 6, 9, 12 tables, to keep track of all those d, u and i names. Naming things is hard, but reading unclear code is harder.

I made a pastebin of how I reformatted your query here.

\$\endgroup\$
  • 1
    \$\begingroup\$ And you say you aren't SQL guy. \$\endgroup\$ – Nic Hartley May 13 '16 at 4:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.