Add nolock

[Elliot2718]

Adds with (nolock) to list_relations_without_caching macro. Fixes deadlock issues when running dbt tests in issue #195. Also see Slack thread here: https://getdbt.slack.com/archives/CMRMDDQ9W/p1640064902122500

Outstanding questions:

Are we sure adding nolock is acceptable here? My understanding is that each dbt test is independent and does not use the same database objects, which should make this fine. There could be something I'm missing though.

[deanja]

I am torn on this one. I am a user of dbt and SQL Server, not a dbt core or adapter developer.

The PR fixes the issue. No deadlocks when doing dbt test with 4 treads and ~150 tests, which all run in 7 seconds. Without this fix it would cause multiple deadlocks.

Like you, I'm concerned about introducing nolock to the project. It's usually better to understand if/how the application (in this case dbt) is causing the deadlocks. and try to prevent them there. It could be harder in this case because SQL Server might have its own quirks about concurrency of DDL operations and reads on information_schema.

A agree that use of nolock in this particular macro (sqlserver__list_relations_without_caching) is probably safe to use in dbt test, because the tests are independent of each other. It is probably also safe in dbt run and dbt build (which also call that macro) because they are constrained by dbt to run in DAG order. They are unlikely to be sensitive to uncommitted rows/deletes from another transaction being returned by READ_UNCOMMITTED, which is the behaviour invoked by nolock.

Are their other callers of that macro?

Do we need to consider MS db platform compatibility? nolock is supported on SQL Server and Azure SQL Database. Does dbt-sqlserver target any other MS data platforms?

[mikaelene]

Maybe this should be optional? Perhaps set in profiles.yml.

[deanja]

Good idea. And/or only use nowait when threads > 1 Either way gives users/developers a heads up that 'nowait' is used as a concession for multithreading. As profile/cli doco, or simply the presence of 'if' condition in the macro.

…

On Fri, 28 Jan 2022, 6:32 pm Mikael Ene, ***@***.***> wrote: Maybe this should be optional? Perhaps set in profiles.yml. — Reply to this email directly, view it on GitHub <#204 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANX27CR3FTXRJCRQXOWTWMTUYJA73ANCNFSM5M6VYAZQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[sdebruyn]

Needs rebase on latest master

[SportsTribution]

Hi folks,

Like you, I'm concerned about introducing nolock to the project. It's usually better to understand if/how the application (in this case dbt) is causing the deadlocks. and try to prevent them there. It could be harder in this case because SQL Server might have its own quirks about concurrency of DDL operations and reads on information_schema.

Not sure, I am of much help, but the locks seem to always be on
database_name.sys.sysschobjs
and involves more or less

select
   table_catalog as [database],
   table_name as [name],
   table_schema as [schema],
   case when table_type = 'BASE TABLE' then 'table'
when table_type = 'VIEW' then 'view'
else table_type
   end as table_type
  from [database_name].information_schema.tables
 where table_schema like 'dbt_test__audit'

and things like

USE [database_name];   if object_id ('"dbt_test__audit"."unique_table_name1_key_temp_view"','V') is not null
   begin
   drop view "dbt_test__audit"."unique_table_name1_key_temp_view"
   end

... long story short, I also cannot come up with a better solution than nolock on the list_relations_without_caching - (even though I don't know why the upper statement can not simply what that the lower one is finished. But it's getting late).
Anyways, maybe this PR could proceed?

Cheers
Hannes

[alieus]

@mikaelene @sdebruyn @dataders - any updates/suggestions on this PR?

[mikaelene]

I've not been super involved in the development of this adapter lately. But when running tests, you usually don't update any data or definitions so for that reason I think it's perfectly fine to use "with no lock". I don't know how people use this adapter these days but my aim when creating it was to implement cloudDW-like behaviour on SQL server and the other clouds doesn't use lock what I know of. In an OLTP-setting it would be a bad idea, but as for DW and querying data for tests I guess it would be ok. Looks like the CI build is failing. If someone fix that I think we can merge.

[sdebruyn]

I managed to fix some other deadlocks as well in #368 but I definitely think we need this one as well