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

Avoid manipulating search path in table metadata fetch #147

Merged
merged 4 commits into from Dec 11, 2018

Conversation

cwegrzyn
Copy link
Contributor

@cwegrzyn cwegrzyn commented Jun 28, 2018

We've run into some issues with transactions and the way that _get_all_column_info manipulates the search path. The search path manipulation only appears to be necessary because pg_table_def is used-- but pg_table_def is a pretty simple view that we can integrate into the query, minus its pesky search path filter.

For reference, pg_table_def is defined on Redshift as follows:

CREATE OR REPLACE VIEW pg_catalog.pg_table_def AS
SELECT n.nspname AS schemaname, c.relname AS tablename, a.attname AS "column", format_type(a.atttypid, a.atttypmod) AS "type", format_encoding(a.attencodingtype::integer) AS "encoding", a.attisdistkey AS "distkey", a.attsortkeyord AS "sortkey", a.attnotnull AS "notnull"
FROM pg_namespace n, pg_class c, pg_attribute a
WHERE n.oid = c.relnamespace AND c.oid = a.attrelid AND a.attnum > 0 AND NOT a.attisdropped AND pg_table_is_visible(c.oid)
ORDER BY n.nspname, c.relname, a.attnum

pg_table_is_visible is the bit that limits to tables in the search path.

Todos

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

@cwegrzyn
Copy link
Contributor Author

Hey! I was just wondering about next steps for merging this in. I'd be happy to add an entry to CHANGES. Not sure if new tests/docs are relevant here. We're also bumping into the issue solved by #105, so I'd be happy to update that one to reflect the changes here.

Copy link
Member

@jklukas jklukas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing integration tests, so this looks great! All we need is a line in CHANGES.rst that describes the change in behavior and links to this PR, and I'll go ahead and merge.

@cwegrzyn
Copy link
Contributor Author

Does that look okay?

Copy link
Member

@jklukas jklukas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small typo to fix

CHANGES.rst Outdated Show resolved Hide resolved
Co-Authored-By: cwegrzyn <chris.wegrzyn@gmail.com>
@cwegrzyn
Copy link
Contributor Author

Oops, thanks! Committed

@jklukas jklukas merged commit e60c519 into sqlalchemy-redshift:master Dec 11, 2018
@jklukas
Copy link
Member

jklukas commented Dec 11, 2018

Released in https://pypi.org/project/sqlalchemy-redshift/0.7.2/

@cwegrzyn cwegrzyn deleted the no-more-search-path branch August 6, 2019 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants