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

Allow to find table in temporary schema for Redshift #358

Merged
merged 15 commits into from
Dec 12, 2021

Conversation

galachad
Copy link
Contributor

Issue:

> DBI::dbWriteTable(con, "iris", iris, temporary = TRUE, overwrite = TRUE)
> DBI::dbWriteTable(con, "iris", iris, temporary = TRUE, overwrite = TRUE)
 Error: Failed to fetch row: ERROR:  Relation "iris" already exists 

Using

(SELECT 1 AS nr, current_schema() AS table_schema) ttt

is nice solution but it do not return temporary schema same as current_schemas(true).

This change works correctly with temp schema after creating temporary table in Redshift.

@krlmlr
Copy link
Member

krlmlr commented Dec 2, 2021

Thanks, this is amazing! A slew of tests now pass on Redshift, 4025336. The only thing that seems to be missing now is dbRemoveTable(temporary = TRUE) . Any idea how to achieve this?

@krlmlr
Copy link
Member

krlmlr commented Dec 2, 2021

Also, I'd appreciate a brief review of my CTE rewrite.

@galachad
Copy link
Contributor Author

galachad commented Dec 4, 2021

The changes look great. I also added support for dbRemoveTable(temporary = TRUE) in Redshift, please check. It's possible to return error/warning when the temp schema not exist yet but the standard behavior of function looks ok here.

@krlmlr
Copy link
Member

krlmlr commented Dec 4, 2021

Thanks a lot, this looks great! A few remarks:

  • Do the schema names always start with pg_temp_ ? The pattern should be pg_temp_% in this case perhaps.
  • If we can't fetch the schema name because it doesn't exist, we should raise an error if fail_if_missing is TRUE .
  • Should we cache the schema name so that it is fetched only once?
  • Can we extract the logic of obtaining the schema name into a function? This function can return NULL if the temp schema doesn't exist yet, or perhaps take a fail_if_missing argument.

Would you like to take another stab?

@krlmlr krlmlr force-pushed the find-temporary-table branch from d50fdde to 628f2aa Compare December 4, 2021 10:40
@krlmlr krlmlr modified the milestones: 1.4.1, 1.4.2, 1.4.3 Dec 5, 2021
@galachad
Copy link
Contributor Author

galachad commented Dec 5, 2021

Sure

  • Yes it's pg_temp_%, I think Redshift is managing temp schema just by adding access to one of existing schemas and clean up after finish connection. Redshift doesn't has alias like in Postgres.
SELECT * FROM pg_catalog.pg_namespace WHERE nspname like 'pg_temp_%'
  • I added raising error when fail_if_missing is TRUE (for Redshift).
  • That's nice idea. Added.
  • Extracted to find_temp_schema function.

@krlmlr
Copy link
Member

krlmlr commented Dec 12, 2021

══ Results ══════════════════════════════════════════════════════════════════════════════════════
Duration: 385.5 s

── Skipped tests  ───────────────────────────────────────────────────────────────────────────────
• tweak: omit_blob_tests (7)

[ FAIL 0 | WARN 0 | SKIP 7 | PASS 6386 ]

With all tests enabled. Thanks a lot!

@krlmlr krlmlr merged commit 05c25ea into r-dbi:main Dec 12, 2021
@krlmlr
Copy link
Member

krlmlr commented Dec 12, 2021

One more thing: do you agree to contribute under the MIT license? Please indicate your consent or disapproval in #312. Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants