Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Nov 15, 2021

Conversation

rlaveycal
Copy link
Contributor

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

Only create string_split if DB level doesn't have it

Signed-off-by: Richard Lavey <richard.lavey@calastone.com>
@magne
Copy link
Contributor

magne commented Nov 15, 2021

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 😄

@magne magne self-requested a review November 15, 2021 08:09
@magne magne merged commit d520670 into betr-io:master Nov 15, 2021
@arouxAgicap arouxAgicap mentioned this pull request Feb 23, 2022
@mkusmiy
Copy link

mkusmiy commented Mar 15, 2022

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:
hashicorp/terraform-plugin-sdk#67

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Race condition with String_Split causes failure
3 participants