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

[ADAP-1030] [Regression] Back-to-back hooks with a transaction using CTAS + DML fails in 1.5+ #663

Open
2 tasks done
rlh1994 opened this issue Nov 13, 2023 · 8 comments
Open
2 tasks done
Labels
bug Something isn't working help_wanted Extra attention is needed regression transactions

Comments

@rlh1994
Copy link

rlh1994 commented Nov 13, 2023

Is this a regression in a recent version of dbt-redshift?

  • I believe this is a regression in dbt-redshift functionality
  • I have searched the existing issues, and I could not find an existing issue for this regression

Current Behavior

So this is a weird one, I am struggling to really debug the issue and work out why it is happening, let alone how to fix it even with a workaround. I have a macro (used in a post hook) that creates a temp table, and uses this table to delete from then insert into a fixed table (based off a model). We use this in our packages to update our manifest based on the run outcomes, but I have simplified it for this report.

Prior to version 1.5, this worked, and importantly this worked on multiple threads AND when the hook is called multiple times. In reality the hook shouldn't need to be called multiple times, but it can happen if there is user error or something else going on. Setting this to a single thread does not solve the issue.

What now happens is we get an could not open relation with OID 3251611 error on the running of the second hook. I have tried making the temp table not temporary, I have tried adding a commit inside the transaction block, outside the transaction block. I have tried dropping the temporary table before the transaction starts, and inside the start of the transaction, and i have tried adding autocommit: false to my profile - none of it seems to work.

I have confirmed this works on version 1.4.0, so it must be related to the change in connection method.

Expected/Previous Behavior

The post hooks would be complete successfully. I'd even be fine with a workaround that allows it to still work, I am out of ideas.

Steps To Reproduce

  1. New project, redshift profile, dbt 1.5 or greater
  2. Create a model called test_table.sql content:
    {{
      config(
        materialized = 'table',
        )
    }}
    select 1 as test_col
    
  3. create macro transaction_macro.sql content:
    {% macro transaction_macro() %}
        {% if execute %}
            begin transaction;
              --temp table
              create temporary table trans_temp_table as (
                  select 1 as test_col
              );
      
              delete from {{target.schema}}.test_table where test_col in (select test_col from trans_temp_table);
              insert into {{target.schema}}.test_table  (select * from trans_temp_table);
              commit;
            end transaction;
    
            drop table trans_temp_table;
        {% endif %}
    {% endmacro %}
    
  4. Add the macro twice as an on-end hook in the project
    on-run-end:
      - "{{ transaction_macro() }}"
      - "{{ transaction_macro() }}"
    
  5. dbt run --target redshift. Compare if you run on dbt 1.4.0 that it is successful.

Relevant log output

11:34:37  Running with dbt=1.6.6
11:34:38  Registered adapter: redshift=1.6.2
11:34:38  Unable to do partial parsing because profile has changed
11:34:38  Found 1 model, 2 operations, 0 sources, 0 exposures, 0 metrics, 401 macros, 0 groups, 0 semantic models
11:34:38  
11:34:40  Concurrency: 1 threads (target='redshift')
11:34:40  
11:34:40  1 of 1 START sql table model dbt_ryan.test_table ............................... [RUN]
11:34:46  1 of 1 OK created sql table model dbt_ryan.test_table .......................... [SUCCESS in 6.71s]
11:34:47  
11:34:47  Running 2 on-run-end hooks
11:34:47  1 of 2 START hook: dbt_demo.on-run-end.0 ....................................... [RUN]
11:34:59  1 of 2 OK hook: dbt_demo.on-run-end.0 .......................................... [SUCCESS in 12.41s]
11:34:59  2 of 2 START hook: dbt_demo.on-run-end.1 ....................................... [RUN]
11:34:59  Database error while running on-run-end
11:34:59  
11:34:59  Finished running 1 table model, 1 hook in 0 hours 0 minutes and 21.29 seconds (21.29s).
11:34:59  
11:34:59  Completed with 1 error and 0 warnings:
11:34:59  
11:34:59    on-run-end failed, error:
 could not open relation with OID 3251641
