SqlServer AdoJobStore SqlParameter without text size generates pressure on server

[lucamazzanti]

The AdoJobStore implementation was recently fixed for the sched_name parameter in 3.1.0 but is still having pressure on Sql Server plan cache due to the missing SqlParameter size property for text parameters.

This generates a lot of distinct sql plans. Here some plans generated, this is the 3.0.0 version without the fix on scheduler name parameter but the issue still remains in latest:

(@triggerRepeatCount int,@triggerRepeatInterval bigint,@triggerTimesTriggered int,@triggerName nvarchar(12),@triggerGroup nvarchar(17))UPDATE QRTZ_SIMPLE_TRIGGERS SET REPEAT_COUNT = @triggerRepeatCount, REPEAT_INTERVAL = @triggerRepeatInterval, TIMES_TRIGGERED = @triggerTimesTriggered WHERE SCHED_NAME = 'scheduler' AND TRIGGER_NAME = @triggerName AND TRIGGER_GROUP = @triggerGroup

(@triggerRepeatCount int,@triggerRepeatInterval bigint,@triggerTimesTriggered int,@triggerName nvarchar(10),@triggerGroup nvarchar(10))UPDATE QRTZ_SIMPLE_TRIGGERS SET REPEAT_COUNT = @triggerRepeatCount, REPEAT_INTERVAL = @triggerRepeatInterval, TIMES_TRIGGERED = @triggerTimesTriggered WHERE SCHED_NAME = 'scheduler' AND TRIGGER_NAME = @triggerName AND TRIGGER_GROUP = @triggerGroup

As you can see without the parameter size it take the size from the value itself generating similar plans.
To watch the plan cache generated on a SqlServer:

SELECT DISTINCT dm_exec_sql_text.text, creation_time FROM sys.dm_exec_cached_plans CROSS APPLY sys.dm_exec_query_plan(plan_handle) INNER JOIN sys.dm_exec_query_stats ON dm_exec_query_stats.plan_handle = dm_exec_cached_plans.plan_handle CROSS APPLY sys.dm_exec_sql_text(dm_exec_query_stats.plan_handle) WHERE text LIKE '%qrtz%' ORDER BY dm_exec_sql_text.text DESC;

This port to a plan flooding that has a critical impact on the entire server.

To avoid that we can gather at startup the column size from the schema and set that value during parametrization, I think in SqlServerDelegate.AddCommandParameter for example.

[lahma]

Very interesting find and thank you for the detailed breakdown. Would you be interested to submit a PR to fix this?

[lucamazzanti]

Sure, let me know if you have some hints or precautions. I fork it for submit asap a PR with the fix.

[lahma]

Quick check makes me believe that it's just like you said, adding size info to invoked AddCommandParameter. I think it's easiest to add the sizes to AdoConstants.cs where the column names already are. New ones like public const int ColumnSchedulerNameSize = 120 .

AddCommandParameter(cmd, "schedulerName", schedName);

Becomes:

AddCommandParameter(cmd, "schedulerName", schedName, size: AdoConstants.ColumnSchedulerNameSize);

Hopefully we don't run to issues between databases, the sizes should be same for every schema and provider code quite the same.

[lucamazzanti]

I agree with you this is a simple solution and in the same place where the code is more binded to the schema so the first place to check in case of future schema changes. This change is supported by others engine and the sizes are the same across the different engines. Other options like read schema metadata to get the column size are too complex just for that. I will test that fix also in a real production environment where now is clearly an isssue and check the result.

[lahma]

One option could also infer some default like size 4000 when it's the case of string going in. Size would be fixed and always the same? Even though bigger than the actual database size but I guess that doesn't matter?

[lucamazzanti]

There are different sizes for each column so a better approach is to set a default value like that or nvarchar(max).
I'm checking how some ORMs treat that because I rember I applied a similar approach reading the code behind of Entity Framework, but probably it reads metadata. The only doubt is about memory allocation for a declared size like that, but testing the query plan generated it seems not changing at all, for Sql Server.

SET TRAN ISOLATION LEVEL READ UNCOMMITTED; IF OBJECT_ID('TEST_TABLE') IS NOT NULL DROP table [dbo].[TEST_TABLE]; CREATE TABLE [dbo].[TEST_TABLE] ( [SCHED_NAME] nvarchar(120) NOT NULL, [TRIGGER_NAME] nvarchar(150) NOT NULL, [TRIGGER_GROUP] nvarchar(150) NULL ); GO DECLARE @p1 NVARCHAR(4000) = 'scheduler-name'; DECLARE @p2 NVARCHAR(4000) = 'trigger-name'; DECLARE @p3 NVARCHAR(4000) = 'trigger-group'; DECLARE @sql NVARCHAR(4000) = N'insert into [dbo].[TEST_TABLE] ([SCHED_NAME],[TRIGGER_NAME],[TRIGGER_GROUP]) values (@p1,@p2,@p3)'; DECLARE @params NVARCHAR(4000) = N'@p1 nvarchar(4000),@p2 nvarchar(4000),@p3 nvarchar(4000)'; EXECUTE sys.sp_executesql @sql, @params, @p1=@p1, @p2=@p2, @p3=@p3; SELECT DISTINCT dm_exec_sql_text.text, creation_time FROM sys.dm_exec_cached_plans CROSS APPLY sys.dm_exec_query_plan(plan_handle) INNER JOIN sys.dm_exec_query_stats ON dm_exec_query_stats.plan_handle = dm_exec_cached_plans.plan_handle CROSS APPLY sys.dm_exec_sql_text(dm_exec_query_stats.plan_handle) WHERE text LIKE '%TEST_TABLE%' ORDER BY dm_exec_sql_text.text DESC;

