betr-io / terraform-provider-mssql

Terraform provider for Microsoft SQL Server
https://registry.terraform.io/providers/betr-io/mssql/latest
MIT License
35 stars 28 forks source link

Only create string_split if DB level doesn't have it #24

Closed rlaveycal closed 2 years ago

rlaveycal commented 2 years ago

Fixes #23

Rather than creating the split function in all databases, only create it if it isn't present. This avoids

  1. Unnecessary 3rd party code in a database
  2. A race condition where 2 users being created at the same time result in the second one failing due to the split function being attempted to be created twice.

NB: I haven't compiled this code but have tested the logic in a SQL Azure database and adjusting the compatibility level.

alter database test set compatibility_level = 120
go
if exists (select compatibility_level FROM sys.databases where name = db_name() and compatibility_level < 130)
begin
    print 'will fail'
    SELECT value FROM STRING_SPLIT('1,2', ',')
end

alter database test set compatibility_level = 150
go
if exists (select compatibility_level FROM sys.databases where name = db_name() and compatibility_level < 130)
begin
    print ''
end
else
begin
    print 'will work'
    SELECT value FROM STRING_SPLIT('1,2', ',')
end
magne commented 2 years ago

Thanks for the PR, @rlaveycal. Do you have a similar fix for #21? You seem to have more control over SQL Server versions than me :smile:

mkusmiy commented 2 years ago

IMHO this code change does not change behavior logic, and as such - does not eliminate "race condition where 2 users being created at the same time" as already proven in issue #31 because it does not address the root of the issue before changes - code was trying to determine does such function already exist in the DB and if not - create it after changes - code determines is db compatibility level <130 and only in that case creates the function (because it already exists in MS SQL since 130) race condition appears when two or more resources are being created (terraform does it in parallel) and there was no function on the DB previously - then all the resources running in parallel are trying to create function with the same name. have no idea how to implement single-line fix without much refactoring possible easy workarounds: -- just document that when creating more than one user - depends across user resources is needed starting from the 2nd resource; -- use paultyng/sql/sql_migrate provider to create String_Split function with depends on it from mssql_user resource (probably overkill); harder: -- create String_Split_SomeRandomChars function using randomize and drop it once resource is created; -- refactor cursor not to use this function - have no clue how, since it's already complex dynamic SQL with complex cursor in it; AFAIK there is no way to disable parallelism only for selected type of resource - there is feature request for something similar since 2016 and still not implemented :( more on it here: https://github.com/hashicorp/terraform-plugin-sdk/issues/67