Use the PID to terminate the session #568

dbeatty10 · 2023-08-07T23:56:19Z

resolves #553
docs N/A

Problem

When we switched from psycopg2 to redshift_connector in dbt-redshift 1.5.0, the return type of fetchone() changed from a tuple to an array. This caused the query to terminate the session (based on the PID) to not be executed correctly.

Solution

This solution does two things:

Optimistically assumes there is at least one item returned from the PID query and returns that value
Directly executes a query to terminate the session (rather than using self.add_query(sql) which raises an error for some reason)

One or both can be modified depending on feedback from the code reviewer.

Also

While troubleshooting the solution noticed that there's a stack trace that didn't happen in dbt-core 1.4.0: dbt-labs/dbt-core#8338

Checklist

I have read the contributing guide and understand what's expected of me
I have run this code in development and it appears to resolve the stated issue
👈 This PR does not include any tests, and obviously none existed previously! Maybe that's okay 🤷
This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc)

github-actions · 2023-08-07T23:56:37Z

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the dbt-redshift contributing guide.

jmleon1219 · 2023-10-06T17:41:32Z

Can we get this merged?

mikealfare

Most of the review is context. The one change I would recommend is adding the logging statement backend. It doesn't need to be verbatim what I put, but I imagine that could be useful for troubleshooting when things go awry.

dbt/adapters/redshift/connections.py

Rather than using the `SQLConnectionManager`

* The first element of the result is the PID * Debug-level logging of high-level message + SQL * Using redshift_connector `cursor.fetchone()` returns `(<something>,)` * Use cursor to call `select pg_terminate_backend({pid})` directly rather than using the `SQLConnectionManager` --------- Co-authored-by: Mike Alfare <[email protected]>

* The first element of the result is the PID * Debug-level logging of high-level message + SQL * Using redshift_connector `cursor.fetchone()` returns `(<something>,)` * Use cursor to call `select pg_terminate_backend({pid})` directly rather than using the `SQLConnectionManager` --------- Co-authored-by: Mike Alfare <[email protected]> (cherry picked from commit 5d4f3f5)

* The first element of the result is the PID * Debug-level logging of high-level message + SQL * Using redshift_connector `cursor.fetchone()` returns `(<something>,)` * Use cursor to call `select pg_terminate_backend({pid})` directly rather than using the `SQLConnectionManager` --------- Co-authored-by: Mike Alfare <[email protected]> (cherry picked from commit 5d4f3f5) Co-authored-by: Doug Beatty <[email protected]>

* adding SSO support for redshift Committer: Abby Whittier <[email protected]> * ADAP-891: Support test results as views (#614) * implement store-failures-as tests * Use the PID to terminate the session (#568) * The first element of the result is the PID * Debug-level logging of high-level message + SQL * Using redshift_connector `cursor.fetchone()` returns `(<something>,)` * Use cursor to call `select pg_terminate_backend({pid})` directly rather than using the `SQLConnectionManager` --------- Co-authored-by: Mike Alfare <[email protected]> * added error checking for new optional user field * black formatting * move connection fixtures into the functional scope * add iam user creds to the test.env template * add test for database connection method * add iam user auth test * add IAM User auth test and second user auth method * changie * maintain existing behavior when not providing profile * add AWS IAM profile * pull in new env vars * fixed env vars refs for CI * move all repo vars to secrets * split out connect method by connection method and provided information * condition to produce just kwargs, consolidate connect method * update .format to f-strings * incorporate feedback from pr#630 * update kwargs logic flow * updates to make space for iam role * revert type on user * revert test case decorator * revert test case decorator * revert error message * add integration tests * make space for both iam user and iam role in testing * add role arn * naming * try supplying region for CI * add region to CI env * we can only support role credentials by profile * move iam user specific config out of iam and into iam user * add type annotations * move iam defaults out of iam user * add required params to test profiles * add required params to test profiles * simplify test files * add expected fields back in * split out unit test files * split out unit test files * add unit tests for iam role auth method * standardize names * allow for the default profile * add unit tests for iam role access * changie * changie --------- Co-authored-by: Abby Whittier <[email protected]> Co-authored-by: Doug Beatty <[email protected]> Co-authored-by: colin-rogers-dbt <[email protected]> Co-authored-by: Anders <[email protected]>

The first element of the result is the PID

c8880fd

cla-bot bot added the cla:yes label Aug 7, 2023

dbeatty10 marked this pull request as ready for review August 7, 2023 23:56

dbeatty10 requested a review from a team as a code owner August 7, 2023 23:56

dbeatty10 requested a review from mikealfare August 7, 2023 23:56

dbeatty10 added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Aug 7, 2023

Changelog entry

3210ae0

dbeatty10 mentioned this pull request Aug 7, 2023

[ADAP-721] [Bug] [Regression] Canceled or failed run creates a Database Error #553

Closed

2 tasks

mikealfare and others added 2 commits August 11, 2023 12:21

Merge branch 'main' into dbeatty/fix-553

f4f2bc2

Merge branch 'main' into dbeatty/fix-553

bf9776f

dbeatty10 mentioned this pull request Sep 5, 2023

[ADAP-870] [Bug] Adapter seems to malformat kill queries when using --fail-fast dbt-redshift 1.5.8 #593

Closed

2 tasks

mikealfare reviewed Oct 10, 2023

View reviewed changes

dbt/adapters/redshift/connections.py Show resolved Hide resolved

dbt/adapters/redshift/connections.py Show resolved Hide resolved

dbt/adapters/redshift/connections.py Show resolved Hide resolved

Merge branch 'main' into dbeatty/fix-553

140b5b9

dbeatty10 commented Oct 10, 2023

View reviewed changes

dbt/adapters/redshift/connections.py Show resolved Hide resolved

dbeatty10 and others added 4 commits October 10, 2023 16:47

Debug-level logging of high-level message + SQL

943b66d

Using redshift_connector cursor.fetchone() returns (<something>,)

faf85c2

Merge branch 'main' into dbeatty/fix-553

f22315e

Use cursor to call select pg_terminate_backend({pid}) directly

a36d7f8

Rather than using the `SQLConnectionManager`

mikealfare approved these changes Oct 11, 2023

View reviewed changes

mikealfare merged commit 5d4f3f5 into main Oct 11, 2023
26 checks passed

mikealfare deleted the dbeatty/fix-553 branch October 11, 2023 01:15

colin-rogers-dbt added the backport 1.6.latest label Nov 6, 2023

github-actions bot mentioned this pull request Nov 6, 2023

[Backport 1.6.latest] Use the PID to terminate the session #658

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the PID to terminate the session #568

Use the PID to terminate the session #568

dbeatty10 commented Aug 7, 2023 •

edited

Loading

github-actions bot commented Aug 7, 2023

jmleon1219 commented Oct 6, 2023

mikealfare left a comment

Use the PID to terminate the session #568

Use the PID to terminate the session #568

Conversation

dbeatty10 commented Aug 7, 2023 • edited Loading

Problem

Solution

Also

Checklist

github-actions bot commented Aug 7, 2023

jmleon1219 commented Oct 6, 2023

mikealfare left a comment

Choose a reason for hiding this comment

dbeatty10 commented Aug 7, 2023 •

edited

Loading