Make doesTableExist check for table in current search path

[remiremi]

• If a migrations table did exist in a schema outside of the search path, doesTableExist would return true but the migration would then fail with 'relation "migrations" does not exist'
• See https://dba.stackexchange.com/a/86098 for the details on the query
• This new query makes it possible to have one migrations table per schema (in other words to have each schema handle its own migrations)

[ThomWright]

Thanks for this, it looks good and I'd like to consider merging it.

Before I do, I'd like to think about alternatives, and consider any downsides to this implementation.

My understanding from the docs is that default search path is "$user", public. So this decreases the search area from any schema, to just those two. This is good - I can't see any situation where this would break existing uses, so I'm hoping it isn't a breaking change.

I think an alternative would be to explicitly use the public schema, and potentially allow users to override it. Again, I think this wouldn't break anything. I think this could be a better change.

Thoughts?

[remiremi]

My understanding form the docs is that default search path is "$user", public. So this decreases the search area from any schema, to just those two. This is good - I can't see any situation where this would break existing uses, so I'm hoping it isn't a breaking change.

Correct, I don't see any way it could break either (except that the query is postgres 9.4+)

I think an alternative would be to explicitly use the public schema, and potentially allow users to override it. Again, I think this wouldn't break anything. I think this could be a better change.

I think it would be a great improvement. However it could break things for users who have their migrations table in a schema other than public, which is already possible with the current version of postgres-migrations.

For example if you revoke access to public from your migration user:

REVOKE ALL ON SCHEMA public FROM username;

Then postgres-migrations would create the migrations table in the username schema, and this would work just fine.

[ThomWright]

I think an alternative would be to explicitly use the public schema, and potentially allow users to override it. Again, I think this wouldn't break anything. I think this could be a better change.

I think it would be a great improvement. However it could break things for users who have their migrations table in a schema other than public, which is already possible with the current version of postgres-migrations.

For example if you revoke access to public from your migration user:

REVOKE ALL ON SCHEMA public FROM username;

Then postgres-migrations would create the migrations table in the username schema, and this would work just fine.

Ah! Excellent, I wasn't aware of this. It's a real shame that the schema wasn't make explicit to begin with, I don't like that the schema it's in is unpredictable.

I guess if we add the schema as a user option, we'd have to default to omitting the schema and leaving it implicit.

Would you be able to add a test for the following:

• create a migrations table in another schema
• assert that a simple migration runs

And check that this fails without your change, and succeeds with it?

Thanks!

[mbyrne00]

This PR would solve the problem nicely that we have. We've got around supporting another schema by setting the search path for postgres via process.env.PGOPTIONS = -c search_path=<my-schema-name> and this allows migrations to be written to our custom schema. The problem comes if we include another library like graphile-worker which runs first, and runs migrations using the same table name migrations in its own schema. When postgres-migrations starts up it sees there is a table with that name in the db and then when it tries to run migrations it fails to find it.

Confirmed manually that this fix resolves the issue. What do we need to do in order to get this merged?

[ThomWright]

Hi @mbyrne00, thanks for your interest in moving this PR forwards!

I think the main thing is tests.

I'm pretty sure this change is backwards compatible, but ideally I'd like the upgrade path tested.

I think we'll then be able to achieve #40 / #41 - support for defining the schema for the migrations table as a user-supplied option.

As I said above, I think this would be a good test:

• create a migrations table in another schema
• assert that a simple migration runs
• check that this fails without this change, and succeeds with it

[slifty]

Just to connect the dot explicitly -- I opened Issue Request #93 which applies this patch along with the requested tests.