From 859d9b75a7c1cb080889c3b712e918506d15d0fe Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Tue, 16 Jun 2020 15:59:35 +0200 Subject: [PATCH] FIX: Restoring backup from PG12 could fail on PG10 The `EXECUTE FUNCTION` syntax for `CREATE TRIGGER` statements was introduced in PostgreSQL 11. We need to replace `EXECUTE FUNCTION` with `EXECUTE PROCEDURE` in order to be able to restore backups created with PG12 on PG10. --- lib/backup_restore.rb | 4 ++ lib/backup_restore/database_restorer.rb | 11 +++- spec/fixtures/db/restore/trigger.sql | 55 +++++++++++++++++++ .../backup_restore/database_restorer_spec.rb | 45 +++++++++++++++ 4 files changed, 112 insertions(+), 3 deletions(-) create mode 100644 spec/fixtures/db/restore/trigger.sql diff --git a/lib/backup_restore.rb b/lib/backup_restore.rb index 4d75734c27d..5b18ef603b1 100644 --- a/lib/backup_restore.rb +++ b/lib/backup_restore.rb @@ -82,6 +82,10 @@ module BackupRestore ActiveRecord::Migrator.current_version end + def self.postgresql_major_version + DB.query_single("SHOW server_version").first[/\d+/].to_i + end + def self.move_tables_between_schemas(source, destination) owner = database_configuration.username diff --git a/lib/backup_restore/database_restorer.rb b/lib/backup_restore/database_restorer.rb index 929e318a9f4..9ccb6a5f15d 100644 --- a/lib/backup_restore/database_restorer.rb +++ b/lib/backup_restore/database_restorer.rb @@ -95,7 +95,8 @@ module BackupRestore raise DatabaseRestoreError.new("psql failed: #{last_line}") if Process.last_status&.exitstatus != 0 end - # Removes unwanted SQL added by certain versions of pg_dump. + # Removes unwanted SQL added by certain versions of pg_dump and modifies + # the dump so that it works on the current version of PostgreSQL. def sed_command unwanted_sql = [ "DROP SCHEMA", # Discourse <= v1.5 @@ -104,11 +105,15 @@ module BackupRestore "SET default_table_access_method" # PostgreSQL 12 ].join("|") - "sed -E '/^(#{unwanted_sql})/d'" + command = "sed -E '/^(#{unwanted_sql})/d' #{@db_dump_path}" + if BackupRestore.postgresql_major_version < 11 + command = "#{command} | sed -E 's/^(CREATE TRIGGER.+EXECUTE) FUNCTION/\\1 PROCEDURE/'" + end + command end def restore_dump_command - "#{sed_command} #{@db_dump_path} | #{self.class.psql_command} 2>&1" + "#{sed_command} | #{self.class.psql_command} 2>&1" end def self.psql_command diff --git a/spec/fixtures/db/restore/trigger.sql b/spec/fixtures/db/restore/trigger.sql new file mode 100644 index 00000000000..78621ecaf6a --- /dev/null +++ b/spec/fixtures/db/restore/trigger.sql @@ -0,0 +1,55 @@ +-- +-- PostgreSQL database dump +-- + +-- Dumped from database version 12.2 (Debian 12.2-2.pgdg100+1) +-- Dumped by pg_dump version 12.2 (Debian 12.2-2.pgdg100+1) + +-- Started on 2020-06-15 08:06:34 UTC + +SET statement_timeout = 0; +SET lock_timeout = 0; +SET idle_in_transaction_session_timeout = 0; +SET client_encoding = 'UTF8'; +SET standard_conforming_strings = on; +SELECT pg_catalog.set_config('search_path', '', false); +SET check_function_bodies = false; +SET xmloption = content; +SET client_min_messages = warning; +SET row_security = off; + +-- +-- TOC entry 5 (class 2615 OID 2200) +-- Name: public; Type: SCHEMA; Schema: -; Owner: - +-- + +CREATE SCHEMA public; + + +-- +-- TOC entry 7007 (class 0 OID 0) +-- Dependencies: 5 +-- Name: SCHEMA public; Type: COMMENT; Schema: -; Owner: - +-- + +COMMENT ON SCHEMA public IS 'standard public schema'; + +SET default_tablespace = ''; + +SET default_table_access_method = heap; + +-- +-- TOC entry 198 (class 1259 OID 16585) +-- Name: foo; Type: TABLE; Schema: public; Owner: - +-- + +CREATE TABLE public.foo ( + id integer NOT NULL, + topic_id integer, + user_id integer +); + + +CREATE TRIGGER foo_topic_id_readonly BEFORE INSERT OR UPDATE OF redeemed_at ON public.foo FOR EACH ROW WHEN ((new.topic_id IS NOT NULL)) EXECUTE FUNCTION discourse_functions.raise_foo_topic_id_readonly(); + +CREATE TRIGGER foo_user_id_readonly BEFORE INSERT OR UPDATE OF user_id ON public.foo FOR EACH ROW WHEN ((new.user_id IS NOT NULL)) EXECUTE FUNCTION discourse_functions.raise_foo_user_id_readonly(); diff --git a/spec/lib/backup_restore/database_restorer_spec.rb b/spec/lib/backup_restore/database_restorer_spec.rb index 8a7b9ec8a94..09c203878b6 100644 --- a/spec/lib/backup_restore/database_restorer_spec.rb +++ b/spec/lib/backup_restore/database_restorer_spec.rb @@ -127,6 +127,51 @@ describe BackupRestore::DatabaseRestorer do end end + context "rewrites database dump" do + let(:logger) do + Class.new do + attr_reader :log_messages + + def initialize + @log_messages = [] + end + + def log(message, ex = nil) + @log_messages << message if message + end + end.new + end + + def restore_and_log_output(filename) + path = File.join(Rails.root, "spec/fixtures/db/restore", filename) + BackupRestore::DatabaseRestorer.stubs(:psql_command).returns("cat") + execute_stubbed_restore(stub_psql: false, dump_file_path: path) + logger.log_messages.join("\n") + end + + it "replaces `EXECUTE FUNCTION` when restoring on PostgreSQL < 11" do + BackupRestore.stubs(:postgresql_major_version).returns(10) + log = restore_and_log_output("trigger.sql") + + expect(log).not_to be_blank + expect(log).not_to match(/CREATE SCHEMA public/) + expect(log).not_to match(/EXECUTE FUNCTION/) + expect(log).to match(/^CREATE TRIGGER foo_topic_id_readonly .+? EXECUTE PROCEDURE discourse_functions.raise_foo_topic_id_readonly/) + expect(log).to match(/^CREATE TRIGGER foo_user_id_readonly .+? EXECUTE PROCEDURE discourse_functions.raise_foo_user_id_readonly/) + end + + it "does not replace `EXECUTE FUNCTION` when restoring on PostgreSQL >= 11" do + BackupRestore.stubs(:postgresql_major_version).returns(11) + log = restore_and_log_output("trigger.sql") + + expect(log).not_to be_blank + expect(log).not_to match(/CREATE SCHEMA public/) + expect(log).not_to match(/EXECUTE PROCEDURE/) + expect(log).to match(/^CREATE TRIGGER foo_topic_id_readonly .+? EXECUTE FUNCTION discourse_functions.raise_foo_topic_id_readonly/) + expect(log).to match(/^CREATE TRIGGER foo_user_id_readonly .+? EXECUTE FUNCTION discourse_functions.raise_foo_user_id_readonly/) + end + end + context "database connection" do it 'reconnects to the correct database', type: :multisite do RailsMultisite::ConnectionManagement.establish_connection(db: 'second')