From c94ac46cd8293ae723bb69d1516d19f175cf0084 Mon Sep 17 00:00:00 2001 From: Alex Ghiculescu Date: Mon, 7 Mar 2022 12:40:12 -0600 Subject: [PATCH] Remove `--no-comments` from Postgres structure dumps Reverts https://github.com/rails/rails/pull/43216 Fixes https://github.com/rails/rails/issues/44498 Per the discussion at https://github.com/rails/rails/pull/44603#discussion_r820875837, reverting the feature as it's not clear it should be the default. Users who don't want comments in structure dumps can use `ActiveRecord::Tasks::DatabaseTasks.structure_dump_flags = ['--no-comment']`. --- activerecord/CHANGELOG.md | 11 ++++++++++ .../tasks/postgresql_database_tasks.rb | 1 - .../test/cases/tasks/postgresql_rake_test.rb | 20 +++++++++---------- guides/source/active_record_postgresql.md | 13 ++++++++++++ guides/source/configuring.md | 2 +- 5 files changed, 35 insertions(+), 12 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 06a5c26410..aba83eda41 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,14 @@ +* Remove `--no-comments` flag in structure dumps for PostgreSQL + + This broke some apps that used custom schema comments. If you don't want + comments in your structure dump, you can use: + + ```ruby + ActiveRecord::Tasks::DatabaseTasks.structure_dump_flags = ['--no-comments'] + ``` + + *Alex Ghiculescu* + * Reduce the memory footprint of fixtures accessors. Until now fixtures accessors were eagerly defined using `define_method`. diff --git a/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb b/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb index 00bd6e6ae5..890d91ddcc 100644 --- a/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb +++ b/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb @@ -58,7 +58,6 @@ module ActiveRecord end args = ["--schema-only", "--no-privileges", "--no-owner"] - args << "--no-comments" if connection.database_version >= 110_000 args.concat(["--file", filename]) args.concat(Array(extra_flags)) if extra_flags diff --git a/activerecord/test/cases/tasks/postgresql_rake_test.rb b/activerecord/test/cases/tasks/postgresql_rake_test.rb index cfe31509ba..75a4065e05 100644 --- a/activerecord/test/cases/tasks/postgresql_rake_test.rb +++ b/activerecord/test/cases/tasks/postgresql_rake_test.rb @@ -375,7 +375,7 @@ if current_adapter?(:PostgreSQLAdapter) def test_structure_dump_with_env expected_env = { "PGHOST" => "my.server.tld", "PGPORT" => "2345", "PGUSER" => "jane", "PGPASSWORD" => "s3cr3t" } - expected_command = [expected_env, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--no-comments", "--file", @filename, "my-app-db"] + expected_command = [expected_env, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "my-app-db"] assert_called_with(Kernel, :system, expected_command, returns: true) do ActiveRecord::Tasks::DatabaseTasks.structure_dump( @@ -387,7 +387,7 @@ if current_adapter?(:PostgreSQLAdapter) def test_structure_dump_with_ssl_env expected_env = { "PGSSLMODE" => "verify-full", "PGSSLCERT" => "client.crt", "PGSSLKEY" => "client.key", "PGSSLROOTCERT" => "root.crt" } - expected_command = [expected_env, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--no-comments", "--file", @filename, "my-app-db"] + expected_command = [expected_env, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "my-app-db"] assert_called_with(Kernel, :system, expected_command, returns: true) do ActiveRecord::Tasks::DatabaseTasks.structure_dump( @@ -398,7 +398,7 @@ if current_adapter?(:PostgreSQLAdapter) end def test_structure_dump_with_extra_flags - expected_command = [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--no-comments", "--file", @filename, "--noop", "my-app-db"] + expected_command = [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "--noop", "my-app-db"] assert_called_with(Kernel, :system, expected_command, returns: true) do with_structure_dump_flags(["--noop"]) do @@ -408,7 +408,7 @@ if current_adapter?(:PostgreSQLAdapter) end def test_structure_dump_with_hash_extra_flags_for_a_different_driver - expected_command = [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--no-comments", "--file", @filename, "my-app-db"] + expected_command = [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "my-app-db"] assert_called_with(Kernel, :system, expected_command, returns: true) do with_structure_dump_flags({ mysql2: ["--noop"] }) do @@ -418,7 +418,7 @@ if current_adapter?(:PostgreSQLAdapter) end def test_structure_dump_with_hash_extra_flags_for_the_correct_driver - expected_command = [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--no-comments", "--file", @filename, "--noop", "my-app-db"] + expected_command = [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "--noop", "my-app-db"] assert_called_with(Kernel, :system, expected_command, returns: true) do with_structure_dump_flags({ postgresql: ["--noop"] }) do @@ -436,7 +436,7 @@ if current_adapter?(:PostgreSQLAdapter) assert_called_with( Kernel, :system, - [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--no-comments", "--file", @filename, "-T", "foo", "-T", "bar", "my-app-db"], + [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "-T", "foo", "-T", "bar", "my-app-db"], returns: true ) do ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, @filename) @@ -450,7 +450,7 @@ if current_adapter?(:PostgreSQLAdapter) assert_called_with( Kernel, :system, - [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--no-comments", "--file", @filename, "--schema=foo", "--schema=bar", "my-app-db"], + [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "--schema=foo", "--schema=bar", "my-app-db"], returns: true ) do ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, @filename) @@ -463,7 +463,7 @@ if current_adapter?(:PostgreSQLAdapter) assert_called_with( Kernel, :system, - [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--no-comments", "--file", @filename, "my-app-db"], + [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "my-app-db"], returns: true ) do with_dump_schemas(:all) do @@ -476,7 +476,7 @@ if current_adapter?(:PostgreSQLAdapter) assert_called_with( Kernel, :system, - [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--no-comments", "--file", @filename, "--schema=foo", "--schema=bar", "my-app-db"], + [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "--schema=foo", "--schema=bar", "my-app-db"], returns: true ) do with_dump_schemas("foo,bar") do @@ -490,7 +490,7 @@ if current_adapter?(:PostgreSQLAdapter) assert_called_with( Kernel, :system, - [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--no-comments", "--file", filename, "my-app-db"], + [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", filename, "my-app-db"], returns: nil ) do e = assert_raise(RuntimeError) do diff --git a/guides/source/active_record_postgresql.md b/guides/source/active_record_postgresql.md index 181636d66a..08e0c67741 100644 --- a/guides/source/active_record_postgresql.md +++ b/guides/source/active_record_postgresql.md @@ -639,3 +639,16 @@ irb> Article.count NOTE: This application only cares about non-archived `Articles`. A view also allows for conditions so we can exclude the archived `Articles` directly. + +Structure dumps +-------------- + +If your `config.active_record.schema_format` is `:sql`, Rails will call `pg_dump` to generate a +structure dump. + +You can use `ActiveRecord::Tasks::DatabaseTasks.structure_dump_flags` to configure `pg_dump`. +For example, to exclude comments from your structure dump, add this to an initializer: + +```ruby +ActiveRecord::Tasks::DatabaseTasks.structure_dump_flags = ['--no-comments'] +``` diff --git a/guides/source/configuring.md b/guides/source/configuring.md index d3db6ba968..d066cc114a 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -976,7 +976,7 @@ should be large enough to accommodate both the foreground threads (.e.g web serv Controls whether the Active Record MySQL adapter will consider all `tinyint(1)` columns as booleans. Defaults to `true`. -#### `ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.create_unlogged_table` +#### `ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.create_unlogged_tables` Controls whether database tables created by PostgreSQL should be "unlogged", which can speed up performance but adds a risk of data loss if the database crashes. It is