Filter 'public' schema from listSchemaNames #5600
Conversation
As postgresql expects public to be existing as it is the default schema https://www.postgresql.org/docs/current/ddl-schemas.html#DDL-SCHEMAS-CATALOG Application code is not supposed to play with, the same way as we don't return other system-related schemas Fixes doctrine#5596
| assert($this->schemaManager instanceof PostgreSQLSchemaManager); | ||
|
|
||
| $names = $this->schemaManager->getSchemaNames(); | ||
|
|
||
| self::assertContains('public', $names, 'The public schema should be found.'); |
There was a problem hiding this comment.
This behavior was fine up until 3.4.0, so probably the root cause is somewhere else. Could you implement a test that would pass prior to 3.4.0 and fails now? This change doesn't look right.
There was a problem hiding this comment.
I really do think the root cause is here and it was just simply not "important" before
doctrine/migrations#441
doctrine/migrations#1196 (comment)
#1110
#1188 (comment)
doctrine/orm#4777
(and yes my migrations in my projects contain all CREATE SCHEMA public in down migration for some years now, at least 2018 with even commit specifically deleting them from down migration which I think everyone was doing)
it becomes important with 3.4.0 because now the schema list is used by dbal for its "drop" component , so we now (3.4+) "correctly" drop schemas created by the application but as a side effect we incorrectly drop the public schema too.
There was a problem hiding this comment.
and actually these problems to "create schema public is added in down migration" is specifically because of that faulty behaviour
because if 'public' is reported in schema list but as nothing report it in the application as needing it (as you don't need to explicitly precise something is in public schema, not precising schema = public schema ) , then "up" would have deleted it
BUT it was not reported by the stuff generating DROP , only the part generating "CREATE SCHEMA"
I can see this behaviour for other schema as well
in my migration Version20201111213025.php I can see
/**
* Auto-generated Migration: Please modify to your needs!
*/
final class Version20201111213025 extends AbstractMigration
{
public function getDescription() : string
{
return '';
}
public function up(Schema $schema) : void
{
// this up() migration is auto-generated, please modify it to your needs
$this->addSql('CREATE SCHEMA IF NOT EXISTS schema_name');
}
public function down(Schema $schema) : void
{
// this down() migration is auto-generated, please modify it to your needs
$this->addSql('CREATE SCHEMA public');
}
which fit the explanation above (i.e it generates "create" but not "drop" )
There was a problem hiding this comment.
Could you implement a test that would pass prior to 3.4.0 and fails now? This change doesn't look right.
Checking that Schema\Schema::toDropSql should not generate a "DROP SCHEMA public" ? would that be good for you
the core of the issue is "Do we agree that public is not to be touched by the application layer ?"
There was a problem hiding this comment.
From #5596 (comment):
actually I think there's different face of the main behaviour change the fact that DropSchemaSqlCollector was not collecting schema/namespace at all.
@allan-simon if that's the change that caused the regression, should the new builder be adjusted accordingly and not drop any schemas?
Checking that
Schema\Schema::toDropSqlshould not generate a "DROP SCHEMA public" ? would that be good for youthe core of the issue is "Do we agree that
publicis not to be touched by the application layer ?"
No. We need to address the root cause, not a particular symptom.
I really do think the root cause is here and it was just simply not "important" before
The public schema is just another schema alongside others. Why do you think the right behavior is to return all schemas except for specifically public? Note that this method may be used by other applications for different purposes, so changing its behavior to address a symptom of another issue sounds like a bad idea to me.
There was a problem hiding this comment.
I recommend that those who are affected by the issue focus on providing the steps to reproduce it instead of making code changes. You can build a test project like it was done in #5572 or #5584 (comment).
I will not consider any related code changes until the issue is reproduced in an isolated environment.
There was a problem hiding this comment.
No. We need to address the root cause, not a particular symptom.
that's exactly what I mean, so we're aligned, so let's address it :)
the root cause is I disagree with your statement The public schema is just another schema alongside others
-> no it is not. the public schema is the default schema provided by postgresql in any single installation of postgresql, the same as you already don't return "information_schema" and any namespace starting with pg_ , why is public different from them ?
There was a problem hiding this comment.
Why do you think the right behavior is to return all schemas except for specifically public?
i already answered that :(
- we already filter out schema that are managed by postgres
- public is a special one (cf my PR description)
There was a problem hiding this comment.
@allan-simon if that's the change that caused the regression, should the new builder be adjusted accordingly and not drop any schemas?
no because then it will continue to want to create them (and we will continue to have bug report saying that doctrine migration etc. are trying to:
create schema publicin down migration without adrop schema publicin theupmigrationcreate schema my_application_specific_schemain up migration without a drop indown
-> in both case you have up and down migration that needs to edited manually (I made a quick survey around my php friends who also use postgresql and all have said "ah yes i have been manually editing migration for the last 5 years to remove these sneaky create schema" , of course I don't have formal proof of that outside of the numerous github issue I've found and documented in my comment above )
There was a problem hiding this comment.
I will not consider any related code changes until the issue is reproduced in an isolated environment.
I don't have any issue with that, I will spend the time as soon as we agree (or reach a strong disagreement) on the state
PUBLIC is a special schema of postgres , and as all special internal stuff of postgres, should be left untouched by application code and tool
|
@morozov I disagree with your position as you've completly ignored my link to other issues stating it erronously does "CREATE SCHEMA" , so your fix just go back to the previous status quo , without further explanation which is a bit rude ... |
|
@allan-simon your pull request claims to fix #5596 which should be fixed by #5604. Can we close that issue? If you want to address other issues, please feel free to report them and work on addressing them. Do you want this pull request to be reopened? |
|
Closing as all related DBAL issues are closed. Before proposing any related changes again, please see #5609 (comment). |
Fixes #5596
Summary
Filter 'public' schema from listSchemaNames
As postgresql expects public to be existing as it is the default schema
https://www.postgresql.org/docs/current/ddl-schemas.html#DDL-SCHEMAS-CATALOG
Application code is not supposed to play with, the same way as we don't
return other system-related schemas