January 12, 2019 at 12:05 pm
hum.... may have misread it initially. @SQL_CODE is what contains all the calls to dbo.elast.... bad design on how it was implemented.
so back to those functions. Not sure which one is misbehaving - could be both or even others not visible
dbo.elast - if your db is always case insensitive remove the upper's from it
dbo.elast2 - confirm the indexes that exist on table XVBS_elasticity_INPUT are the ones required to cover the 2 selects on the function. and make sure that the schema of the table is explicitly set on it (I'm assuming it is a table and not a view or synonym - if a table then should be dbo.XVBS_elasticity_INPUT most likely)
better yet rewrite the whole thing so that both functions are ITVF and called only once for each combination required.
@jeff and the others - any further ideas on the udfs? the remaining code does not seem to be the issue (although it could be improved on all those selects of XVBS_RPLN_TARIFF_MAPPING_3M being replaced with a single table access)
January 12, 2019 at 12:34 pm
frederico_fonseca - Saturday, January 12, 2019 12:05 PMhum.... may have misread it initially. @SQL_CODE is what contains all the calls to dbo.elast.... bad design on how it was implemented.so back to those functions. Not sure which one is misbehaving - could be both or even others not visible
dbo.elast - if your db is always case insensitive remove the upper's from it
dbo.elast2 - confirm the indexes that exist on table XVBS_elasticity_INPUT are the ones required to cover the 2 selects on the function. and make sure that the schema of the table is explicitly set on it (I'm assuming it is a table and not a view or synonym - if a table then should be dbo.XVBS_elasticity_INPUT most likely)better yet rewrite the whole thing so that both functions are ITVF and called only once for each combination required.
@jeff and the others - any further ideas on the udfs? the remaining code does not seem to be the issue (although it could be improved on all those selects of XVBS_RPLN_TARIFF_MAPPING_3M being replaced with a single table access)
Hi francesco, i know that the code is a pure crap :(. If i run select SQL_CODE from XVBS_RPLN_TARIFF_MAPPING_3M where offer_id = 1
@SQL_CODE contains this:
round ( ( case when (a.MOU_RND_ONNET*dbo.elast('ON',Tariff_Model,@RPLNGRP)-ISNULL(a.MOU_Friend1,0)*dbo.elast('ON',Tariff_Model,@RPLNGRP) + a.MOU_RND_OFFNET*dbo.elast('OF',Tariff_Model,@RPLNGRP) + (case when a.MOU_RND_Fix <= 10 then 0 else a.MOU_RND_Fix end) + a.MOU_RND_USSD <=100) then 0 else (a.MOU_RND_ONNET*dbo.elast('ON',Tariff_Model,@RPLNGRP)-ISNULL(a.MOU_Friend1,0)*dbo.elast('ON',Tariff_Model,@RPLNGRP) + a.MOU_RND_OFFNET*dbo.elast('OF',Tariff_Model,@RPLNGRP) + (case when a.MOU_RND_Fix <= 10 then 0 else a.MOU_RND_Fix end) + a.MOU_RND_USSD - 100)*7.9 end + --100min+10add --price allnet case when (a.CNT_SMS_ONNET*dbo.elast('SM',Tariff_Model,@RPLNGRP)+a.CNT_SMS_OFFNET*dbo.elast('SM',Tariff_Model,@RPLNGRP))<=0 then 0 else (a.CNT_SMS_ONNET*dbo.elast('SM',Tariff_Model,@RPLNGRP)+a.CNT_SMS_OFFNET*dbo.elast('SM',Tariff_Model,@RPLNGRP)-0)*4.9 end + --price SMS a.CNT_MMS*17.7 + --price MMS case when a.MB_RND_DATA<=100 then 0 else (a.MB_RND_DATA-100)*0 end + --data (po preneseni bundla kupa dalsieho) (a.AMT_INT_EUROPE + a.AMT_INT_NEIGH + a.AMT_INT_WORLD + a.AMT_SMS_INT)*1.18 + (AMT_TAG_roamoutg + AMT_EUR_roamoutg + AMT_OTH_roamoutg + AMT_TAG_roaminc + AMT_EUR_roaminc + AMT_OTH_roaminc + AMT_SMS_ROAM_TAG + AMT_SMS_ROAM_EUR + AMT_SMS_ROAM_OTH + AMT_MMS_ROAM+ AMT_DATA_ROAM)*1.18 --Roaming + AMT_OTHER*1.18 + 279 ) /1.18 ,2)
January 12, 2019 at 12:45 pm
alexandermkd - Saturday, January 12, 2019 12:34 PMfrederico_fonseca - Saturday, January 12, 2019 12:05 PMhum.... may have misread it initially. @SQL_CODE is what contains all the calls to dbo.elast.... bad design on how it was implemented.so back to those functions. Not sure which one is misbehaving - could be both or even others not visible
dbo.elast - if your db is always case insensitive remove the upper's from it
dbo.elast2 - confirm the indexes that exist on table XVBS_elasticity_INPUT are the ones required to cover the 2 selects on the function. and make sure that the schema of the table is explicitly set on it (I'm assuming it is a table and not a view or synonym - if a table then should be dbo.XVBS_elasticity_INPUT most likely)better yet rewrite the whole thing so that both functions are ITVF and called only once for each combination required.
@jeff and the others - any further ideas on the udfs? the remaining code does not seem to be the issue (although it could be improved on all those selects of XVBS_RPLN_TARIFF_MAPPING_3M being replaced with a single table access)
Hi francesco, i know that the code is a pure crap :(. If i run select SQL_CODE from XVBS_RPLN_TARIFF_MAPPING_3M where offer_id = 1
@SQL_CODE contains this:
round ( ( case when (a.MOU_RND_ONNET*dbo.elast('ON',Tariff_Model,@RPLNGRP)-ISNULL(a.MOU_Friend1,0)*dbo.elast('ON',Tariff_Model,@RPLNGRP) + a.MOU_RND_OFFNET*dbo.elast('OF',Tariff_Model,@RPLNGRP) + (case when a.MOU_RND_Fix <= 10 then 0 else a.MOU_RND_Fix end) + a.MOU_RND_USSD <=100) then 0 else (a.MOU_RND_ONNET*dbo.elast('ON',Tariff_Model,@RPLNGRP)-ISNULL(a.MOU_Friend1,0)*dbo.elast('ON',Tariff_Model,@RPLNGRP) + a.MOU_RND_OFFNET*dbo.elast('OF',Tariff_Model,@RPLNGRP) + (case when a.MOU_RND_Fix <= 10 then 0 else a.MOU_RND_Fix end) + a.MOU_RND_USSD - 100)*7.9 end + --100min+10add --price allnet case when (a.CNT_SMS_ONNET*dbo.elast('SM',Tariff_Model,@RPLNGRP)+a.CNT_SMS_OFFNET*dbo.elast('SM',Tariff_Model,@RPLNGRP))<=0 then 0 else (a.CNT_SMS_ONNET*dbo.elast('SM',Tariff_Model,@RPLNGRP)+a.CNT_SMS_OFFNET*dbo.elast('SM',Tariff_Model,@RPLNGRP)-0)*4.9 end + --price SMS a.CNT_MMS*17.7 + --price MMS case when a.MB_RND_DATA<=100 then 0 else (a.MB_RND_DATA-100)*0 end + --data (po preneseni bundla kupa dalsieho) (a.AMT_INT_EUROPE + a.AMT_INT_NEIGH + a.AMT_INT_WORLD + a.AMT_SMS_INT)*1.18 + (AMT_TAG_roamoutg + AMT_EUR_roamoutg + AMT_OTH_roamoutg + AMT_TAG_roaminc + AMT_EUR_roaminc + AMT_OTH_roaminc + AMT_SMS_ROAM_TAG + AMT_SMS_ROAM_EUR + AMT_SMS_ROAM_OTH + AMT_MMS_ROAM+ AMT_DATA_ROAM)*1.18 --Roaming + AMT_OTHER*1.18 + 279 ) /1.18 ,2)
Don't get me wrong - doing the SQL like that allow for different calculations per offer ID which can be considered good design - but all those repetitive functions could/should have been implemented as itvf and called on a outer apply of the main table. This is where I consider it was badly implemented.
January 12, 2019 at 11:07 pm
frederico_fonseca - Thursday, January 10, 2019 10:59 AM@jeff - the OP has already tried the some trace flags and also had the legacy configuration turned on .
ALTER DATABASE SCOPED CONFIGURATION SET LEGACY_CARDINALITY_ESTIMATION = ON is the same as enabling TF 9481 or keeping the db on compatibility mode < 120 (it is set to 100 on this case)
This can also be seen on the plan CardinalityEstimationModelVersion="70"
so the issue may not be related to that, at least on this particular DB. But if dbo.elast udf is referring to another db it may be due to it also)
I assume the above TF is the one you were referring to.@alexandermkd can you give us the code for both this proc and udf dbo.elast - according to the plan either this udf (or another one not visible) are the culprits of the slowness. This is mentioned at least 23 times on the code (less executions)
UDF cpu time on new server was 93% of the overall time.overall proc TARIFF_SIM_MATRIX_3M should be rewritten - would not pass on my standards (and on those of many here I presume)
Based on MS's prior performance in this area, I don't trust the new On/Off stuff. 😉 And the OP didn't identify which trace flags he used.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 12, 2019 at 11:57 pm
Actually, the code is pretty good. If we look at one of the variable solutions and add some comments...
SET @ONNET_STAR_AFT =
(
SELECT CASE
--===== When t.onnet_starts is greater than all else, use it as the MAX.
-- Otherwise, we'll need to continue to find the MAX.
WHEN ISNULL( t.onnet_stars,0) >= ISNULL(p1.onnet_stars,0)
AND ISNULL( t.onnet_stars,0) >= ISNULL(p2.onnet_stars,0)
AND ISNULL( t.onnet_stars,0) >= ISNULL(p3.onnet_stars,0)
AND ISNULL( t.onnet_stars,0) >= ISNULL(p4.onnet_stars,0)
AND ISNULL( t.onnet_stars,0) >= ISNULL(p5.onnet_stars,0)
THEN ISNULL( t.onnet_stars,0)
--===== When p1.onnet_stars is greater than all else that hasn't been disqualified yet, use it as the MAX.
-- Otherwise, we'll need to continue to find the MAX.
WHEN ISNULL(p1.onnet_stars,0) >= ISNULL(p2.onnet_stars,0)
AND ISNULL(p1.onnet_stars,0) >= ISNULL(p3.onnet_stars,0)
AND ISNULL(p1.onnet_stars,0) >= ISNULL(p4.onnet_stars,0)
AND ISNULL(p1.onnet_stars,0) >= ISNULL(p5.onnet_stars,0)
THEN ISNULL(p1.onnet_stars,0)
--===== When p2.onnet_stars is greater than all else that hasn't been disqualified yet, use it as the MAX.
-- Otherwise, we'll need to continue to find the MAX.
WHEN ISNULL(p2.onnet_stars,0) >= ISNULL(p3.onnet_stars,0)
AND ISNULL(p2.onnet_stars,0) >= ISNULL(p4.onnet_stars,0)
AND ISNULL(p2.onnet_stars,0) >= ISNULL(p5.onnet_stars,0)
THEN ISNULL(p2.onnet_stars,0)
--===== When p3.onnet_stars is greater than all else that hasn't been disqualified yet, use it as the MAX.
-- Otherwise, we'll need to continue to find the MAX.
WHEN ISNULL(p3.onnet_stars,0) >= ISNULL(p4.onnet_stars,0)
AND ISNULL(p3.onnet_stars,0) >= ISNULL(p5.onnet_stars,0)
THEN ISNULL(p3.onnet_stars,0)
--===== When p4.onnet_stars is greater than all else that hasn't been disqualified yet, use it as the MAX.
-- Otherwise, we'll need to continue to find the MAX.
WHEN ISNULL(p4.onnet_stars,0) >= ISNULL(p5.onnet_stars,0)
THEN ISNULL(p4.onnet_stars,0)
--===== Since everything else has been disqualified, all that remains is p5.onnet_stars. Use it as the MAX.
ELSE ISNULL(p4.onnet_stars,0) --I believe this should actually be "ISNULL(p5.onnet_stars,0)"
END
--===== The sole purpose of all these LEFT JOINs is to "unpivot" the PROMO_ID_* columns.
-- The only thing that prevents this from being a massive CROSS JOIN is the WHERE clause which identifies
-- a single value that the tariff_id can be and assumes that only one possible row will be returned from
-- the tariff mapping table for any given offer_id.
FROM epi_tariffs t
LEFT JOIN epi_promotions p1 ON p1.id = (SELECT PROMO_ID_1 FROM XVBS_RPLN_TARIFF_MAPPING_3M WHERE offer_id = @OID)
LEFT JOIN epi_promotions p2 ON p2.id = (SELECT PROMO_ID_2 FROM XVBS_RPLN_TARIFF_MAPPING_3M WHERE offer_id = @OID)
LEFT JOIN epi_promotions p3 ON p3.id = (SELECT PROMO_ID_3 FROM XVBS_RPLN_TARIFF_MAPPING_3M WHERE offer_id = @OID)
LEFT JOIN epi_promotions p4 ON p4.id = (SELECT PROMO_ID_4 FROM XVBS_RPLN_TARIFF_MAPPING_3M WHERE offer_id = @OID)
LEFT JOIN epi_promotions p5 ON p5.id = (SELECT PROMO_ID_5 FROM XVBS_RPLN_TARIFF_MAPPING_3M WHERE offer_id = @OID)
WHERE t.TARIFF_CODE = (SELECT tariff_ID FROM XVBS_RPLN_TARIFF_MAPPING_3M WHERE offer_id = @OID)
)
;
The sole purpose of that crazy CASE statement is simply to find the Max of 6 different items. Because of its length, it looks mighty inefficient and yet nothing could be further from the truth. It's a mathematical sieve that will usually beat an equivalent UNPIVOT (or simulated one using CROSS APPLY/VALUES with a MAX, which is more efficient than an UNPIVOT by quite a bit). It (the sieve) starts to lose the race against unpivots/Max combos right around 6 items, like this one.
Also, note the error in the sieve. Look for where is say "--I believe" to find the error. Nasty copy and paste error that looks like it's been around for quite a while.
The real key here is that the execution plan (the SQLPLN file and not that God awful thing that was pasted to Brent's site) indicates that there's no index on the offer_id column of the tariff mapping table and that results in a full table scan. Since it's doing a full table scan, that also indicates there's no Clustered Index on the table, either.
I know the little Green dodah of an index suggestion is frequently wrong and can even suggest an index that already exists but have you actually checked to ensure that the indexes on both machines are identical? If they are and they're actually missing from both, then you desperately need to add the index I previously mentioned.
To further improve the performance of this code, we need to fix what the original author did not understand. You CAN set dozens of variables from the same lookup on a table in a SELECT. As it is, the original author is beatng the crap out of the table with one or more table lookups for each and every variable. Still, that will be a minor improvement compared to adding the index I spoke of. Might as well get the religion and add a carefully chosen UNIQUE clustered index to the table while you're at it.
As for that function that Frederico was talking about before? Either the wrong code was posted to Brent's site, the test tool has it, the XVBS_RPLN_TARIFF_MAPPING_3M has a non-persisted computed column in it that suddenly appeared because of the table scans instead of index seeks, or the XVBS_RPLN_TARIFF_MAPPING_3M table is actually a view or some other crazy thing and the plan read it as a table instead of what it really is (and, yeah, I've seen such a thing happen before).
So, bottom line is, check to see if the indexes are actually there because the SQLPLN file is saying they're not. Then combine all of the variable assignments into a single SELECT for an additional boost.
And let's hope the upgrade to this sight will soon make it so that people can actually copy and paste readable code instead of having to jump through flaming hoops to do so. It used to work just fine until that made the last change a couple of years ago and it's been a huge pain in the "Depends" ever since.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 13, 2019 at 3:17 am
Jeff Moden - Saturday, January 12, 2019 11:07 PMfrederico_fonseca - Thursday, January 10, 2019 10:59 AM@jeff - the OP has already tried the some trace flags and also had the legacy configuration turned on .
ALTER DATABASE SCOPED CONFIGURATION SET LEGACY_CARDINALITY_ESTIMATION = ON is the same as enabling TF 9481 or keeping the db on compatibility mode < 120 (it is set to 100 on this case)
This can also be seen on the plan CardinalityEstimationModelVersion="70"
so the issue may not be related to that, at least on this particular DB. But if dbo.elast udf is referring to another db it may be due to it also)
I assume the above TF is the one you were referring to.@alexandermkd can you give us the code for both this proc and udf dbo.elast - according to the plan either this udf (or another one not visible) are the culprits of the slowness. This is mentioned at least 23 times on the code (less executions)
UDF cpu time on new server was 93% of the overall time.overall proc TARIFF_SIM_MATRIX_3M should be rewritten - would not pass on my standards (and on those of many here I presume)
Based on MS's prior performance in this area, I don't trust the new On/Off stuff. 😉 And the OP didn't identify which trace flags he used.
perhaps - but explain plan is clearly stating it used the old CE 🙂
And from my own experience setting the database configuration does work.
January 13, 2019 at 5:35 am
Dear friends i want to say big tnx for all effort and help about my problem. It is clear that design is really bad and i will try to escalate this problem to our development team or i will try to find some vendor which gonna recompile whole code.
I would once again to like to thank you for all of ur effort.
BR
January 13, 2019 at 12:50 pm
frederico_fonseca - Sunday, January 13, 2019 3:17 AMJeff Moden - Saturday, January 12, 2019 11:07 PMfrederico_fonseca - Thursday, January 10, 2019 10:59 AM@jeff - the OP has already tried the some trace flags and also had the legacy configuration turned on .
ALTER DATABASE SCOPED CONFIGURATION SET LEGACY_CARDINALITY_ESTIMATION = ON is the same as enabling TF 9481 or keeping the db on compatibility mode < 120 (it is set to 100 on this case)
This can also be seen on the plan CardinalityEstimationModelVersion="70"
so the issue may not be related to that, at least on this particular DB. But if dbo.elast udf is referring to another db it may be due to it also)
I assume the above TF is the one you were referring to.@alexandermkd can you give us the code for both this proc and udf dbo.elast - according to the plan either this udf (or another one not visible) are the culprits of the slowness. This is mentioned at least 23 times on the code (less executions)
UDF cpu time on new server was 93% of the overall time.overall proc TARIFF_SIM_MATRIX_3M should be rewritten - would not pass on my standards (and on those of many here I presume)
Based on MS's prior performance in this area, I don't trust the new On/Off stuff. 😉 And the OP didn't identify which trace flags he used.
perhaps - but explain plan is clearly stating it used the old CE 🙂
And from my own experience setting the database configuration does work.
Thanks for the feedback, Frederico. I didn't do the deep dive into the Execution Plan that I should have to recognize it was, in fact, using the old CE. For me, your explanation is positive proof that the new On/Off stuff does work and you've saved me some good bit of testing time. Thanks.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 13, 2019 at 12:53 pm
alexandermkd - Sunday, January 13, 2019 5:35 AMDear friends i want to say big tnx for all effort and help about my problem. It is clear that design is really bad and i will try to escalate this problem to our development team or i will try to find some vendor which gonna recompile whole code.
I would once again to like to thank you for all of ur effort.BR
It doesn't answer the question though. Does the index I spoke of exist or not exist on both machines? It may save you some time on resolving the performance issue that you're currently experiencing, which would get you out of the woods at least in the short term.
{Edit} Sorry... long thread and I missed where you said "yes, tried with missing index too (table has 96 rows so index dosent help much)". Please disregard my last on the index.
--Jeff Moden
Change is inevitable... Change for the better is not.
Viewing 9 posts - 16 through 23 (of 23 total)
You must be logged in to reply to this topic. Login to reply