search

laravel / ideas

Issues board used for Laravel internals discussions.
939 stars 28 forks source link

Fix lingering inconsistencies in PostgreSQL implementation #2438

Closed cbj4074 closed 3 years ago

cbj4074 commented 3 years ago

I've been working diligently as of late to clean-up Laravel's PostgreSQL implementation, specifically as it relates to schemas and the search_path.

There are, however, a few lingering inconsistencies in how the search_path is treated, which have existed since at least Laravel 6.x:

  1. As of https://github.com/laravel/framework/commit/5b7a7ecfa43a296fc5a3cd9f76f50db958928523 , compileGetAllTables($schema) and compileGetAllViews($schema) methods query every schema in the search_path, and are called from within dropAllTables() and dropAllViews(), respectively. This means that if the search_path contains more than one schema, the tables or views in all of those schemas would be dropped. This behavior might not be anticipated nor desired. In fact, it could be catastrophic if the user has access to numerous schemas, but probably has its place in testing scenarios and similar.
  2. The compileTableExists() method queries only one schema for the given table. If the table reference is qualified, the schema specified therein is checked, whereas if the table reference is unqualified, the first schema in the search_path is checked. (Checking the first schema in the search path is rather arbitrary, because that's not how PostgreSQL works, internally; it will check them all until it finds a match or fails after exhausting all schemas in the list.)
  3. The same behavior applies to the compileColumnListing() method. However, in this particular instance, it probably makes sense to check only one schema, because the use-case for listing the columns on more than one table of the same name that exists in two different schemas within the search_path is extremely limited and perhaps non-existent.

My feeling is that we should either roll-back the changes that added multi-schema support to the methods described in 1 (so that only one schema is queried in these cases), or we should update the method described in 2 to check every schema in the search_path for the table when the reference is unqualified. (I think we should leave 3 as is.) The only question to be answered is which approach is more desirable.

I should mention also that virtually none of this code has tests, so it's difficult to say whether methods like dropAllTables() or dropAllViews() even work correctly under the various conditions that apply.

@raymondtri I'd love your input here, since you contributed the changes in item 1, above. Mostly, I'm curious if you encountered a specific use-case that necessitates dropping the tables and/or views from all schemas in the search_path.

@mfn I'd appreciate your input here, too, since you seem to have a good handle on the nature and implications of the behaviors in question, based on past conversations regarding this subject.

Of course, input from anyone else is welcomed and appreciated!

If nobody else feels strongly about any of this, then I'll forge ahead with a PR that I feel best addresses the remaining issues.

cbj4074 commented 3 years ago

I spent more time with this, studying the existing implementation, and my updated assessment regarding each item is as follows:

  1. Even though it could cause catastrophic data loss in unlikely use-cases, it can also be very handy to drop all tables/views in all schemas, particularly for testing or data dumping/restoring purposes. The more I mull it over, the more I think the risk is acceptable and anyone who is calling these methods should understand the implications. (I also noticed the undocumented dont_drop configuration variable that provides a safeguard for anyone who may be aware of its existence.)
  2. Upon further consideration, this method already behaves exactly as it should, in light of the recent changes that I made to it. Despite the arbitrariness of checking only the first schema for a table's existence, doing so actually makes some sense in this instance, because that's where PostgreSQL will attempt to create any new table with an unqualified name; and given that hasTable() is most commonly used to check if a table exists before attempting to create it, the current approach is reasonable. Of course, this method will therefore never be able to "check if a table exists in any schema within the search_path", but that's probably a sacrifice worth making for the aforementioned hasTable() use-case.
  3. Again, no change required.

So, provided https://github.com/laravel/framework/pull/35588 is merged, I am happy with the implementation and consider this resolved.