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

db:wipe doesn't remove migrations table from second schema #41483

Closed
abelharisov opened this issue Mar 15, 2022 · 13 comments · Fixed by #41701
Closed

db:wipe doesn't remove migrations table from second schema #41483

abelharisov opened this issue Mar 15, 2022 · 13 comments · Fixed by #41701

Comments

@abelharisov
Copy link

abelharisov commented Mar 15, 2022

  • Laravel Version: 9.4.1
  • PHP Version: 8.1.3
  • Database Driver & Version: postgres

Description:

We have two schemas in our app. During update to laravel 9 we updated postgress config. schema was replaced with search_path with value schemaA,schemaB
Command db:wipe clears all tables in both schemas except migrations table in second schema. We have to call this command twice to clear DB

@driesvints
Copy link
Member

Please post your database.php file.

@cbj4074 since you worked on this: do you have any idea here?

@abelharisov
Copy link
Author

abelharisov commented Mar 15, 2022

database.php:

<?php

return [
    'default' => env('DB_CONNECTION', 'pgsql'),

    'connections' => [
        'pgsql' => [
            'driver' => 'pgbouncer',
            'host' => env('DB_HOST', '...'),
            'port' => env('DB_PORT', '...'),
            'database' => env('DB_DATABASE', '...'),
            'username' => env('DB_USERNAME', '...'),
            'password' => env('DB_PASSWORD', ''),
            'charset' => 'utf8',
            'prefix' => '',
            'search_path' => 'public,common',
            'sslmode' => 'prefer',
            'options' => [
                PDO::ATTR_EMULATE_PREPARES => true,
            ],
        ],
    ],

    'migrations' => 'migrations',
];

@driesvints
Copy link
Member

Does it work if you try 'search_path' => '"public","common"',?

@abelharisov
Copy link
Author

No, one migrations table is left after wipe:
image

@driesvints
Copy link
Member

And this definitely worked in Laravel 8?

@abelharisov
Copy link
Author

No, it didn't work in Laravel 8 at all. The second schema was ignored by wipe. We used to call db:wipe twice with different connections (for each schema).
When we switched to search_path it started to work much better - all tables in all schemas were removed, except migrations in the second schema

@driesvints
Copy link
Member

I see. Thanks for that info. I currently do not now how to solve this one.

@cbj4074
Copy link
Contributor

cbj4074 commented Mar 15, 2022

Absolutely; I'm happy to take a look at this, but it will probably be later today or tomorrow.

@bdaler
Copy link

bdaler commented Mar 16, 2022

Maybe in this line drop table should be with a schema name? Something like: drop table schemaA.tableName cascade;

@cbj4074
Copy link
Contributor

cbj4074 commented Mar 16, 2022

Finally getting a chance to dig into this in earnest...

As is the case with all things search_path-related, the fix is likely to be nuanced and will have to be vetted carefully so as not to break anything else, as not all of the affected code has test coverage (particularly the older code).

It doesn't look like I've touched any of the db:wipe-related functionality to date (only schema:dump), but my hunch is that the fix here will be similar.

For anyone else who is interested, #36046 contains a lot of background information regarding schema:dump, much of which is probably relevant to this issue.

I'm hoping to be able to propose a fix today.

@cbj4074
Copy link
Contributor

cbj4074 commented Mar 16, 2022

Okay, I've had a chance to assess the issue pretty thoroughly at this point.

This is a complicated problem and understanding #36046 is important to ascertaining why.

@abelharisov , artisan db:wipe only "kind-of works" for you because the table names across your two schemas are unique, with the exception of migrations (which is why that one is not dropped from the second schema). That is to say, if you had the exact same tables in both schemas, only the tables in the first schema listed would be dropped, and none of the tables in any subsequent schema with the same name would be dropped.

The reason for this is rooted in the following method:

public function getAllTables()
{
return $this->connection->select(
$this->grammar->compileGetAllTables(
$this->parseSearchPath(
$this->connection->getConfig('search_path') ?: $this->connection->getConfig('schema')
)
)
);
}

In a multi-schema configuration, if one were to run php artisan migrate on two different schemas, so that both contain the exact same tables, this method will return something like the following:

select tablename from pg_catalog.pg_tables where schemaname in ('laravel','public');
       tablename
------------------------
 migrations
 users
 password_resets
 password_resets
 migrations
 users
 failed_jobs
 personal_access_tokens
 failed_jobs
 personal_access_tokens
(10 rows)

The dropAllTables() method (by way of the compileDropAllTables() method) simply implodes that list and executes it in a single SQL statement, the effect of which is to drop each unique table name from the first schema in which it is found.

An important point of note here is that if the DROP TABLE ... command were run $y times, where $y is simply the number of schemas defined in the search_path, it would drop any of those table names from every schema. The reason for this is not straightforward: with each DROP TABLE ... invocation, the tables with those names are dropped from the first schema in which they are found, and then with each successive DROP TABLE ... execution, the previously-emptied schema would be skipped, and the tables would be dropped from the next schema in the search_path, and so on.

That said, I don't think that would necessarily be the best or most intuitive way to handle this.

Rather,

public function compileGetAllTables($searchPath)
{
return "select tablename from pg_catalog.pg_tables where schemaname in ('".implode("','", (array) $searchPath)."')";
}
could be modified to include the schemaname, e.g.:

postgres=# select schemaname, tablename, CONCAT(schemaname, '.', tablename) AS fully_qualified_name from pg_catalog.pg_tables where schemaname in ('laravel','public');
 schemaname |       tablename        |      fully_qualified_name                                                                                                       
------------+------------------------+--------------------------------                                                                                                 
 laravel    | migrations             | laravel.migrations                                                                                                              
 laravel    | users                  | laravel.users                                                                                                                   
 laravel    | password_resets        | laravel.password_resets                                                                                                         
 public     | password_resets        | public.password_resets                                                                                                          
 public     | migrations             | public.migrations                                                                                                               
 public     | users                  | public.users                                                                                                                    
 public     | failed_jobs            | public.failed_jobs                                                                                                              
 public     | personal_access_tokens | public.personal_access_tokens                                                                                                   
 laravel    | failed_jobs            | laravel.failed_jobs                                                                                                             
 laravel    | personal_access_tokens | laravel.personal_access_tokens                                                                                                  
(10 rows)                                                                                                                                                              

We could then target the various tables in the appropriate schemas. However, there were specific reasons for which I didn't modify that method in this way previously, which I'll do my best to articulate in a subsequent post, as I'm out of time for today. (The gist of it was that while it might be desirable in this specific scenario, it could be undesirable to use the fully-qualified object names in others, in which doing so would actually mitigate and work against the usefulness of the search_path's intent.)

I'll continue to think through possible solutions to this and post another update tomorrow.

@cbj4074
Copy link
Contributor

cbj4074 commented Mar 18, 2022

I have a draft PR, #41541 , which should resolve this issue (and potentially other yet-to-be-identified issues of a similar nature), if anybody wants take it for a spin before I submit it for review. "Works for me!", of course. ;-)

@abelharisov
Copy link
Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants