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

Public is the default schema but if you use different name your are #5892

Merged
merged 1 commit into from
Oct 8, 2015
Merged

Public is the default schema but if you use different name your are #5892

merged 1 commit into from
Oct 8, 2015

Conversation

yverry
Copy link
Contributor

@yverry yverry commented Sep 28, 2015

Unable to start the upgrade process.
This new SQL command exclude 'information_schema' instead of 'public'

This PR fix the issue #5891

@ErisDS
Copy link
Member

ErisDS commented Sep 29, 2015

As far as I understand, this has been working for people with pg since we implemented it. I don't understand enough of what's going on here or why this is a valid fix to merge this.

Please take the time to explain it, and I recommend also finding another postgres user to help you test and champion this change so that we can get it merged.

@jberkus
Copy link

jberkus commented Sep 29, 2015

Looking into this change now.

@jberkus
Copy link

jberkus commented Sep 29, 2015

First: I disagree with the actual SQL of the change. There's a better query for this.

However, that raises a question of what we want to support, exactly, in the way of PostgreSQL schema for Ghost databases. If we want robust support for setting a specific schema for the Ghost application to store its tables in, then just changing the search query for table names is insufficient; you could have a "posts" table for a different application in a different schema. However, having robust schema support would require adding a bunch of plubming to keep track of which schema Ghost is supposed to be in, and I don't really see the use case for it; why not keep Ghost isolated to its own database?

@jberkus
Copy link

jberkus commented Sep 30, 2015

OK, per discussion on Slack, we're not going to try to implement general non-public schema support at this time. As such, the migration queries should be fixed like this:

 SELECT table_name FROM information_schema.tables WHERE table_schema NOT IN ( 'information_schema', 'pg_catalog' )

Alternately, a simpler query relies on PostgreSQL-specific views:

SELECT relname AS table_name FROM pg_stat_user_tables

... either of those will pick up the Ghost tables if you've saved them to a schema other than public. However, they will result in a false positive if you happen to have other apps installed in other schema whose names happen to be the same as Ghost table names.

Personally, I'd recommend just keeping Ghost in its own database until full schema support is available.

@jberkus
Copy link

jberkus commented Oct 1, 2015

@Yanntech hello? Please comment.

@yverry
Copy link
Contributor Author

yverry commented Oct 2, 2015

Hi,

Sorry for late, I need to configure github notification.

First, postgres is not mysql, schema != databases (namespace is like: database.schema.tables).
As this I provide to my customer only one database per user, in this database each user can create all schema as needed.

For remember, the current SQL is:

SELECT table_name FROM information_schema.tables WHERE table_schema = 'public' and table_name not like 'pg_%'

This query work only if database is dedicated to ghost.

My new query is compatible with the old (pg specific) :

SELECT table_name FROM information_schema.tables WHERE table_schema = CURRENT_SCHEMA() and table_name not like 'pg_%'
  • If user as a dedicated database to ghost, and use default schema public, CURRENT_SCHEMA() return public, this change nothing.
  • If user set a search_path with custom schema CURRENT_SCHEMA() return the good value

For example:

ce=> SELECT CURRENT_SCHEMA();
 current_schema
----------------
 ghost
(1 row)

ce=> SELECT table_name FROM information_schema.tables WHERE table_schema = CURRENT_SCHEMA() and table_name not like 'pg_%';
       table_name
------------------------
 roles
 roles_users
 permissions
 permissions_users
 permissions_roles
 permissions_apps
 tags
 apps
 app_fields
 accesstokens
 users
 posts
 app_settings
 client_trusted_domains
 posts_tags
 refreshtokens
 clients
 settings
(18 rows)

@jberkus
Copy link

jberkus commented Oct 2, 2015

Yann,

The current_schema() solution is nice; we'd want a doc or wiki update somewhere which explained how to set a schema for the ghost user, though.

Also, if you're using current_schema, "where table_name not like 'pg%'" is completely unnecessary.

@yverry
Copy link
Contributor Author

yverry commented Oct 2, 2015

The where clause is to maintain the backward compat If user still use the default (public) schema. About the documentation, I can explain this good practice but I didn't found anything more than http://support.ghost.org/config/ or you own issue #5878

@jberkus
Copy link

jberkus commented Oct 2, 2015

Yeah, we'll need a wiki page or something.

I don't see why the checks are failing? I looked at the supposed failed test, and no individual test failed. What's up with that?

@yverry
Copy link
Contributor Author

yverry commented Oct 3, 2015

I don't know why, this is a timeout, can you ping someone has wrote this test unit ?

not ok 2 PhantomJS 1.9 - Integration : Component : gh-navigation triggers reorder action
    ---

        message: >

            timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

        Log: >

    ...

@yverry
Copy link
Contributor Author

yverry commented Oct 3, 2015

I found my previous commit and travis also fail in mysql ... ( https://travis-ci.org/TryGhost/Ghost/builds/82523130 )
I don't understand why but I guess this is not my PR

@jberkus
Copy link

jberkus commented Oct 3, 2015

Anyway, change it to:

    SELECT table_name FROM information_schema.tables WHERE table_schema = CURRENT_SCHEMA();

... and I'll sign off on it.

@yverry
Copy link
Contributor Author

yverry commented Oct 4, 2015

done and travis is happy ;-)

@jberkus
Copy link

jberkus commented Oct 4, 2015

+1 to merge.

@ErisDS
Copy link
Member

ErisDS commented Oct 6, 2015

@jberkus thanks for leading this.

@Yanntech could you please update this PR to match our contributing guidelines? Please squash the commits and leave the final commit with a commit message in this format. Thanks 👍

…PostgreSQL

closes #5891
- use CURRENT_SCHEMA() instead of 'public'
- remove the WHERE condition
@yverry
Copy link
Contributor Author

yverry commented Oct 6, 2015

I fail the rebase, the commit message is OK.
I can do better next time

@yverry
Copy link
Contributor Author

yverry commented Oct 7, 2015

Travis fail again for no reason :(

@kevinansfield
Copy link
Contributor

I've restarted the failing build for you.

@yverry
Copy link
Contributor Author

yverry commented Oct 8, 2015

Thank you @kevinansfield , travis success ;-)

ErisDS added a commit that referenced this pull request Oct 8, 2015
Public is the default schema but if you use different name your are
@ErisDS ErisDS merged commit eed6879 into TryGhost:master Oct 8, 2015
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.

4 participants