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

[5.3] Fix support for multi-schema search_path in Postgres #15535

Merged
merged 1 commit into from
Sep 21, 2016

Conversation

dcro
Copy link
Contributor

@dcro dcro commented Sep 21, 2016

Following my bug report from #8639, the PostgresConnector class was modified to support a multi-schema search_path if the connection's schema configuration value is an array, e.g.

'connections' => [
    'postgres' => [
        // ... other options here ...
        'schema' => ['app', 'public'],
    ],
],

That fix does correctly set the search_path to the specified one but using an array instead of a string for the schema now produces an error in artisan migrations.

Running php artisan migrate now trows the following:

[Illuminate\Database\QueryException]
Array to string conversion (SQL: select * from information_schema.tables where table_schema = app and table_name = migrations)

The fix submitted with this pull request addresses this issue by using the first schema specified in the schema configuration array as the default schema for artisan migrations.

Users are still able to override this default schema if needed by specifying the schema in the migrations configuration options:

 'migrations' => 'public.migrations',

or by specifying a schema in the actual migration script:

public function up()
{
    Schema::create('public.users', function (Blueprint $table) {
        // ... migration ....
    }
}

@dcro dcro changed the title Fix support for multi-schema search_path in Postgres [5.3] Fix support for multi-schema search_path in Postgres Sep 21, 2016
@taylorotwell
Copy link
Member

What is the purpose of using an array for the schema in your configuration file?

@taylorotwell
Copy link
Member

Can you explain more about why the array would be used in the first place... is it supposed to fall back to the second schema?

@dcro
Copy link
Contributor Author

dcro commented Sep 21, 2016

Yes, it's basically a fallback as it sets the search_path in PostgreSQL to search multiple schemas. To quote the Postgres docs:

This variable specifies the order in which schemas are searched when an object (table, data type, function, etc.) is referenced by a simple name with no schema specified.

In 5.0 this used to work without an array but sometime before 5.1 it was modified due to issues with improper quoting and was later transformed to take an array which seems to work ok now, apart from the migration issues that are addressed with this pull request.

@taylorotwell
Copy link
Member

So wouldn't the proper behavior be for it to check each schema in this case - not just the first one?

@dcro
Copy link
Contributor Author

dcro commented Sep 21, 2016

I think that actually depends on the use case:

Use Case 1

You want to check if a table exists in the specified search_path. In this case hasTable should indeed check each schema or simply avoid specifying a schema to let Postgres handle this on it's own.

Use Case 2

You do migrations and want to be able to create a table with the same name in multiple schemas.

For example in a multi-tenant app, where each tenant has it's own schema and there's also a defaults schema, you can use the multi-schema search_path as a fallback mechanism to some default table content while still having the option to override that default table with a tenant specific one.

My use case is actually the second one as I don't need to know if the table exists in other schemas since migrations are always performed in the primary schema (the first one). Also, based on the feedback from other users on #8639 it seems they also had similar use cases.

One possible solution to cover both use cases would probably be for hasTable to support both tables with schemas specified (hasTable('schema.table')) as well as tables without any schema (hasTable('table')). The first one would only check the specified schema while the second one would check all search_path schemas. The same would apply for Schema::create(), Schema::drop(), etc.

Let me know your thoughts on this and I can look into updating this pull request with such changes.

@taylorotwell taylorotwell merged commit f0266cb into laravel:5.3 Sep 21, 2016
@taylorotwell
Copy link
Member

I'll merge this for now and we'll see if the other use case arises.

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.

None yet

2 participants