July 29, 2015 at 8:20 am
So this post is more Cathartic than a professional question.
I do want the input of anyone that would like to comment about the content of this post.
This is a Select statement created from with these requirements:
*Paramaterized select that returns all patients with a LastName value like the provided string (parameter) and the FirstName is like the provided string (parameter)
*Return specific columns from the Patient, User, and PatientInfo tables
*Order by LastName, then FirstName
*Only Patient records where the STAFFCATEGORYPERMISSIONS.CategoryId is not null for the supplied userid (parameter)
* Also need parameters to filter on the following column values in Patient table:
STATUS, DECEASED, DELFLAG
* Need to filter on parameters supplied for following values in User table:
USERTYPE, DELFLAG, STATUS
After reading over this I was asked if a Temp Table could be used.
I said sure.
This is what was given. Needless to say it now has over 2000 execution plans for 2000 executions.
My questions are:
Is this good SQL? If you were grading it (1 to 10) what would your grade be?
Did the developer meet the requirements.
(@0 int,@1 int,@2 int,@3 int,@4 int,@5 int,@6 int,@7 int,@8 int)
select * from
( select uid , ufname , uminitial , ulname , dob , upphone , uemail , webenabled , status
, upreviousname , osuusertype , sex , registryenabled , ( voiceenabled | textenabled ) as voiceenabled
, suffix , controlno , billingalert , giventocoll , deceased
, Row_Number ( ) over ( order by ulname , ufname ) as RowCounter
from PATIENTS , USERS left outer join PATIENTINFO PINFO on PINFO . PID = USERS . UID
where PATIENTS . PID not in ( select P . PATIENTID from PATIENTCATEGORIES P left outer join STAFFCATEGORYPERMISSIONS S on P . CATEGORYID = S . CATEGORYID and S . USERID = @0 where S . CATEGORYID is null )
and STATUS = @1
and DECEASED = @2
and PATIENTS . PID = USERS . UID
and USERTYPE = @3
and DELFLAG = @4
and STATUS = @5
and DECEASED = @6
and UFNAME like '%'
and ULNAME like 'WIN%' ) Temp
where Temp . RowCounter between @7 and @8
July 29, 2015 at 9:41 pm
PHYData DBA (7/29/2015)
So this post is more Cathartic than a professional question.I do want the input of anyone that would like to comment about the content of this post.
This is a Select statement created from with these requirements:
*Paramaterized select that returns all patients with a LastName value like the provided string (parameter) and the FirstName is like the provided string (parameter)
*Return specific columns from the Patient, User, and PatientInfo tables
*Order by LastName, then FirstName
*Only Patient records where the STAFFCATEGORYPERMISSIONS.CategoryId is not null for the supplied userid (parameter)
* Also need parameters to filter on the following column values in Patient table:
STATUS, DECEASED, DELFLAG
* Need to filter on parameters supplied for following values in User table:
USERTYPE, DELFLAG, STATUS
After reading over this I was asked if a Temp Table could be used.
I said sure.
This is what was given. Needless to say it now has over 2000 execution plans for 2000 executions.
My questions are:
Is this good SQL? If you were grading it (1 to 10) what would your grade be?
Did the developer meet the requirements.
(@0 int,@1 int,@2 int,@3 int,@4 int,@5 int,@6 int,@7 int,@8 int)select * from
( select uid , ufname , uminitial , ulname , dob , upphone , uemail , webenabled , status
, upreviousname , osuusertype , sex , registryenabled , ( voiceenabled | textenabled ) as voiceenabled
, suffix , controlno , billingalert , giventocoll , deceased
, Row_Number ( ) over ( order by ulname , ufname ) as RowCounter
from PATIENTS , USERS left outer join PATIENTINFO PINFO on PINFO . PID = USERS . UID
where PATIENTS . PID not in ( select P . PATIENTID from PATIENTCATEGORIES P left outer join STAFFCATEGORYPERMISSIONS S on P . CATEGORYID = S . CATEGORYID and S . USERID = @0 where S . CATEGORYID is null )
and STATUS = @1
and DECEASED = @2
and PATIENTS . PID = USERS . UID
and USERTYPE = @3
and DELFLAG = @4
and STATUS = @5
and DECEASED = @6
and UFNAME like '%'
and ULNAME like 'WIN%' ) Temp
where Temp . RowCounter between @7 and @8
Just an opinion here...
First, unless it's a peer review BEFORE it goes to QA, you don't look at the code to see if it meets requirements. You [font="Arial Black"]test [/font]it to see if it meets requirements. Both Happy-Path and Notso-Happy-Path. According to the number of execution plans you have, I'm thinking that someone is in the process of testing it. Hopefully, those tests aren't being done in production. 😉
As for the code itself, it's the same crappy ORM-written code that comes out of many front-end development efforts. If a human actually wrote the code in the form we see above, I'd have a serious "come to Jesus" talk with that person. Both ANSI joins and older Equi-joins have been used in the same code, the best practice of using 2 part naming for the tables is not present, and the sense of proper indentation for readability is making serious sucking sounds, all of which is typical of ORM generated code.
I've also not written a lick of front-end code since 2002 (long story there) and so I may be talking out of my hat a bit but it LOOKS like this code has a possible SQL Injection problem for the first and last names because they don't appear to be parameterized (like I said, I could be wrong there). Rather, they appear to have simply been concatenated together in/by the front-end code so I'd have to give the code, the developer, and everyone else that's looked at the code up 'til now a -10 out of 10 for not picking up on that possibility.
The code does have some serious other indications of some rather large smells as to how the database may have been developed and a general lack of culture for the development and management teams. First, having all lower case or all upper case column names is just plain lazy on the part of whomever designed/implemented the tables. Since there's a mix in this code (which was generated by machine), I can only assume that there's no standards at the company, no peer reviews to enforce standards, and that the developers may have access to promote their own code to production, all of which are really, really bad things.
As for the 2000 executions plans, that's what happens when people and ORMs aren't setup for or aware of sp_ExecuteSQL.
I'm not sure I'd do paging with ROW_NUMBER, either. I've done paging in the past using nested TOPs that fairly well blows ROW_NUMBER and the use of Temp Tables out of the water for such a thing. Of course, it's difficult to do from the front-end. It would likely need a stored procedure and then you'd have to fight that battle.
Seriously... get back to the source code that spawns this mess and make sure the first and last name "parameters" aren't simply being concatenated to code because of the risk of SQL Injection that would pose.
--Jeff Moden
Change is inevitable... Change for the better is not.
Viewing 2 posts - 1 through 1 (of 1 total)
You must be logged in to reply to this topic. Login to reply