[lahma]

I see there's now the 4000? It could be easy too hook into here:

quartznet/src/Quartz/Impl/AdoJobStore/StdAdoDelegate.cs

Lines 3357 to 3365 in effbc3e

	public virtual void AddCommandParameter(
	DbCommand cmd,
	string paramName,
	object? paramValue,
	Enum? dataType = null,
	int? size = null)
	{
	adoUtil.AddCommandParameter(cmd, paramName, paramValue, dataType, size);
	}

As SQL Server already does to detect binary:

quartznet/src/Quartz/Impl/AdoJobStore/SqlServerDelegate.cs

Lines 62 to 82 in effbc3e

	public override void AddCommandParameter(
	DbCommand cmd,
	string paramName,
	object? paramValue,
	Enum? dataType = null,
	int? size = null)
	{
	// deeded for SQL Server CE
	if (paramValue is bool && dataType == null)
	{
	paramValue = (bool) paramValue ? 1 : 0;
	}
	// varbinary support
	if (size == null && dataType != null && dataType.Equals(DbProvider.Metadata.DbBinaryType))
	{
	size = -1;
	}
	base.AddCommandParameter(cmd, paramName, paramValue, dataType, size);
	}

So it would be enough do something like if type == string && size is null => size = 4000;

[lahma]

I was reminded that NHibernate sets 4000 for nvarchar and 8000 for varchar when none supplied, haven't double-checked.

[lucamazzanti]

i'm testing if has sense a default value inside AdoUtil.AddCommandParameter for every type because it seems size is used only for variable sized types like varchar, vcarchar, varbinary and -1 is max value.

param.Size = size ?? -1;

otherwise after the param creation, the param.DbType has the real type and we can fix it only for a string type where size is not specified.

otherwise if we want to be more strict and fix it just for now in the SqlServer part we can extend the part in SqlServerDelegate for datatype null, size null and check if it works well with integer and others or check the object value type for a string type to restrict the fix (because dataType is never specified unless varbinary).

i'm not sure about performance on setting a default value (never specified in the real queries) with a max value, maybe is a memory evluation overkill i don't know the impact but i can test it.

[lahma]

I think

            if (size == null && paramValue is string)
            {
                size = 4000;
            }

would be quite safe? Data type is only set explicitly for binary if I remember rights. This could also go just to SqlServerDelegate, I'm unsure how query plans work with other DBs, do they suffer from this problem.

[lucamazzanti]

This fix in SqlServerDelegate is perfect to have a first feedback and think about the issue on other DBs in a second time.
I will check how it works Dapper\EF\NH and gather other best practices just to have another external feedback on that case.
I will keep an eye on production for memory consumption after the fix.

[lucamazzanti]

• a useful link reference from Brent Ozar on the SqlParameter issue on Plan Cache pollution.

• MSDN documentation of SqlParameter.Size usage

[lucamazzanti]

I tested NHibernate 5.1.2 version, it set a fixed default value of 4000 for nvarchar when querying a mapped column with [StringLength(50)] and the same size on database column
EXEC sp_executesql N'select this_.Id as id1_7_0_, ... from dbo.test this_ where this_.Description=@p0',N'@p0 nvarchar(4000)',@p0 = N'test';
same thing during insert and update.
I can't test Entity Framework but I clearly rember in version 6 something similar.
I tested Dapper 2.0.35 and it set a fixed default value of 4000 when not specified through parameters on a column with database size of 50 chars.
exec sp_executesql N'SELECT COUNT(*) FROM [dbo].[test] WHERE [Description] = @Name',N'@Name nvarchar(4000)',@Name=N'test'
I think we can consider a good solution set it as 4000 size as default value.

[lahma]

Great investigation, thank you for this. Out of curiosity, how big is your deployment? Trigger/job/node count-wise. I bet it requires some load to surface this.

[lucamazzanti]

tens of processes, 10 schedulers, a few hundreds of jobs with single simple triggers running clustered on a single dedicated database for quartz. Probably it is too chatty on the server also after the fix on the plan cache.
I appreciate if you want to go deeper in details in our use case, you can write me in private.
Tomorrow morning i will submit the PR with the fix.