Encounter history build time

jwallace00 wrote on Wednesday, October 13, 2010:

We’ve got a patient base of about 20k and a lot of patients in Dr. Bowen’s instance have many 10s or 100s of encounters.  When looking at the visit history on a default 4.0 installation,  the display for /interface/patient_file/history/encounters.php takes several seconds too long to load completely.  Estimates vary, but suffice it to say the users are ticking their fingers waiting for the page to load.   

We’ve taken a couple steps to help alleviate the problem which consist of indexing for the patient_data.{pid,pubpid} columns as well as the form_encounter.pid column.   This seems to have a dramatic impact on the load time for the encounter history.   We also uncommented the counter in /interface/patient_file/history/encounters.php which restricts the number of displayed results to approximately 15 records or so.  

I don’t know that this second effort really saves us much by way of performance since the counter only restricts the number of records displayed and not the number queried from the database.  

Would it be advisable to add a new global setting that might modify the behavior of the get_encounters() function to restrict the number of results gathered from the database? 

Thanks,
wallace

sunsetsystems wrote on Wednesday, October 13, 2010:

Yes, a global setting for that sounds reasonable to me.

Rod
www.sunsetsystems.com

whimmel wrote on Thursday, October 14, 2010:

Speaking of a large patient base, we have one client with at least that many patients and lots of providers in multiple facilities, each with staff beating on it all day long.  It was taking a loooong time for the calendar to load.

Adding indexes wasn’t enough because MySQL wasn’t able to optimize the query in pnuserapi.php postcalendar_userapi_pcQueryEventsFA() and postcalendar_userapi_pcQueryEvents()

I applied this patch and performance was improved considerably:

Index: pnuserapi.php
===================================================================
--- pnuserapi.php	(revision 384)
+++ pnuserapi.php	(revision 812)
@@ -754,12 +754,12 @@
     "concat(u.fname,' ',u.lname) as provider_name, " .
     "concat(pd.fname,' ',pd.lname) as patient_name, " .
     "concat(u2.fname, ' ', u2.lname) as owner_name, pd.DOB as patient_dob " .
-    "FROM  ( $table AS a, $cattable AS b ) " .
+    "FROM  ( $table AS a ) " .
+    "LEFT JOIN $cattable AS b ON b.pc_catid = a.pc_catid ".
     "LEFT JOIN users as u ON a.pc_aid = u.id " .
     "LEFT JOIN users as u2 ON a.pc_aid = u2.id " .
     "LEFT JOIN patient_data as pd ON a.pc_pid=pd.pid " .
-    "WHERE  b.pc_catid = a.pc_catid " .
-    "AND a.pc_eventstatus = $eventstatus " .
+    "WHERE a.pc_eventstatus = $eventstatus " .
     "AND (a.pc_endDate >= '$start' OR a.pc_endDate = '0000-00-00') " .
     "AND a.pc_eventDate <= '$end' " .
     "AND (a.pc_aid = '" . $provider_id . "' OR a.pc_aid = '')";
@@ -985,12 +985,12 @@
     "concat(pd.lname,', ',pd.fname) as patient_name, " .
     "concat(u2.fname, ' ', u2.lname) as owner_name, " .
     "DOB as patient_dob, pd.pubpid " .
-    "FROM  ( $table AS a, $cattable AS b ) " .
+    "FROM  ( $table AS a ) " .
+    "LEFT JOIN $cattable AS b ON b.pc_catid = a.pc_catid ".
     "LEFT JOIN users as u ON a.pc_aid = u.id " .
     "LEFT JOIN users as u2 ON a.pc_aid = u2.id " .
     "LEFT JOIN patient_data as pd ON a.pc_pid = pd.pid " .
-    "WHERE  b.pc_catid = a.pc_catid " .
-    "AND a.pc_eventstatus = $eventstatus " .
+    "WHERE  a.pc_eventstatus = $eventstatus " .
     "AND ((a.pc_endDate >= '$start' AND a.pc_eventDate <= '$end') OR " .
     "(a.pc_endDate = '0000-00-00' AND a.pc_eventDate >= '$start' AND " .
     "a.pc_eventDate <= '$end')) ";

bradymiller wrote on Friday, October 15, 2010:

hey,

Was any functionality lost on above query? Can you briefly describe why it’s so much faster (trying to figure out joined sql statements generally gives me a headache.

Regarding speed on the encounter listing. To elements likely playing a part. The query itself and then the rendering onto the page. Limiting the number will help with the rendering, but I recommend paginating it, so docs can go back if needed. Could either paginate it by number of items or by time (for example: Last 6 months, Last 1 year, All); important because commonly we have to go back to the history to quickly investigate/confirm something. On the query front, well placed keys/indexes are vital. These can be the difference of querying through a million records vs a hundred (since you’ve keyed the pid). Generally best to find the faulty query and then test it with EXPLAIN comand:
http://www.databasejournal.com/features/mysql/article.php/1382791/Optimizing-MySQL-Queries-and-Indexes.htm

I had to did this a year or so with the translation tables, which reduces each query from number of definitions (about 50,000) to just the number of languages (about 20).

Also, I don’t think simply  reducing the number of mysql elements returned on the query will save time, since still going sort through all the available records.

-brady

whimmel wrote on Friday, October 15, 2010:

Was any functionality lost on above query? Can you briefly describe why it’s so much faster (trying to figure out joined sql statements generally gives me a headache.

No functionality lost that I was able to detect. A real DBA might have more insight.

It’s so much faster because no matter how I indexed the tables, mysql wouldn’t use them. With the one join in the from statement, it would scan 100K rows no matter what according to the explain output. Once I tried the left join like the rest, it used the index and cut down the number of rows that mysql had to pick through.

tmccormi wrote on Friday, November 05, 2010:

Any reason not to go ahead and implement this patch?
-Tony

bradymiller wrote on Saturday, November 06, 2010:

hey,
Go for it. This can be your first git commit.
-brady

tmccormi wrote on Saturday, November 06, 2010:

commit 499918033ccf52f197f27ae5694b1b2a32dae469
Author: Tony McCormick <tony@mi-squared.com>
Date:   Sat Nov 6 01:26:35 2010 -0700

    Phyaura Calendar speed enhancement

bradymiller wrote on Saturday, November 06, 2010:

Tony,
You brought in a lot of extra commits? (24 of them it looks like) I remember warning you regarding your repo via email in the not so distant past :slight_smile: Please look at the log before going through with the commit.

Stephen,
All of the commits have already been pushed to the mirrors so don’t want to reverse it. The diff between the current master and previous master only consist of Tony’s main commit, but there’s like 23 or so merge commits with lots of changes in them; very weird. Two quick questions:
How did this happen?
Is there a way to avoid this in the future?

thanks,
-brady

stephen-smith wrote on Saturday, November 06, 2010:

Tony,
Brady’s right.  You shouldn’t merge master into a feature branch.  After you do that you branch stops being “commits related to feature X” and becomes “commits related to feature X plus random stuff from master”.  If you want to change your “branch point” from “master last time I was working” to “master right now” you should be using rebase, but you should also be prepared for the rebase fallout.

Brady,
How did what happen?  How did the commits get in the sourceforce repository?  Tony did a fast-forward push.  How did tony create the commits?  With a git merge master that had no conflicts.
Is there a way to avoid what in the future?  Clean merge commits?  It’s possible, PostgreSQL has an update hook that does it, I guess.  I won’t be writing that hook though.

GitHub / Gitorious (really most git tools) are a bit “generous” with the diff they show for a merge commit.{1}  I’ve inspected all Tony’s merges from master and most are clean and conflict-free.  d7a44ad053c6a is an exception - there were merge conflicts Tony had to resolve outside of Git; the commit message probably should have mentioned that.  However, 97be6b42070759 reverts changes pulled in my that commit.{2}  QGit (and probably some other tools) separates changes made by the merge commit from changes made in (one of) the parents of a merge commit, so it was easy for me to see which merges were clean.

{1} See “DIFF FORMAT FOR MERGES” in the git-diff(1) manpage.  It answers the question “How is this commit’s tree different from ANY of the trees in its parents?”.  It’s not what you want if you are asking “What changes did this commit pull in?”.  In that case you want git diff merge^0 merge or similar, i.e. a diff against exactly one parent.  N.B.: the git developers refer to any merge commit that has changes that are not in ANY parent as “dirty” or “evil”; it’s possible to create one of these by resolving conflicts outside of git.

{2} That commit reverts a merge commit, which is probably not what Tony wanted.  The git-revert(1) manpage has a warning about it.  You need to read and understand http://www.kernel.org/pub/software/scm/git/docs/howto/revert-a-faulty-merge.txt before using git revert on a merge commit.

HTH

bradymiller wrote on Saturday, November 06, 2010:

Stephen,

I’ll read up on your links and go through the tools myself, but just one more simple question:
1) Is the repo ok (I noted you mentioned a merge conflict with d7a44ad053c6a; is this bad?)?

-brady

bradymiller wrote on Saturday, November 06, 2010:

Stephen,
To be more descriptive on my question (I noted in your above message that you prefer more descriptive questions):
Is the current ‘master’ branch on sourceforge still robust or does it need to be fixed?
-brady

tmccormi wrote on Sunday, November 07, 2010:

Guys,

What step did I miss?  my git hub account was not used in the process at all to my knowledge, just local work, but I had re-forked the project before I stared and it look perfectly clean.   What conflicts would I have resolved out side of git?  I saw no conflict merge errors or warnings of any sort.

What are you actually looking at to see this information, nothing I looked at locally or on my github looks usual to me.  May I just don;t know what to look at?

The commit is: 499918033ccf52f197f2 according to what on openemr/openemr github now and looks also looks just fine to me.

I followed the steps exactly as published.  So I don’t know what the issue was.

  I pulled the current source from SF into my local master
  I created a brand new branch, made the changes manually to one file.
  did a rebase from the master into that branch (which seemed unnecessary since I started in sync on this one)
  checkout the master branch
  git merge from the branch into the master
  confirmed with git log that ONLY the new stuff was added
  and pushed it.

How could that have included anything else?

Do I need to flush my git repos local and remote?  Local is easy, and I thought I had taked care of the remote when I re-forked openermr on github.

-Tony

tmccormi wrote on Sunday, November 07, 2010:

Tony, Brady’s right. You shouldn’t merge master into a feature branch.

So … isn’t this instruction form the wiki exactly what you are saying not to do?  These are the step I follow, exactly.

3. Rebase your custom branch (fix any conflicts that arise during the rebase before proceeding).

git checkout <your_branch>
git rebase master

    4. Merge your custom branch into your master branch.:

git checkout master
git merge <your_branch>

Just trying to get a handle on what looks like conflicting information to me now.
-Tony

bradymiller wrote on Sunday, November 07, 2010:

Tony,

Though your public github repo looked good after the fork, I’m guessing you used your previous local repo by mistake.  It’s the merge commits you also brought in with it (the first was the second one in the git log output). Here’s a listing of some of them:
http://openemr.git.sourceforge.net/git/gitweb.cgi?p=openemr/openemr;a=search;s=Tony+McCormick;st=author
And a couple more intermixed here:
http://openemr.git.sourceforge.net/git/gitweb.cgi?p=openemr/openemr;a=search;s=tmccormi;st=author

Look through the code with the qgit program; then it’s more clear what happened in regards to Stephen’s input above.

When you update your local ‘master’ from official remote (either github official mirror or sourceforge), how do you do it?
Are you doing following described here:
http://www.openmedsoftware.org/wiki/Git_for_dummies#Step_3:_Feeding_your_repository

My take is that the sourceforge git repo is still ok (still awaiting Stephen’s input); it’s just that the additional 24 commits confuse things and time was spent by others to ensure no other things were changed.

This isn’t about blame; it’s more about figuring out how we can avoid this in the future (such as clarifying our documentation, being more stringent on commit access,  or instituting a hook).

-brady

sunsetsystems wrote on Sunday, November 07, 2010:

Wow, it sure looks like a lot of stuff was committed here that was not intended.  Here is a diff between “git log” before and after pulling in the new stuff:

0a1,13
> commit 499918033ccf52f197f27ae5694b1b2a32dae469
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Sat Nov 6 01:26:35 2010 -0700
> 
>     Phyaura Calendar speed inhancement
> 
> commit 352041b300668b4b28a19fa5ecc2ab573db410c4
> Merge: d104c7d... 198e01f...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Sat Nov 6 01:27:22 2010 -0700
> 
>     Merge branch 'master' of ssh://openemr.git.sourceforge.net/gitroot/openemr/openemr
> 
13a27,33
> commit d104c7de21c13b4f1e950b6d0bfe04bff6a59e33
> Merge: d69d2c6... 3ac5bc5...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Tue Nov 2 22:13:41 2010 -0700
> 
>     Merge branch 'master' of ssh://openemr.git.sourceforge.net/gitroot/openemr/openemr
> 
80a101,107
> commit d69d2c656cdac1727052500f1c02eb05bfa33c91
> Merge: f3690f6... fab6993...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Thu Oct 28 22:49:04 2010 -0700
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
116a144,150
> commit f3690f6e52c01288f6054e5c4eb3e2ab36de26a3
> Merge: 6b3010f... ec5bed3...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Wed Oct 27 14:39:03 2010 -0500
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
128a163,169
> commit 6b3010f36994dd39968179c381fe1ea8f18c1e1a
> Merge: adfea96... 20b0395...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Fri Oct 22 17:56:51 2010 -0700
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
149a191,197
> commit adfea9669ab8cbea07fba196c014feea23448616
> Merge: c09c67c... c2b3a99...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Fri Oct 15 23:59:32 2010 -0700
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
244a293,299
> commit c09c67cec9d9d08e0413b5084f5aadc75fccc991
> Merge: 7377a2f... 457d4d2...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Sun Oct 10 22:15:03 2010 -0700
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
399a455,461
> commit 7377a2f0dc6aed74962c4bd84d3d7353d47419e2
> Merge: 0ccd8da... ea88eaa...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Sat Oct 2 10:16:31 2010 -0500
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
405a468,481
> commit 0ccd8da2b3ff8f9798de9b7d93e8baa09a7b757c
> Merge: daa312a... 10f51a6...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Fri Oct 1 08:43:59 2010 -0500
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
> commit daa312adc047b26a6a40362bd27dd1349e6b904a
> Merge: da3f9f5... 7feb10d...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Thu Sep 30 22:38:07 2010 -0500
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
421a498,504
> commit da3f9f557076407ea2a71dd3018cd16ee7f4ab84
> Merge: b242e96... 4da6315...
> Author: tmccormi <tony@mi-squared.com>
> Date:   Thu Sep 23 07:25:45 2010 -0700
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
433a517,523
> commit b242e96f595917a7413749967b8fd9b9eac8b6e0
> Merge: 1010480... 1ab7d1e...
> Author: tmccormi <tony@mi-squared.com>
> Date:   Tue Sep 21 09:57:37 2010 -0700
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
475a566,572
> commit 1010480f2acc473452eaea3b9ab6e95188c9ec5b
> Merge: 5637075... d034a8e...
> Author: tmccormi <tony@mi-squared.com>
> Date:   Mon Sep 13 23:38:36 2010 -0700
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
559a657,663
> commit 5637075d6dedbb139560a3cab8634dfb2a0519d8
> Merge: dc0ebdc... c14b662...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Fri Aug 27 12:52:55 2010 -0700
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
565a670,676
> commit dc0ebdcd8a487661b7ef5f32343aa41645029aa1
> Merge: 2faca05... 92f3fc3...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Tue Aug 24 21:29:33 2010 -0700
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
583a695,701
> commit 2faca05572ffdd27d63acbdd74e9827a782fdf0d
> Merge: 97be6b4... 21f0b97...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Fri Aug 20 21:32:35 2010 -0700
> 
>     Merge branch 'master' of git://github.com/openemr/openemr
> 
699a818,833
> commit 97be6b420707590159d77592fe3c7ecb1919f228
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Fri Aug 6 22:37:37 2010 -0700
> 
>     Revert "Merge branch 'fmc-custom' of github.com:tmccormi/openemr"
>     
>     This reverts commit b0231af6922f86c003c53086bc7a76493f5c9f16, reversing
>     changes made to dcff0e1f6cc819f543b5486e9b52850275992cf4.
> 
> commit b0231af6922f86c003c53086bc7a76493f5c9f16
> Merge: dcff0e1... c9f0fa3...
> Author: Tony McCormick <tony@mi-squared.com>
> Date:   Fri Aug 6 22:26:01 2010 -0700
> 
>     Merge branch 'fmc-custom' of github.com:tmccormi/openemr
> 
719a854,876
> commit c9f0fa339e25413256dd17884c58fa7b323b5947
> Author: tmccormi <tony@mi-squared.com>
> Date:   Tue Aug 3 22:38:00 2010 -0700
> 
>     Update comments
> 
> commit 4f1b3a4ff96236013682de8b459a226798cd9b2a
> Author: tmccormi <tony@mi-squared.com>
> Date:   Tue Aug 3 22:36:03 2010 -0700
> 
>     Minor fixes to links and comments
>     `
> 
> commit d7a44ad053c6a3b4d2aabff80a0325361e2cd5f4
> Merge: 7b16494... 7da2b7a...
> Author: tmccormi <tony@mi-squared.com>
> Date:   Tue Aug 3 19:46:48 2010 -0700
> 
>     Merge branch 'master' into fmc-custom
>     
>     Conflicts:
>     	sql/3_2_0-to-4_0_0_upgrade.sql
> 
810a968,979
> commit 7b16494445038648e520e7fc306d97d7179cd101
> Author: tmccormi <tony@mi-squared.com>
> Date:   Wed Jul 28 00:23:16 2010 -0700
> 
>     Add New FMC code from Intesync
> 
> commit 69adb27cadd4d7e21b1c637f611a79ca14f62b4a
> Author: tmccormi <tony@mi-squared.com>
> Date:   Tue Jun 29 16:39:31 2010 -0700
> 
>     And the new stuff...
> 

Top priority here is the integrity of the SF repository.  I don’t think it’s acceptable for us to not know what’s been changed.  Can someone assure me that no unintended changes remain?  If not, what are our options for reverting or restoring from a prior copy?

Rod
www.sunsetsystems.com

tmccormi wrote on Sunday, November 07, 2010:

Not concerned about blame or anything like that.  Just want to figure it out. 
-Tony

sunsetsystems wrote on Sunday, November 07, 2010:

Main thing here is to learn from our mistakes and establish procedures to minimize them and be able to recover easily.  They are going to happen, nobody is immune.

Rod
www.sunsetsystems.com

tmccormi wrote on Sunday, November 07, 2010:

The graph looks like the unintended delivers all dead end without merging back into  into the main trunk.  but I’d certainly support reseting it or whatever you think is the right solution. 

I am sorry about this, it did not look like that locally, just a simple 1 file change.

-Tony

bradymiller wrote on Sunday, November 07, 2010:

Rod,

The diff of the development tip code before and after all of Tony’s above commits only include the code committed in the Phyaura patch. So, basically, all the other commits amount to nothing in code changes.  Most are considered “Trivial Merges”, which I think were there because Tony was merging from the official git repo rather than pulling from it while updating his local repo. He actually recently fixed his repo (at least the public gihub repo), which is why I’m guessing he used his previous repo by mistake. Above, Stephen mentions a commit with a conflict, which was then reverted, so doesn’t seem to be an issue. So, I think the SF repo is still robust (although awaiting Stephen’s confirmation).  If the repo is ok, then no reason to remove these commits, because it will likely cause incompatibilities to all downstream repos.

There are actually several other developers whom have the same issues as Tony’s in their ‘master’ github master branch; meaning they’ll bring in a bunch of empty merges if try to push to the SF git repo. Is their a command that can compare commits in repo branches; for example compare a local ‘master’ branch to the SF ‘master’ branch as a sanity check. If they are not the same, then need to rebuild the local master again before attempting a commit.

-brady