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

Do not include external schemas in search_path #120

Merged
merged 3 commits into from May 23, 2017

Conversation

Projects
None yet
2 participants
@mjschultz
Contributor

mjschultz commented May 5, 2017

Amazon's new Redshift Spectrum makes use of external schemas but you cannot set the search_path to include external schemas which breaks reflection. This prevents any external schemas from being added to the search_path.

Todos

  • MIT compatible
  • Tests
  • Documentation
  • Updated CHANGES.rst

mjschultz added some commits May 5, 2017

@jklukas

This comment has been minimized.

Show comment
Hide comment
@jklukas

jklukas May 22, 2017

Collaborator

lgtm. I'll try to run the tests against real Redshift.

Collaborator

jklukas commented May 22, 2017

lgtm. I'll try to run the tests against real Redshift.

WHERE nspname !~ '^pg_'
AND nspname <> 'information_schema'
AND n.oid NOT IN
(SELECT esoid FROM pg_catalog.pg_external_schema)

This comment has been minimized.

@jklukas

jklukas May 22, 2017

Collaborator

Tests against Redshift were failing for this. It looks like schema_names was returning an empty string every time. Pushed this change, moving to a check based on membership in an array populated from a subquery.

@jklukas

jklukas May 22, 2017

Collaborator

Tests against Redshift were failing for this. It looks like schema_names was returning an empty string every time. Pushed this change, moving to a check based on membership in an array populated from a subquery.

@jklukas

This comment has been minimized.

Show comment
Hide comment
@jklukas

jklukas May 23, 2017

Collaborator

I can confirm that this is now passing tests against Redshift with d370d0a

@mjschultz - If the extra commit here looks good to you, I'll go ahead and merge.

Collaborator

jklukas commented May 23, 2017

I can confirm that this is now passing tests against Redshift with d370d0a

@mjschultz - If the extra commit here looks good to you, I'll go ahead and merge.

@mjschultz

This comment has been minimized.

Show comment
Hide comment
@mjschultz

mjschultz May 23, 2017

Contributor

lgtm with the new commit. That's what I had used originally. I thought the other form was a bit cleaner but I guess if it doesn't work quite right this is the one to go with.

Contributor

mjschultz commented May 23, 2017

lgtm with the new commit. That's what I had used originally. I thought the other form was a bit cleaner but I guess if it doesn't work quite right this is the one to go with.

@jklukas jklukas merged commit 230f874 into sqlalchemy-redshift:master May 23, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jklukas

This comment has been minimized.

Show comment
Hide comment
@jklukas

jklukas May 23, 2017

Collaborator

Thanks for submitting this, @mjschultz!

Collaborator

jklukas commented May 23, 2017

Thanks for submitting this, @mjschultz!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment