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

issues with concurrent transaction #653

Closed
drewbanin opened this issue Feb 9, 2018 · 13 comments
Closed

issues with concurrent transaction #653

drewbanin opened this issue Feb 9, 2018 · 13 comments
Assignees
Labels
bug Something isn't working redshift

Comments

@drewbanin
Copy link
Contributor

drewbanin commented Feb 9, 2018

The proper way to atomically replace a view/table in Postgres/Redshift is:

  1. begin transaction
  2. rename model to "model_old"
  3. rename new model to "model'"
  4. commit
  5. drop table "model_old"

Currently, dbt drops the table inside of the transaction, which can lead to errors like:

ERROR: table 12345 dropped by concurrent transaction

or

could not complete because of conflict with concurrent transaction

Via @teej
See also: this SO post

To fix this, dbt's table and view materializations need to be updated to drop the old model outside of the transaction.

This PR will ultimately require < 5 lines of code change, but since it involves the table and view materializations, we should be very cautious here. Is there a good way to write an integration test for this?

@drewbanin drewbanin added bug Something isn't working redshift labels Feb 9, 2018
@abelsonlive
Copy link
Contributor

abelsonlive commented Feb 9, 2018

You might be able to simulate this behavior by:

  • First, run dbt and build a model.
  • Second, start a second dbt run which builds a model in a thread
  • Finally, in a concurrent thread, attempt to access the results of the model.

However I think the model would have to take long enough to build so as to ensure that the test doesn't sporadically pass.

@teej
Copy link

teej commented Feb 9, 2018

You can force this condition to trigger by adding a sleep in the model's SQL. You can sleep in Redshift with:

CREATE OR REPLACE FUNCTION f_sleep (x float)
RETURNS bool IMMUTABLE
AS
$$
  from time import sleep
  sleep(x)
  return True
$$ LANGUAGE plpythonu;

@abelsonlive
Copy link
Contributor

oooh nice idea @teej

@igrayson
Copy link
Contributor

We only started seeing this issue when updating from 0.8.0 to 0.9.1.

Is there any workaround? And, do we know which version introduced this issue?

@drewbanin
Copy link
Contributor Author

@igrayson we overhauled how model materializations work in 0.9.0, but it looks like the core SQL logic was unchanged.

Ultimately, this is going to be a small code change, but it's a little daunting to make because Redshift's transaction management is pretty finicky.

Happy to point someone in the right direction if they're interested in tackling this, and if not, I think we'll be able to hit it pretty soon. @igrayson @teej @abelsonlive are you folks able to help test an alpha version of this fix when one is ready? That would go a long way towards making me feel good about any prospective fix.

@abelsonlive
Copy link
Contributor

@drewbanin can definitely help test anything you put out... have been a bit swamped but might still take a stab at this if my schedule clears up

@drewbanin
Copy link
Contributor Author

thanks @abelsonlive -- we're focused on getting 0.9.2 out, we'll tackle this one for 0.9.3

@allanw
Copy link

allanw commented May 11, 2018

Sounds good - as discussed on the Slack channel, this could potentially also fix an issue I've been having where DBT is forced to wait indefinitely due to Redshift failing to execute some statements (e.g. DROP VIEW) within a transaction block. When I check the stv_recents Redshift table, the query is left in a 'running' state and doesn't complete.

@cmcarthur
Copy link
Member

This change is going to apply to Redshift / Postgres / Snowflake

@louisguitton
Copy link

louisguitton commented Nov 6, 2019

I could reproduce that bug when specifying a pre-hook at the model level like so: docs

image

Running with dbt=0.14.3

@drewbanin
Copy link
Contributor Author

@louisguitton that's surprising -- can you tell me which database you're using? We have definitely seen this issue when grants are applied to a schema, but i've never seen it happen when grants are applied to a specific model.

Eg, this can fail:

models:
  project-name:
    post-hook: "grant select on all tables in schema {{ this.schema }} to db_reader"

or

models:
  project-name:
    post-hook: "grant usage on schema {{ this.schema }} to db_reader"

What's the exact error message that you're seeing from the database?

@louisguitton
Copy link

Thanks for following up @drewbanin .
I'm using Redshift, and a pre_hook that creates 1 UDF.
I'm running dbt run --models model_1 model_2 with 8 threads and I get

Completed with 1 error and 0 warnings:

Database Error in model stg_localytics__users_match_viewed (models/staging/localytics/stg_localytics__users_match_viewed.sql)
  could not complete because of conflict with concurrent transaction
  compiled SQL at target/compiled/of_models/staging/localytics/stg_localytics__users_match_viewed.sql

Due to the error message, I assume this is to be expected: the UDF is created twice (once for model_1 and once for model_2) on separate threads so they end up being conflicting concurrent transactions.

If I run dbt run --models model_1 && dbt run --models model_2 it works fine.

I guess then I'm approaching creating the UDFs wrong.

@drewbanin
Copy link
Contributor Author

Ah! ok - are you able to use something like an operation to create these UDFs? Ideally Redshift would let you create UDFs transactionally, but I suppose there isn't a ton of merit to creating the same UDF once for each model anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working redshift
Projects
None yet
Development

No branches or pull requests

7 participants