-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Comments
You might be able to simulate this behavior by:
However I think the model would have to take long enough to build so as to ensure that the test doesn't sporadically pass. |
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; |
oooh nice idea @teej |
We only started seeing this issue when updating from Is there any workaround? And, do we know which version introduced this issue? |
@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. |
@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 |
thanks @abelsonlive -- we're focused on getting 0.9.2 out, we'll tackle this one for 0.9.3 |
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. |
This change is going to apply to Redshift / Postgres / Snowflake |
- add integration tests for Redshift
I could reproduce that bug when specifying a pre-hook at the model level like so: docs
|
@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:
or
What's the exact error message that you're seeing from the database? |
Thanks for following up @drewbanin .
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 I guess then I'm approaching creating the UDFs wrong. |
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 |
The proper way to atomically replace a view/table in Postgres/Redshift is:
Currently, dbt drops the table inside of the transaction, which can lead to errors like:
or
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?
The text was updated successfully, but these errors were encountered: