July 22, 2015 at 11:41 pm
In some cases the best solution might be to split very complex query apart using temp tables. This query is definitely not an easy one for optimizer and it tells about it in the plan as Reason for Early Termination - TimeOut.
The one thing you can try, if you have time for that, is to create this filtered statistics. It may help to guide optimizer away from the bad query plan.
CREATE STATISTICS stat_dbo_Client_DisciplineID ON [dbo].[Client]([DisciplineID])
WHERE ClientStatusID>0 WITH FULLSCAN
July 23, 2015 at 1:43 am
DavidDroog (7/22/2015)
Hi Guysthanks for the replies.
...
@chrism-2 - Yes, it does look complicated and there may be a couple of unecessary joins, but it works fine without the addition of the OwnershipRoleDiscipline table. I should also point out that the actual query is a dynamic one, this query is the output of a section of it that after much pain I have narrowed down to be the problem.
...
Thanks again for the feedback everyone, I really appreciate it.
DavidDroog (7/22/2015)
Hi Guys... It is completely non-sensical, "bad" and "unreadable", but the query is down to under a second now....
How to write TSQL, 101:
1. Make it work - the query does what it should and nothing else, referencing only the objects it needs to.
2. Make it fast - you're working on this, hampered by half-complete step 1.
3. Make it pretty - that's not just code formatting and documentation, it includes abiding by standards and conventions so that others can understand the intent of your code by looking at it.
This query is ghastly as it stands and it's no surprise that SQL Server balks and times out on optimising both the slow and the fast version. Now that you've got the attention of some highly respected masters of TSQL (not me, I'm just a coder), you might wish to consider the options you have, for instance an assisted rewrite of this query. You'd get an exemplar to refer to and you'd pick up some useful lessons on coding too. You'd have to be daft to let go an opportunity like this.
For fast, accurate and documented assistance in answering your questions, please read this article.
Understanding and using APPLY, (I) and (II) Paul White
Hidden RBAR: Triangular Joins / The "Numbers" or "Tally" Table: What it is and how it replaces a loop Jeff Moden
July 23, 2015 at 2:17 am
@Op instead of using temptables , you could try using recompile hint and see if the performance improves. Using a temptable just to avoid problems with the base table when creating the execution plan seems to be a way address the symptom and not the root cause. Chances are it will come back to bite you at a later stage , most likely with tempdb being the problem now.
Let me know how if goes.
My two cents on the side discussion-
**Avoid dynamic queries where possbile
**if you want to make code readable add comments the rest is upto the developer to understand the code and developer worth his salt will be able to read the code regardless of the filters in the join predicate or where condition. as for sql its doesnt care.
July 23, 2015 at 8:10 am
Just an FYI, your query is so complex that the optimizer timed-out in both the fast query and the slow query while attempting to generate an execution plan. This means it probably didn't find the optimum plan for your query.
One thing you should understand, set based does not always mean all in one query. Sometimes it is actually better to break up a complex query such as yours into smaller steps using temporary tables and then query the temporary tables to get the final result. Breaking the query into smaller pieces can also make the overall query easier to understand and maintain.
July 23, 2015 at 9:15 am
Also, as I try to make sense of the multiple joins I am starting see some circular joins occurring as you query the same table multiple times.
To be honest, if I were to walk into your position and this is what I had to work with, it would be difficult to maintain. Hopefully you have some documentation some where that explains the logic behind what this query is doing as I am having some difficulty trying to figure it out. I think I am seeing some redundant joins with some of the tables that are joined to multiple times.
If you would really like to improve the performance and scalability of this query you may want to consider a rewrite, perhaps using a divide and conquer process and temporary tables.
July 23, 2015 at 9:29 am
Here are the tables used in the FROM clause and the number times each is referenced:
Table Name References
[Profile].[ProfileValueColour] 2
[Profile].[ClientProductProfile] 4
[dbo].[Product] 2
[Profile].[ProfileProductRule] 1
[dbo].[OwnershipPosition] 1
[dbo].[CompanyPosition] 1
[dbo].[OwnershipRole] 1
[dbo].[Geography] 3
[dbo].[GeographyHierarchy] 2
[dbo].[Address] 1
[dbo].[Client] 1
[dbo].[Person] 1
[dbo].[OwnershipRoleDiscipline] 1
July 23, 2015 at 9:39 am
Jayanth_Kurup (7/23/2015)
@Op instead of using temptables , you could try using recompile hint and see if the performance improves. Using a temptable just to avoid problems with the base table when creating the execution plan seems to be a way address the symptom and not the root cause. Chances are it will come back to bite you at a later stage , most likely with tempdb being the problem now.Let me know how if goes.
My two cents on the side discussion-
**Avoid dynamic queries where possbile
**if you want to make code readable add comments the rest is upto the developer to understand the code and developer worth his salt will be able to read the code regardless of the filters in the join predicate or where condition. as for sql its doesnt care.
Reading the code isn't the problem. Comprehending what it is doing is the issue. A cursory review of the code it appears that there are circular references occurring and possibly some redundant joins as well. Without the DDL for the tables and some sample data for the tables (some that would meet the criteria of the query and some that doesn't) it is really difficult to know what it is actually trying to accomplish.
Also, set based does not necessarily mean all done in one query. Sometimes you need to break a complex query into smaller pieces using temporary tables to improve the performance of a complex query.
July 23, 2015 at 11:30 am
Jayanth_Kurup (7/23/2015)
@Op instead of using temptables , you could try using recompile hint and see if the performance improves. Using a temptable just to avoid problems with the base table when creating the execution plan seems to be a way address the symptom and not the root cause. Chances are it will come back to bite you at a later stage , most likely with tempdb being the problem now.
Using temp tables to overcome SQL Server statistics and query optimization limitations is a pretty standard practice as long as you know that there is no other ways to simplify the query.
Recompile hint only helps in very specific scenarios, for example if you are using table variables in a query or if you have issues with parameter sniffing or if for different parameters you want to omit some of the WHERE clauses and basically generate totally different execution plans for the same query. I see nothing like this in OP's query, so putting RECOMPILE option will only force server to spend resources with no evident benefits.
July 23, 2015 at 9:04 pm
Hi All
Surprised this thread was still going when logged in this morning. ChrisM - I didn't realise people were actually offering to optimise my query, but if that is the case I gratefully accept for 2 reasons.
1. As a developer I am always on the look-out for better ways of doing things and would be very happy if I was shown the light.
2. I am somewhat cynical that some of more idealistic statements can actually survive the real world (eg no dynamic sql, no temp tables). I know my code has been universally bagged, but I actually think it is pretty good and it performs a complex function accurately and reasonably quickly. I therefore would like to issue a challenge to anyone who believes they can improve on it to submit their code here (by improve I essentially mean speed which is quantifiable - changing join types etc with no performance improvement is pointless as far as I am concerned).
If anyone is willing to take the challenge I will post a database with some sample data (I assume I can attach a bak file here).
Now the code I submitted before was just the output from a subquery for the full proc, so if you think that was complex wait until you see the full thing which is posted below, however I am posting a requirement description so you do not have to follow my code at all, just re-produce the results from my beast. I should also state this is a real world proc in development, so has not gone through testing yet, but I do not anticipate it to change much (unless as stated someone shows me the light!)
So who is up for the challenge? - I will quite happily eat humble pie if my cynicism is proven false.
Regards
David
Requirements:
A dynamic screen in our in-house app displays all potential targets and existing targets for a given representative and a given geographical area (both passed in as params to the query).
The proc itself is dynamic, and displays a list of clients that is specific to
1. the geography entered
2. the disciplines the reps own as defined by the OwnershipRoleDiscipline table
3. Having high enough profiles for products defined for the reps ownershiprole or being an existing target for one of the rep products .
the screen itself displays the client details as well as profile information for products linked to their role (Profile.ProfileProductRule table) and targets associated with their role (OwnershipTargetRule table). the reason the proc is dynamic is because different sets of products and profiles (and therefore different columns displayed) are associated with different ownershiproles.
My Query below:
create PROC [dbo].[TargeterChallengeProc] --92, 1248
@OwnershipPositionID INT,
@GeographyID INT
as
DECLARE
@colTargets AS VARCHAR(MAX),
@colPointsHeaders AS VARCHAR(MAX),
@colPointsHeaders2 AS VARCHAR(MAX),
@colProfileProducts AS VARCHAR(MAX),
@colProfileProductsHeaders AS VARCHAR(MAX),
@colProfileProductsHeaders2 AS VARCHAR(MAX),
@query AS VARCHAR(MAX),
@OwnershipRoleID AS VARCHAR(10),
@GeographyHierarchyID AS VARCHAR(10)
--SET @ownershippositionid = 94
SET @ownershiproleid = (SELECT op.ownershiproleid FROM dbo.OwnershipPosition op JOIN dbo.OwnershipRole owr ON op.OwnershipRoleID = owr.OwnershipRoleID AND owr.RoleTypeID = 1 WHERE op.OwnershipPositionID = @ownershippositionID)
SET @GeographyHierarchyID = (SELECT GeographyHierarchyTypeID FROM dbo.OwnershipRole WHERE OwnershipRoleID = @OwnershipRoleID)
SET @colTargets = STUFF((SELECT distinct ',' + QUOTENAME(t.Description)
FROM dbo.OwnershipTargetRule otr
JOIN dbo.Target t
ON t.TargetID = otr.TargetID
WHERE otr.OwnershipRoleID = @OwnershipRoleID
FOR XML PATH(''), TYPE
).value('.', 'VARCHAR(MAX)')
,1,1,'')
SET @colPointsHeaders = STUFF((SELECT distinct ',' + QUOTENAME(p.Description) + ' as ' + QUOTENAME(p.description + ' Points')
FROM Profile.ProfileProductRule ppr
JOIN dbo.Product p
ON p.ProductID = ppr.ProductID
WHERE ppr.OwnershipRoleID = @OwnershipRoleID
FOR XML PATH(''), TYPE
).value('.', 'VARCHAR(MAX)')
,1,1,'')
SET @colPointsHeaders2 = STUFF((SELECT distinct ',' + QUOTENAME(p.description + ' Points')
FROM Profile.ProfileProductRule ppr
JOIN dbo.Product p
ON p.ProductID = ppr.ProductID
WHERE ppr.OwnershipRoleID = @OwnershipRoleID
FOR XML PATH(''), TYPE
).value('.', 'VARCHAR(MAX)')
,1,1,'')
SET @colProfileProducts = STUFF((SELECT distinct ',' + QUOTENAME(p.Description)
FROM Profile.ProfileProductRule ppr
JOIN dbo.Product p
ON p.ProductID = ppr.ProductID
WHERE ppr.OwnershipRoleID = @OwnershipRoleID
FOR XML PATH(''), TYPE
).value('.', 'VARCHAR(MAX)')
,1,1,'')
SET @colProfileProductsHeaders2 = STUFF((SELECT distinct ',' + QUOTENAME(p.description + ' Colour')
FROM Profile.ProfileProductRule ppr
JOIN dbo.Product p
ON p.ProductID = ppr.ProductID
WHERE ppr.OwnershipRoleID = @OwnershipRoleID
FOR XML PATH(''), TYPE
).value('.', 'VARCHAR(MAX)')
,1,1,'')
SET @colProfileProductsHeaders = STUFF((SELECT distinct ',' + QUOTENAME(p.Description) + ' as ' + QUOTENAME(p.description + ' Colour')
FROM Profile.ProfileProductRule ppr
JOIN dbo.Product p
ON p.ProductID = ppr.ProductID
WHERE ppr.OwnershipRoleID = @OwnershipRoleID
FOR XML PATH(''), TYPE
).value('.', 'VARCHAR(MAX)')
,1,1,'')
SET @query = 'SELECT cpa.ClientID, p2.Description AS Product, ppc2.ProfileColour, c.DisciplineID, owr.OwnershipRoleID, g2.GeographyID INTO #tmp
FROM profile.ProfileValueColour ppc
JOIN Profile.ClientProductProfile cpa
ON cpa.ProfileValue = ppc.Actual
AND cpa.ProfileTypeID = 13
AND ppc.ProductID = cpa.ProductID
JOIN Profile.ClientProductProfile cpp
ON cpp.ProductID = cpa.ProductId
AND cpp.ProfileValue = ppc.Potential
AND cpp.ProductID = ppc.ProductID
AND cpa.clientid = cpp.ClientID
AND cpp.ProfileTypeID = 14
JOIN dbo.Product p
ON p.ProductID = ppc.ProductID
join Profile.ProfileProductRule ppr
on ppc.ownershiproleid = ppr.ownershiproleid
and ppc.productid = ppr.productid
AND ppc.ForDisplay = 1
AND ppc.ownershiproleid = ' + CAST(@OwnershipRoleID AS VARCHAR(10)) + '
JOIN Profile.ClientProductProfile cpa2
ON cpa.clientid = cpa2.ClientID
AND cpa2.ProfileTypeID = 13
JOIN Profile.ClientProductProfile cpp2
ON cpa2.clientid = cpp2.ClientID
AND cpp2.ProfileTypeID = 14
JOIN profile.ProfileValueColour ppc2
ON ppc2.ProductID = cpa2.ProductID
AND ppc2.ProductID = cpp2.ProductID
AND cpp2.ProfileValue = ppc2.Potential
AND cpa2.ProfileValue = ppc2.Actual
AND cpp2.ProductID = ppc2.ProductID
AND ppc2.OwnershipRoleID = ' + CAST(@OwnershipRoleID AS VARCHAR(10)) + '
JOIN product p2
ON ppc2.ProductID = p2.ProductID
JOIN dbo.OwnershipPosition op
ON op.OwnershipRoleID = ppc.OwnershipRoleID
JOIN dbo.CompanyPosition cp
ON cp.CompanyPositionID = op.CompanyPositionID
AND op.OwnershipPositionID = ' + CAST(@OwnershipPositionID AS VARCHAR(10)) + '
JOIN dbo.OwnershipRole owr
ON owr.OwnershipRoleID = op.OwnershipRoleID
JOIN dbo.Geography g1
ON g1.GeographyID = cp.GeographyID
AND g1.GeographyTypeID = 6
JOIN dbo.GeographyHierarchy gh1
ON g1.GeographyID = gh1.ParentGeographyID
AND gh1.GeographyHierarchyTypeID = owr.GeographyHierarchyTypeID
JOIN dbo.Geography g2
ON g2.GeographyID = gh1.ChildGeographyID
AND g2.GeographyTypeID = 7
JOIN dbo.GeographyHierarchy gh2
ON g2.GeographyID = gh2.ParentGeographyID
AND gh2.GeographyHierarchyTypeID = owr.GeographyHierarchyTypeID
JOIN dbo.Geography g3
ON g3.GeographyID = gh2.ChildGeographyID
AND g3.GeographyTypeID = 8
JOIN dbo.Address a
ON a.GeographyBrickReferenceID = g3.ReferenceID
JOIN client c
ON c.PrimaryAddressId = a.AddressID
AND a.AddressStatusID > 0
AND c.ClientStatusID > 0
JOIN dbo.Person per
ON per.PersonID = c.ClientID
AND per.StatusID > 0
AND cpa.clientid = c.ClientID
--high profile clients
SELECT distinct c.ClientID, dn.DisplayName as [Client Name], d.description as discipline, a.BusinessName, a.StreetAddress, a.Locality, g2.Description AS SRA,
' + @colPointsHeaders2 + ',' + @colProfileProductsHeaders2 + ',' + @colTargets + '
FROM dbo.OwnershipPosition op
JOIN dbo.CompanyPosition cp
ON cp.CompanyPositionID = op.CompanyPositionID
AND op.OwnershipPositionID = ' + CAST(@OwnershipPositionID AS VARCHAR(10)) + '
JOIN dbo.OwnershipRole owr
ON owr.OwnershipRoleID = op.OwnershipRoleID
JOIN dbo.Geography g1
ON g1.GeographyID = cp.GeographyID
AND g1.GeographyTypeID = 6
JOIN dbo.GeographyHierarchy gh1
ON g1.GeographyID = gh1.ParentGeographyID
AND gh1.GeographyHierarchyTypeID = owr.GeographyHierarchyTypeID
JOIN dbo.Geography g2
ON g2.GeographyID = gh1.ChildGeographyID
AND g2.GeographyTypeID = 7
JOIN dbo.GeographyHierarchy gh2
ON g2.GeographyID = gh2.ParentGeographyID
AND gh2.GeographyHierarchyTypeID = owr.GeographyHierarchyTypeID
JOIN dbo.Geography g3
ON g3.GeographyID = gh2.ChildGeographyID
AND g3.GeographyTypeID = 8
JOIN dbo.Address a
ON a.GeographyBrickReferenceID = g3.ReferenceID
JOIN (SELECT * FROM dbo.GeoTreeDownWithID(' + CAST(@GeographyID AS VARCHAR(10))+ ',' + CAST(@GeographyHierarchyID AS VARCHAR(10)) + ') WHERE StatusID = 1 AND geographytypeid = 8) tt
ON a.GeographyBrickReferenceID = tt.referenceid
JOIN client c
ON c.PrimaryAddressId = a.AddressID
AND a.AddressStatusID > 0
AND c.ClientStatusID > 0
join discipline d
on c.disciplineid = d.disciplineid
JOIN dbo.Person per
ON per.PersonID = c.ClientID
AND per.StatusID > 0
JOIN dbo.vPersonDisplayName dn
ON dn.PersonID = per.PersonID
JOIN dbo.OwnershipRoleDiscipline ord
ON ord.DisciplineID = c.DisciplineID
AND ord.OwnershipRoleID = owr.OwnershipRoleID
AND ord.SRAGeographyID = g2.GeographyID
LEFT OUTER JOIN
(SELECT clientid, ' + @colTargets + '
FROM
(SELECT ct.clientid, t.Description AS Target, ct.clientspecialisttargetid AS isTarget
FROM clientspecialisttarget ct
JOIN dbo.OwnershipPosition op
ON op.OwnershipPositionID = ct.OwnershipPositionID
and ct.targetstatusid = 1
JOIN dbo.OwnershipRole owr
ON owr.OwnershipRoleID = op.OwnershipRoleID
JOIN dbo.OwnershipProductRule opr
ON opr.OwnershipRoleID = owr.OwnershipRoleID
AND op.OwnershipPositionID = ' + CAST(@OwnershipPositionID AS VARCHAR(10)) + '
JOIN target t
ON ct.TargetID = t.TargetID) cst
PIVOT
(MIN(isTarget) FOR target IN (' + @colTargets + ')
) piv
) cst
ON c.ClientID = cst.ClientID
LEFT OUTER JOIN
(SELECT ClientID, ' + @colPointsHeaders + '
from
(SELECT cpa.ClientID, p.Description AS Product, ppv.Points
FROM profile.profilevaluerule ppv
JOIN Profile.ClientProductProfile cpa
ON cpa.ProductID = ppv.ProductId
AND cpa.ProfileTypeID = 13
JOIN Profile.ClientProductProfile cpp
ON cpp.ProductID = ppv.ProductId
JOIN profile.profileProductRule ppr
ON ppr.OwnershipRoleID = ppv.OwnershipRoleId
JOIN dbo.Product p
ON p.ProductID = ppr.ProductID
AND p.ProductID = cpa.ProductID
AND p.ProductID = cpp.ProductID
AND cpp.ProfileValue = ppv.Potential
AND cpa.ProfileValue = ppv.Actual
AND cpa.clientid = cpp.ClientID
AND cpp.ProfileTypeID = 14
AND ppv.ownershiproleid = ' + CAST(@OwnershipRoleID AS VARCHAR(10)) + '
and ppv.productid = ppr.productid
) cvp
PIVOT
(max(Points) FOR Product IN (' + @colProfileProducts + ')
) pv
) prf
ON prf.ClientID = C.ClientID
JOIN
(SELECT ClientID, ' + @colProfileProductsHeaders + '
from
(SELECT ClientID, Product, ProfileColour
FROM #tmp a
JOIN dbo.OwnershipRoleDiscipline ord
ON ord.DisciplineID = a.DisciplineID
AND ord.OwnershipRoleID = a.OwnershipRoleID
AND ord.SRAGeographyID = a.GeographyID) cvp
PIVOT
(max(ProfileColour) FOR Product IN (' + @colProfileProducts + ')
) pv
) pc
ON pc.ClientID = C.ClientID
UNION
--targeted clients
SELECT DISTINCT c.ClientID, dn.DisplayName AS [Client Name], d.Description AS Discipline, a.BusinessName, a.StreetAddress, a.Locality, g2.Description AS SRA,
' + @colPointsHeaders2 + ',' + @colProfileProductsHeaders2 + ',' + @colTargets + '
FROM dbo.OwnershipPosition op
JOIN dbo.CompanyPosition cp
ON cp.CompanyPositionID = op.CompanyPositionID
AND op.OwnershipPositionID = 92
JOIN dbo.OwnershipRole owr
ON owr.OwnershipRoleID = op.OwnershipRoleID
JOIN dbo.Geography g1
ON g1.GeographyID = cp.GeographyID
AND g1.GeographyTypeID = 6
JOIN dbo.GeographyHierarchy gh1
ON g1.GeographyID = gh1.ParentGeographyID
AND gh1.GeographyHierarchyTypeID = owr.GeographyHierarchyTypeID
JOIN dbo.Geography g2
ON g2.GeographyID = gh1.ChildGeographyID
AND g2.GeographyTypeID = 7
JOIN dbo.GeographyHierarchy gh2
ON g2.GeographyID = gh2.ParentGeographyID
AND gh2.GeographyHierarchyTypeID = owr.GeographyHierarchyTypeID
JOIN dbo.Geography g3
ON g3.GeographyID = gh2.ChildGeographyID
AND g3.GeographyTypeID = 8
JOIN dbo.Address a
ON a.GeographyBrickReferenceID = g3.ReferenceID
JOIN (SELECT * FROM dbo.GeoTreeDownWithID(' + CAST(@GeographyID AS VARCHAR(10))+ ',' + CAST(@GeographyHierarchyID AS VARCHAR(10)) + ') WHERE StatusID = 1 AND geographytypeid = 8) tt
ON a.GeographyBrickReferenceID = tt.referenceid
JOIN client c
ON c.PrimaryAddressId = a.AddressID
AND a.AddressStatusID > 0
AND c.ClientStatusID > 0
join discipline d
on c.disciplineid = d.disciplineid
JOIN dbo.Person per
ON per.PersonID = c.ClientID
AND per.StatusID > 0
JOIN dbo.vPersonDisplayName dn
ON dn.PersonID = per.PersonID
JOIN dbo.OwnershipRoleDiscipline ord
ON ord.DisciplineID = c.DisciplineID
AND ord.OwnershipRoleID = owr.OwnershipRoleID
AND ord.SRAGeographyID = g2.GeographyID
JOIN
(SELECT clientid, ' + @colTargets + '
FROM
(SELECT ct.clientid, t.Description AS Target, ct.clientspecialisttargetid AS isTarget
FROM clientspecialisttarget ct
JOIN dbo.OwnershipPosition op
ON op.OwnershipPositionID = ct.OwnershipPositionID
and ct.targetstatusid = 1
JOIN dbo.OwnershipRole owr
ON owr.OwnershipRoleID = op.OwnershipRoleID
JOIN dbo.OwnershipProductRule opr
ON opr.OwnershipRoleID = owr.OwnershipRoleID
AND op.OwnershipPositionID = ' + CAST(@OwnershipPositionID AS VARCHAR(10)) + '
JOIN target t
ON ct.TargetID = t.TargetID) cst
PIVOT
(MIN(isTarget) FOR target IN (' + @colTargets + ')
) piv
) cst
ON c.ClientID = cst.ClientID
LEFT OUTER JOIN
(SELECT ClientID, ' + @colPointsHeaders + '
from
(SELECT cpa.ClientID, p.Description AS Product, ppv.Points
FROM profile.profilevaluerule ppv
JOIN Profile.ClientProductProfile cpa
ON cpa.ProductID = ppv.ProductId
AND cpa.ProfileTypeID = 13
JOIN Profile.ClientProductProfile cpp
ON cpp.ProductID = ppv.ProductId
JOIN profile.profileProductRule ppr
ON ppr.OwnershipRoleID = ppv.OwnershipRoleId
JOIN dbo.Product p
ON p.ProductID = ppr.ProductID
AND p.ProductID = cpa.ProductID
AND p.ProductID = cpp.ProductID
AND cpp.ProfileValue = ppv.Potential
AND cpa.ProfileValue = ppv.Actual
AND cpa.clientid = cpp.ClientID
AND cpp.ProfileTypeID = 14
AND ppv.ownershiproleid = ' + CAST(@OwnershipRoleID AS VARCHAR(10)) + '
and ppv.productid = ppr.productid
) cvp
PIVOT
(max(Points) FOR Product IN (' + @colProfileProducts + ')
) pv
) prf
ON prf.ClientID = C.ClientID
LEFT OUTER JOIN
(SELECT ClientID, ' + @colProfileProductsHeaders + '
from
(SELECT cpa.ClientID, p.Description AS Product, ppc.ProfileColour
FROM profile.ProfileValueColour ppc
JOIN Profile.ClientProductProfile cpa
ON cpa.ProfileValue = ppc.Actual
AND cpa.ProfileTypeID = 13
AND ppc.ProductID = cpa.ProductID
JOIN Profile.ClientProductProfile cpp
ON cpp.ProductID = cpa.ProductId
AND cpp.ProfileValue = ppc.Potential
AND cpp.ProductID = ppc.ProductID
AND cpa.clientid = cpp.ClientID
AND cpp.ProfileTypeID = 14
JOIN dbo.Product p
ON p.ProductID = ppc.ProductID
join Profile.ProfileProductRule ppr
on ppc.ownershiproleid = ppr.ownershiproleid
and ppc.productid = ppr.productid
WHERE ppc.ownershiproleid = ' + CAST(@OwnershipRoleID AS VARCHAR(10)) + '
) cvp
PIVOT
(max(ProfileColour) FOR Product IN (' + @colProfileProducts + ')
) pv
) pc
ON pc.ClientID = C.ClientID'
--print @query
exec(@query)
July 23, 2015 at 9:55 pm
DavidDroog (7/22/2015)
Hi DrewI am a human and I disagree. I find it much easier to follow when the clauses are with the table joins.
To each their own...
Regards
David
I find it to be about 6's either way (with the sample given).
On the other hand, I abhor the look of putting all of the criteria down in the predicate.
It's certainly nothing to quibble over imho.
There are other issues to fix with the query.
Jason...AKA CirqueDeSQLeil
_______________________________________________
I have given a name to my pain...MCM SQL Server, MVP
SQL RNNR
Posting Performance Based Questions - Gail Shaw[/url]
Learn Extended Events
July 23, 2015 at 9:59 pm
DavidDroog (7/22/2015)
Hi GuysI have figured this out. My theory is that adding the extra table somehow screwed up SQL Servers execution plan so badly no amount of indexing etc was going to help, I just had to force it to reshuffle its execution plan ( I have actually seen this sort of thing before, but not to this degree). To to this I have utilised a temp table and then joined the problematic OwnershipRoleDiscipline table to the temp table. It is completely non-sensical, "bad" and "unreadable", but the query is down to under a second now.
Lynn - sorry, but the logistics of scripting out all those tables and proving sets of usable, non sensitive data was more than I had time for.
SELECT cpa.ClientID, p2.Description AS Product, ppc2.ProfileColour, c.DisciplineID, owr.OwnershipRoleID, g2.GeographyID INTO #tmp
FROM profile.ProfileValueColour ppc
JOIN Profile.ClientProductProfile cpa
ON cpa.ProfileValue = ppc.Actual
AND cpa.ProfileTypeID = 13
AND ppc.ProductID = cpa.ProductID
JOIN Profile.ClientProductProfile cpp
ON cpp.ProductID = cpa.ProductId
AND cpp.ProfileValue = ppc.Potential
AND cpp.ProductID = ppc.ProductID
AND cpa.clientid = cpp.ClientID
AND cpp.ProfileTypeID = 14
JOIN dbo.Product p
ON p.ProductID = ppc.ProductID
join Profile.ProfileProductRule ppr
on ppc.ownershiproleid = ppr.ownershiproleid
and ppc.productid = ppr.productid
AND ppc.ForDisplay = 1
AND ppc.ownershiproleid = 1
JOIN Profile.ClientProductProfile cpa2
ON cpa.clientid = cpa2.ClientID
AND cpa2.ProfileTypeID = 13
JOIN Profile.ClientProductProfile cpp2
ON cpa2.clientid = cpp2.ClientID
AND cpp2.ProfileTypeID = 14
JOIN profile.ProfileValueColour ppc2
ON ppc2.ProductID = cpa2.ProductID
AND ppc2.ProductID = cpp2.ProductID
AND cpp2.ProfileValue = ppc2.Potential
AND cpa2.ProfileValue = ppc2.Actual
AND cpp2.ProductID = ppc2.ProductID
AND ppc2.OwnershipRoleID = 1
JOIN product p2
ON ppc2.ProductID = p2.ProductID
JOIN dbo.OwnershipPosition op
ON op.OwnershipRoleID = ppc.OwnershipRoleID
JOIN dbo.CompanyPosition cp
ON cp.CompanyPositionID = op.CompanyPositionID
AND op.OwnershipPositionID = 92
JOIN dbo.OwnershipRole owr
ON owr.OwnershipRoleID = op.OwnershipRoleID
JOIN dbo.Geography g1
ON g1.GeographyID = cp.GeographyID
AND g1.GeographyTypeID = 6
JOIN dbo.GeographyHierarchy gh1
ON g1.GeographyID = gh1.ParentGeographyID
AND gh1.GeographyHierarchyTypeID = owr.GeographyHierarchyTypeID
JOIN dbo.Geography g2
ON g2.GeographyID = gh1.ChildGeographyID
AND g2.GeographyTypeID = 7
JOIN dbo.GeographyHierarchy gh2
ON g2.GeographyID = gh2.ParentGeographyID
AND gh2.GeographyHierarchyTypeID = owr.GeographyHierarchyTypeID
JOIN dbo.Geography g3
ON g3.GeographyID = gh2.ChildGeographyID
AND g3.GeographyTypeID = 8
JOIN dbo.Address a
ON a.GeographyBrickReferenceID = g3.ReferenceID
JOIN client c
ON c.PrimaryAddressId = a.AddressID
AND a.AddressStatusID > 0
AND c.ClientStatusID > 0
AND cpa.clientid = c.ClientID
JOIN dbo.Person per
ON per.PersonID = c.ClientID
AND per.StatusID > 0
SELECT ClientID, Product, ProfileColour
FROM #tmp a
JOIN dbo.OwnershipRoleDiscipline ord
ON ord.DisciplineID = a.DisciplineID
AND ord.OwnershipRoleID = a.OwnershipRoleID
AND ord.SRAGeographyID = a.GeographyID
How many of these objects are views and not tables?
How many of those views are referencing other views?
Jason...AKA CirqueDeSQLeil
_______________________________________________
I have given a name to my pain...MCM SQL Server, MVP
SQL RNNR
Posting Performance Based Questions - Gail Shaw[/url]
Learn Extended Events
July 23, 2015 at 10:17 pm
Looking over the code and the execution plans, it seems to me that the query is waaaaay over-complicated.
Several of the joins appear to be unnecessary and can be handled properly via case statements or even a subquery.
It is very hard to tell if that is the case without some sort of sample data. It would only take a handful of records in order to validate a different query is getting the correct results and if that query appears to have a better execution plan.
Jason...AKA CirqueDeSQLeil
_______________________________________________
I have given a name to my pain...MCM SQL Server, MVP
SQL RNNR
Posting Performance Based Questions - Gail Shaw[/url]
Learn Extended Events
July 24, 2015 at 2:59 am
Hi Jason
No views, just tables and 1 function (geotreedownwithID)
So is anyone accepting the challenge?
Regards
David
July 24, 2015 at 5:50 am
DavidDroog (7/23/2015)
Hi AllSurprised this thread was still going when logged in this morning. ChrisM - I didn't realise people were actually offering to optimise my query, but if that is the case I gratefully accept for 2 reasons.
1. As a developer I am always on the look-out for better ways of doing things and would be very happy if I was shown the light.
2. I am somewhat cynical that some of more idealistic statements can actually survive the real world (eg no dynamic sql, no temp tables). I know my code has been universally bagged, but I actually think it is pretty good and it performs a complex function accurately and reasonably quickly. I therefore would like to issue a challenge to anyone who believes they can improve on it to submit their code here (by improve I essentially mean speed which is quantifiable - changing join types etc with no performance improvement is pointless as far as I am concerned).
If anyone is willing to take the challenge I will post a database with some sample data (I assume I can attach a bak file here).
Now the code I submitted before was just the output from a subquery for the full proc, so if you think that was complex wait until you see the full thing which is posted below, however I am posting a requirement description so you do not have to follow my code at all, just re-produce the results from my beast. I should also state this is a real world proc in development, so has not gone through testing yet, but I do not anticipate it to change much (unless as stated someone shows me the light!)
So who is up for the challenge? - I will quite happily eat humble pie if my cynicism is proven false.
...
Absolutely. Can you make a start by providing these three queries, please:
1.Clients with “disciplines” filtered by geography using the geography parameter. This query should reference client, client address and geography tables plus those required to identify "disciplines". If any other tables are referenced in the query then provide an explanation. If any tables are referenced more than once then provide a full justification.
2.Identify how to distinguish between existing and potential targets, but keep this separate from query 1 for now. It might be as simple as a grab from sales tables using clients identified by Query 1.
3.One rep with “disciplines” and products including info to determine “high enough product profile”, chosen using the rep parameter. If any table is referenced more than once in this query, then provide a full justification for it.
For fast, accurate and documented assistance in answering your questions, please read this article.
Understanding and using APPLY, (I) and (II) Paul White
Hidden RBAR: Triangular Joins / The "Numbers" or "Tally" Table: What it is and how it replaces a loop Jeff Moden
July 24, 2015 at 9:09 am
ChrisM@Work (7/24/2015)
DavidDroog (7/23/2015)
Hi AllSurprised this thread was still going when logged in this morning. ChrisM - I didn't realise people were actually offering to optimise my query, but if that is the case I gratefully accept for 2 reasons.
1. As a developer I am always on the look-out for better ways of doing things and would be very happy if I was shown the light.
2. I am somewhat cynical that some of more idealistic statements can actually survive the real world (eg no dynamic sql, no temp tables). I know my code has been universally bagged, but I actually think it is pretty good and it performs a complex function accurately and reasonably quickly. I therefore would like to issue a challenge to anyone who believes they can improve on it to submit their code here (by improve I essentially mean speed which is quantifiable - changing join types etc with no performance improvement is pointless as far as I am concerned).
If anyone is willing to take the challenge I will post a database with some sample data (I assume I can attach a bak file here).
Now the code I submitted before was just the output from a subquery for the full proc, so if you think that was complex wait until you see the full thing which is posted below, however I am posting a requirement description so you do not have to follow my code at all, just re-produce the results from my beast. I should also state this is a real world proc in development, so has not gone through testing yet, but I do not anticipate it to change much (unless as stated someone shows me the light!)
So who is up for the challenge? - I will quite happily eat humble pie if my cynicism is proven false.
...
Absolutely. Can you make a start by providing these three queries, please:
1.Clients with “disciplines” filtered by geography using the geography parameter. This query should reference client, client address and geography tables plus those required to identify "disciplines". If any other tables are referenced in the query then provide an explanation. If any tables are referenced more than once then provide a full justification.
2.Identify how to distinguish between existing and potential targets, but keep this separate from query 1 for now. It might be as simple as a grab from sales tables using clients identified by Query 1.
3.One rep with “disciplines” and products including info to determine “high enough product profile”, chosen using the rep parameter. If any table is referenced more than once in this query, then provide a full justification for it.
With Chris, yes we are always up for a good challenge.:cool:
If you attach a bak, zip it first. It might be better to upload it to something like github or onedrive, share that and then post the link to it here.
Jason...AKA CirqueDeSQLeil
_______________________________________________
I have given a name to my pain...MCM SQL Server, MVP
SQL RNNR
Posting Performance Based Questions - Gail Shaw[/url]
Learn Extended Events
Viewing 15 posts - 16 through 30 (of 38 total)
You must be logged in to reply to this topic. Login to reply