Reflection regression

[jklukas]

Iteration on #62. Because this branch is on the main repo, travis tests should run.

This adds creation of the schema, but the new test fails with NoSuchTableError: other_schema.basic. I don't understand why this happens.

cc @berdario, @graingert

[jklukas]

@berdario - Am I correct in understanding that the problem you've seen is that databases in alternate schemas don't get reflected properly?

I have successfully used the most recent version of the dialect to reflect tables from schemas other than public, so I'm trying to figure out exactly what the failure mode is.

I'm not sure whether the failing test here is due to a defect of the dialect, or a defect of the test framework.

[berdario]

Yup, the problem I've seen is that tables don't get reflected properly... The outermost error I get is requested table(s) not available... with version 0.1.2 it seems to work fine

Since I found the problem here with a schema/table that is actually in use I think it's perfectly normal that the test fails, and thus it should be a defect in the dialect

[jklukas]

@berdario: In your case, is the schema you're reflecting a table from not in the search_path? I'm beginning to suspect that the problem here has to do with the concept of "visibility". For all the work that I've done with this dialect, the remote schema has been on the search path, so the tables are considered visible. That might explain why I haven't seen this before.

[berdario]

Indeed, if I execute show search_path;, I don't see the schemas that I'm currently using

[jklukas]

I think this is the important gotcha (from http://docs.aws.amazon.com/redshift/latest/dg/r_PG_TABLE_DEF.html):

PG_TABLE_DEF only returns information about tables that are visible to the user. If PG_TABLE_DEF does not return the expected results, verify that the search_path parameter is set correctly to include the relevant schemas.

I'm looking at some options for solving this.

[jklukas]

It's ridiculous that we have to do this, but it works and I think it'll be robust.

[graingert]

So what was postgres default reflection doing before to get the tables?

[jklukas]

I just pushed an amended commit that actually fixes #62. @berdario's test case now passes; again, thank you for reporting the issue and writing the test.

@berdario and @graingert - Does this look good to you?

[graingert]

Does this table https://docs.aws.amazon.com/redshift/latest/dg/r_SVV_TABLE_INFO.html have anything useful in?

[graingert]

Otherwis have a look at https://github.com/awslabs/amazon-redshift-utils/blob/master/src/AdminScripts/table_info.sql ?

[jklukas]

My understanding of the situation is the pg_table_def is the only place in Redshift that holds a complete column-level account of distkey, sortkey, and encoding configuration. SVV_TABLE_INFO and table_info.sql present the same partial view of this info (they both present a table-level view that tells whether any column encoding is set, what the first element of the sort key is, etc.).

pg_table_def is named as if it's a Postgres table, but it's Redshift-specific. It appears to be the only pg_* table that's Redshift-specific, and the only pg_* table that has this search_path restriction.

The PG dialect doesn't have this visibility problem because it doesn't have to access the pg_table_def table.

You can see that AWS also needs to manipulate the search_path in their column encoding utility, which I take as confirmation that search_path manipulation is the only approach here that's going to be comprehensive.

The SET made here to modify the search_path specifies LOCAL so that it only takes effect within that transaction and shouldn't affect the search_path of the session overall.

[graingert]

Is this connection always a connection? Could it be an engine? I think you might need to use a nested transaction here

[jklukas]

Added a nested transaction per @graingert's recommendation.

[jklukas]

Removed print statement in test case, per @graingert's suggestion.

[jklukas]

There's a lot of discussion happening on 94723ec which isn't automatically pulled in here anymore because the commit has been amended.

[graingert]

ah nuts I meant to comment on the PR

[jklukas]

As discussed in 94723ec, we now use a contextual_connect to ensure we aren't jumping to different connections on the same engine, and we explicitly set the search_path back to its original value.

There's also now a test to verify that the search_path change doesn't leak.

[jklukas]

Note that since we now set the search_path back, there's no need for ensuring a transaction, so I'm not explicitly creating one.

[graingert]

That makes sense, I'm happy for you to merge this when you're ready

[jbasko]

This LOCAL keyword breaks it for me #145

When I remove it (SET search_path ...), it works. I have autocommit=True if that's relevant.

[jbasko]

What also works is putting the SET LOCAL search_path ... inside the same query string passed to cc.execute as in:

cc.execute("""
    SET LOCAL search_path TO %s;
    SELECT ....
""" % modified_search_path)