11:34:59  
11:34:59  Done. PASS=1 WARN=0 ERROR=1 SKIP=0 TOTAL=2

Environment

- OS: MacOS
- Python: 3.9.13
- dbt-core (working version): 1.4.0
- dbt-redshift (working version): 1.4.0
- dbt-core (regression version): 1.6.6
- dbt-redshift (regression version): 1.6.2

(I believe it has been failing since 1.5.0 but have not verified specifically, we have a customer reporting the issue on 1.5.9 at least)

Additional Context

It's maybe related to this #521 and possibly also this #501 ?

@rlh1994 rlh1994 added bug Something isn't working regression triage labels Nov 13, 2023
@github-actions github-actions bot changed the title [Regression] Something to do with temporary tables and transactions in multiple hooks fails in 1.5+ [ADAP-1030] [Regression] Something to do with temporary tables and transactions in multiple hooks fails in 1.5+ Nov 13, 2023
@rlh1994
Copy link
Author

rlh1994 commented Nov 13, 2023

For additional context, this also fails if you call the macro twice within the model (you'll need to have run the project at least once so the table exists, but you get the idea). So it doesn't seem related specifically to hooks.

@rlh1994
Copy link
Author

rlh1994 commented Nov 13, 2023

Running with -d the error specifically occurs on the call of the delete from... line within the second run of the macro/hook - this suggests the relation it can't open is the test_model. From my understanding that means this table has an ongoing transaction, but given the previous macro has finished and it is single threaded, this shouldn't be the case.

@rlh1994
Copy link
Author

rlh1994 commented Nov 13, 2023

Final context, this happens even if I make it not bounded by a transaction block. So I really don't know what's going on here...

@dbeatty10
Copy link
Contributor

For context, dbt-redshift switched from psychopg2 to amazon-redshift-python-driver / redshift_connector in 1.5.0, so that probably explains why it first showed up in v1.5.

@rlh1994 do you think it is similar to what is described here?

Do you know if this is only happening in cross-database scenarios? Or does it also happen in single database scenarios?

What happens if you try a greatly simplified version using a permanent table (that isn't dropped at the end)?

{% macro transaction_macro() %}
    {% if execute %}
          create table if not exists trans_temp_table as (
              select 1 as test_col
          );
  
          delete from {{target.schema}}.test_table where test_col in (select test_col from trans_temp_table);
          insert into {{target.schema}}.test_table  (select * from trans_temp_table);
    {% endif %}
{% endmacro %}

@rlh1994
Copy link
Author

rlh1994 commented Nov 14, 2023

I'm not 100% sure if it is the same - it's not entirely clear if the OID reference it can't find is the temporary table, or the main table. It's also single database only, no cross-database stuff here.

I had to alter your code slightly, as redshift only seems to let you use create table if not exists when you are specifying the structure, not from a query:

{% macro transaction_macro() %}
    {% if execute %}
          create table if not exists trans_temp_table (
            test_col int
          );
          truncate table trans_temp_table;
          insert into  trans_temp_table (test_col) values (1);
  
          delete from {{target.schema}}.test_table where test_col in (select test_col from trans_temp_table);
          insert into {{target.schema}}.test_table  (select * from trans_temp_table);
    {% endif %}
{% endmacro %}

So this does work, and it also works in a transaction interestingly enough. I've re-added the drop statement in and that also works, but the temporary table and dropping seems to be where the issue is!

i.e. this still works:

{% macro transaction_macro() %}
    {% if execute %}
    begin transaction;
          create table if not exists trans_temp_table (
            test_col int
          );
          truncate table trans_temp_table;
          insert into  trans_temp_table (test_col) values (1);
  
          delete from {{target.schema}}.test_table where test_col in (select test_col from trans_temp_table);
          insert into {{target.schema}}.test_table  (select * from trans_temp_table);
          drop table trans_temp_table;
    end transaction;
    {% endif %}
{% endmacro %}

I think it's not ideal, and I'm not sure I entirely trust transactions now being run with one command at a time, but at least it gives us a fix we can implement that should resolve any issues people are seeing...

@dbeatty10 dbeatty10 changed the title [ADAP-1030] [Regression] Something to do with temporary tables and transactions in multiple hooks fails in 1.5+ [ADAP-1030] [Regression] Back-to-back hooks that create and drop a temporary table within a transaction fails in 1.5+ Nov 14, 2023
@dbeatty10
Copy link
Contributor

Okay, glad that you found a workaround!

I'm wondering if there is some kind of interaction effect with the way redshift_connector handles transactions and multi-statement SQL differently than psycopg2.

Do either temporary or permanent tables work if you pass a unique postfix when you call the transaction_macro?

{% macro transaction_macro(hook_num=0) %}
    {% set  trans_temp_table = "trans_temp_table_" ~ hook_num %}
    {% if execute %}
    begin transaction;
          create temporary table if not exists {{ trans_temp_table }} (
            test_col int
          );
          truncate table {{ trans_temp_table }};
          insert into {{ trans_temp_table }} (test_col) values (1);
  
          delete from {{target.schema}}.test_table where test_col in (select test_col from {{ trans_temp_table }});
          insert into {{target.schema}}.test_table  (select * from {{ trans_temp_table }});
          drop table {{ trans_temp_table }};
    end transaction;
    {% endif %}
{% endmacro %}
on-run-end:
  - "{{ transaction_macro(1) }}"
  - "{{ transaction_macro(2) }}"

@rlh1994
Copy link
Author

rlh1994 commented Nov 15, 2023

I'm not sure what I was doing yesterday but it does seem to work with temporary tables using this method regardless of a suffix, sorry. That seems to suggest that the create table ... as is acting differently to create table ... (types) approach for some reason...

Even testing removing the DML on the temp table (I wondered if the implicit commit from a truncate was doing something) seems to keep it working, so the only difference is how the temp table is created.

Working:

{% macro transaction_macro() %}
    {% if execute %}
        begin transaction;
          --temp table
          create temporary table trans_temp_table (
              test_col int
          );
          truncate table trans_temp_table;
          insert into  trans_temp_table (test_col) values (1);
  
          delete from {{target.schema}}.test_table where test_col in (select test_col from trans_temp_table);
          insert into {{target.schema}}.test_table  (select * from trans_temp_table);
          commit;
        end transaction;

        drop table trans_temp_table;
    {% endif %}
{% endmacro %}

OID error:

{% macro transaction_macro() %}
    {% if execute %}
        begin transaction;
          --temp table
          create temporary table trans_temp_table as (
              select 1 as test_col
          );
          truncate table trans_temp_table;
          insert into  trans_temp_table (test_col) values (1);
  
          delete from {{target.schema}}.test_table where test_col in (select test_col from trans_temp_table);
          insert into {{target.schema}}.test_table  (select * from trans_temp_table);
          commit;
        end transaction;

        drop table trans_temp_table;
    {% endif %}
{% endmacro %}

@dbeatty10 dbeatty10 changed the title [ADAP-1030] [Regression] Back-to-back hooks that create and drop a temporary table within a transaction fails in 1.5+ [ADAP-1030] [Regression] Back-to-back hooks with a transaction using CTAS + DML fails in 1.5+ Nov 15, 2023
@dbeatty10
Copy link
Contributor

Glad we've honed in on it!

Based on your testing, this kind of CTAS raises that OID error when it's within back-to-back post-hooks regardless if the table is temporary or not. It requires the following:

  • hook containing CTAS followed by DML
  • multiple back-to-back hooks with the same logic

I'm going to label this as "help wanted".

@dbeatty10 dbeatty10 added help_wanted Extra attention is needed and removed triage labels Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help_wanted Extra attention is needed regression transactions
Projects
None yet
Development

No branches or pull requests

3 participants