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

(Issue#45) Scope Configuration doesn't work with Postgres #54

Closed
wants to merge 1 commit into from

Conversation

oleksm
Copy link

@oleksm oleksm commented Jul 28, 2020

Hi,
I noticed same issue described by @Manny651 when tried using scope config.json as described in issue #45 . The problem is that 'database' property attempted to use as a schema despite of warning message that it is going to be ignored, and 'schema' property ignored at the same time. More importantly, SET search_path TO databaseName statement contains tildas which is not correct postgres syntax.
The change am I proposing is:

  1. Fix SET search_path TO schema syntax
  2. Use 'schema' property instead of 'database' inside switchDatabase method.
  3. When switching db, preserve original schema in search path so migrations table is still available
    Please approve PR.

@oleksm
Copy link
Author

oleksm commented Jul 30, 2020

@wzrdtales Could you please approve pr?

@wzrdtales
Copy link
Member

Hey, thanks for pinging me again and thank you for your contribution, I will review it today.

@wzrdtales
Copy link
Member

looks good in general, however the pg driver is a dependency to a lot of other drivers, so SET search_path TO %s,%s this alteration of the search path could impact them. I will have to validate a few targets before I can merge this.

this.runSql(
util.format('SET search_path TO `%s`', options.database),
Copy link
Member

Choose a reason for hiding this comment

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

this will certainly break a lot of dbs connected using the pg wire protocol, so that can't just be overwritten here (also in pg dbs exist though

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