Skip to content

Avoid manipulating search path in table metadata fetch#147

Merged
jklukas merged 4 commits into
sqlalchemy-redshift:masterfrom
bluelabsio:no-more-search-path
Dec 11, 2018
Merged

Avoid manipulating search path in table metadata fetch#147
jklukas merged 4 commits into
sqlalchemy-redshift:masterfrom
bluelabsio:no-more-search-path

Conversation

@cwegrzyn

@cwegrzyn cwegrzyn commented Jun 28, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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.

@jklukas jklukas left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

Does that look okay?

@jklukas jklukas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

small typo to fix

Comment thread CHANGES.rst Outdated
Co-Authored-By: cwegrzyn <chris.wegrzyn@gmail.com>
@cwegrzyn

Copy link
Copy Markdown
Contributor Author

Oops, thanks! Committed

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

jklukas commented Dec 11, 2018

Copy link
Copy Markdown
Member

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.

2 participants