From 8041e666cf1eb375ce7853ccccace048d2b9776c Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Thu, 4 Feb 2021 22:05:10 +0300 Subject: [PATCH 01/20] tests: fix tests.auth_test.SimpleAuthTest.test_backup_via_unprivileged_user --- tests/auth_test.py | 58 ++++++++++++---------------------------------- 1 file changed, 15 insertions(+), 43 deletions(-) diff --git a/tests/auth_test.py b/tests/auth_test.py index eca62316..c84fdb98 100644 --- a/tests/auth_test.py +++ b/tests/auth_test.py @@ -34,10 +34,8 @@ class SimpleAuthTest(ProbackupTest, unittest.TestCase): node = self.make_simple_node( base_dir=os.path.join(module_name, fname, 'node'), set_replication=True, - initdb_params=['--data-checksums'], - pg_options={ - 'max_wal_senders': '2'} - ) + initdb_params=['--data-checksums']) + backup_dir = os.path.join(self.tmp_path, module_name, fname, 'backup') self.init_pb(backup_dir) self.add_instance(backup_dir, 'node', node) @@ -64,7 +62,15 @@ class SimpleAuthTest(ProbackupTest, unittest.TestCase): "GRANT EXECUTE ON FUNCTION" " pg_start_backup(text, boolean, boolean) TO backup;") - time.sleep(1) + if self.get_version(node) < 100000: + node.safe_psql( + 'postgres', + "GRANT EXECUTE ON FUNCTION pg_catalog.pg_switch_xlog() TO backup") + else: + node.safe_psql( + 'postgres', + "GRANT EXECUTE ON FUNCTION pg_catalog.pg_switch_wal() TO backup") + try: self.backup_node( backup_dir, 'node', node, options=['-U', 'backup']) @@ -84,8 +90,6 @@ class SimpleAuthTest(ProbackupTest, unittest.TestCase): "GRANT EXECUTE ON FUNCTION" " pg_create_restore_point(text) TO backup;") - time.sleep(1) - try: self.backup_node( backup_dir, 'node', node, options=['-U', 'backup']) @@ -129,50 +133,18 @@ class SimpleAuthTest(ProbackupTest, unittest.TestCase): node.stop() node.slow_start() - try: - self.backup_node( - backup_dir, 'node', node, options=['-U', 'backup']) - self.assertEqual( - 1, 0, - "Expecting Error due to missing grant on clearing ptrack_files.") - except ProbackupException as e: - self.assertIn( - "ERROR: must be superuser or replication role to clear ptrack files\n" - "query was: SELECT pg_catalog.pg_ptrack_clear()", e.message, - '\n Unexpected Error Message: {0}\n CMD: {1}'.format( - repr(e.message), self.cmd)) - - time.sleep(1) - - try: - self.backup_node( - backup_dir, 'node', node, - backup_type='ptrack', options=['-U', 'backup']) - self.assertEqual( - 1, 0, - "Expecting Error due to missing grant on clearing ptrack_files.") - except ProbackupException as e: - self.assertIn( - "ERROR: must be superuser or replication role read ptrack files\n" - "query was: select pg_catalog.pg_ptrack_control_lsn()", e.message, - '\n Unexpected Error Message: {0}\n CMD: {1}'.format( - repr(e.message), self.cmd)) - node.safe_psql( "postgres", "ALTER ROLE backup REPLICATION") - time.sleep(1) - # FULL self.backup_node( - backup_dir, 'node', node, - options=['-U', 'backup']) + backup_dir, 'node', node, options=['-U', 'backup']) # PTRACK - self.backup_node( - backup_dir, 'node', node, - backup_type='ptrack', options=['-U', 'backup']) +# self.backup_node( +# backup_dir, 'node', node, +# backup_type='ptrack', options=['-U', 'backup']) # Clean after yourself self.del_test_dir(module_name, fname) From 46c14304d758d713f7204e9e68c996c9f6457af0 Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Mon, 8 Feb 2021 06:00:51 +0300 Subject: [PATCH 02/20] [Issue #320] incorrect crc calculation for pg_filenode.map --- tests/incr_restore.py | 110 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/tests/incr_restore.py b/tests/incr_restore.py index 3aa84121..6246ae19 100644 --- a/tests/incr_restore.py +++ b/tests/incr_restore.py @@ -10,6 +10,7 @@ import hashlib import shutil import json from testgres import QueryException +from distutils.dir_util import copy_tree module_name = 'incr_restore' @@ -2390,5 +2391,114 @@ class IncrRestoreTest(ProbackupTest, unittest.TestCase): # Clean after yourself self.del_test_dir(module_name, fname, [node2]) + def test_incremental_pg_filenode_map(self): + """""" + fname = self.id().split('.')[3] + backup_dir = os.path.join(self.tmp_path, module_name, fname, 'backup') + node = self.make_simple_node( + base_dir=os.path.join(module_name, fname, 'node'), + initdb_params=['--data-checksums']) + + self.init_pb(backup_dir) + self.add_instance(backup_dir, 'node', node) + self.set_archiving(backup_dir, 'node', node) + node.slow_start() + + node1 = self.make_simple_node( + base_dir=os.path.join(module_name, fname, 'node1'), + initdb_params=['--data-checksums']) + node1.cleanup() + + node.pgbench_init(scale=5) + + # FULL backup + backup_id = self.backup_node(backup_dir, 'node', node) + + # in node1 restore full backup + self.restore_node(backup_dir, 'node', node1) + self.set_auto_conf(node1, {'port': node1.port}) + node1.slow_start() + + pgbench = node.pgbench( + stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + options=['-T', '10', '-c', '1']) + + pgbench = node1.pgbench( + stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + options=['-T', '10', '-c', '1']) + + node.safe_psql( + 'postgres', + 'reindex index pg_type_oid_index') + + # FULL backup + backup_id = self.backup_node(backup_dir, 'node', node) + + node1.stop() + + + # incremental restore into node1 + self.restore_node(backup_dir, 'node', node1, options=["-I", "checksum", '--log-level-file=VERBOSE']) + + self.set_auto_conf(node1, {'port': node1.port}) + node1.slow_start() + + node1.safe_psql( + 'postgres', + 'select 1') + + # Clean after yourself + self.del_test_dir(module_name, fname) + + + def test_incr_backup_filenode_map(self): + """""" + fname = self.id().split('.')[3] + backup_dir = os.path.join(self.tmp_path, module_name, fname, 'backup') + node = self.make_simple_node( + base_dir=os.path.join(module_name, fname, 'node'), + initdb_params=['--data-checksums']) + + self.init_pb(backup_dir) + self.add_instance(backup_dir, 'node', node) + self.set_archiving(backup_dir, 'node', node) + node.slow_start() + + node1 = self.make_simple_node( + base_dir=os.path.join(module_name, fname, 'node1'), + initdb_params=['--data-checksums']) + node1.cleanup() + + node.pgbench_init(scale=5) + + # FULL backup + backup_id = self.backup_node(backup_dir, 'node', node) + + pgbench = node.pgbench( + stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + options=['-T', '10', '-c', '1']) + + backup_id = self.backup_node(backup_dir, 'node', node, backup_type='delta') + + node.safe_psql( + 'postgres', + 'reindex index pg_type_oid_index') + + backup_id = self.backup_node( + backup_dir, 'node', node, backup_type='delta', options=['--log-level-file=VERBOSE']) + + # incremental restore into node1 + node.cleanup() + + self.restore_node(backup_dir, 'node', node) + node.slow_start() + + node.safe_psql( + 'postgres', + 'select 1') + + # Clean after yourself + self.del_test_dir(module_name, fname) + # check that MinRecPoint and BackupStartLsn are correctly used in case of --incrementa-lsn # incremental restore + partial restore. From 17cc612089bd200972a4ec735379e18bc4533a53 Mon Sep 17 00:00:00 2001 From: anastasia Date: Mon, 8 Feb 2021 15:04:51 +0300 Subject: [PATCH 03/20] tests: remove debug messages in module 'archive' --- tests/archive.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/archive.py b/tests/archive.py index 482a2502..05675065 100644 --- a/tests/archive.py +++ b/tests/archive.py @@ -1908,7 +1908,7 @@ class ArchiveTest(ProbackupTest, unittest.TestCase): output = self.show_archive( backup_dir, 'node', as_json=False, as_text=True, - options=['--log-level-console=VERBOSE']) + options=['--log-level-console=INFO']) self.assertNotIn('WARNING', output) @@ -2181,7 +2181,7 @@ class ArchiveTest(ProbackupTest, unittest.TestCase): restore_command += ' -j 2 --batch-size=10' - print(restore_command) + # print(restore_command) if node.major_version >= 12: self.set_auto_conf(replica, {'restore_command': restore_command}) @@ -2298,7 +2298,7 @@ class ArchiveTest(ProbackupTest, unittest.TestCase): dst_file = os.path.join(replica.data_dir, wal_dir, 'pbk_prefetch', filename) shutil.copyfile(src_file, dst_file) - print(dst_file) + # print(dst_file) # corrupt file if files[-2].endswith('.gz'): From b2ab1dfef8307cbce8ebd5fa9bcd7d031a99d74c Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Mon, 8 Feb 2021 18:31:00 +0300 Subject: [PATCH 04/20] [Issue #320] always backup and always restore pg_filenode.map --- src/data.c | 5 +++-- src/pg_probackup.h | 3 ++- src/restore.c | 5 +++++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/data.c b/src/data.c index d3f67f43..f7032fb5 100644 --- a/src/data.c +++ b/src/data.c @@ -711,8 +711,9 @@ backup_non_data_file(pgFile *file, pgFile *prev_file, /* * If nonedata file exists in previous backup * and its mtime is less than parent backup start time ... */ - if (prev_file && file->exists_in_prev && - file->mtime <= parent_backup_time) + if ((pg_strcasecmp(file->name, RELMAPPER_FILENAME) != 0) && + (prev_file && file->exists_in_prev && + file->mtime <= parent_backup_time)) { file->crc = fio_get_crc32(from_fullpath, FIO_DB_HOST, false); diff --git a/src/pg_probackup.h b/src/pg_probackup.h index f4adc98c..a766e705 100644 --- a/src/pg_probackup.h +++ b/src/pg_probackup.h @@ -70,7 +70,8 @@ extern const char *PROGRAM_EMAIL; #define BACKUP_RO_LOCK_FILE "backup_ro.pid" #define DATABASE_FILE_LIST "backup_content.control" #define PG_BACKUP_LABEL_FILE "backup_label" -#define PG_TABLESPACE_MAP_FILE "tablespace_map" +#define PG_TABLESPACE_MAP_FILE "tablespace_map" +#define RELMAPPER_FILENAME "pg_filenode.map" #define EXTERNAL_DIR "external_directories/externaldir" #define DATABASE_MAP "database_map" #define HEADER_MAP "page_header_map" diff --git a/src/restore.c b/src/restore.c index 3f0adf7b..b3b9965e 100644 --- a/src/restore.c +++ b/src/restore.c @@ -903,6 +903,11 @@ restore_chain(pgBackup *dest_backup, parray *parent_chain, if (parray_bsearch(dest_backup->files, file, pgFileCompareRelPathWithExternal)) redundant = false; + /* pg_filenode.map are always restored, because it's crc cannot be trusted */ + if (file->external_dir_num == 0 && + pg_strcasecmp(file->name, RELMAPPER_FILENAME) == 0) + redundant = true; + /* do not delete the useful internal directories */ if (S_ISDIR(file->mode) && !redundant) continue; From 7ecb56e7438da41b7f8b6bb6d2a04963daa74c7a Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Mon, 8 Feb 2021 22:25:58 +0300 Subject: [PATCH 05/20] [Issue #320] improve test coverage --- tests/backup.py | 51 ++++++++++++++++++++++++++++++++++++++ tests/incr_restore.py | 57 +++---------------------------------------- tests/merge.py | 52 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 53 deletions(-) diff --git a/tests/backup.py b/tests/backup.py index 8ca96609..2ee5b9ac 100644 --- a/tests/backup.py +++ b/tests/backup.py @@ -3027,3 +3027,54 @@ class BackupTest(ProbackupTest, unittest.TestCase): # Clean after yourself self.del_test_dir(module_name, fname) + + def test_incr_backup_filenode_map(self): + """ + https://github.com/postgrespro/pg_probackup/issues/320 + """ + fname = self.id().split('.')[3] + backup_dir = os.path.join(self.tmp_path, module_name, fname, 'backup') + node = self.make_simple_node( + base_dir=os.path.join(module_name, fname, 'node'), + initdb_params=['--data-checksums']) + + self.init_pb(backup_dir) + self.add_instance(backup_dir, 'node', node) + self.set_archiving(backup_dir, 'node', node) + node.slow_start() + + node1 = self.make_simple_node( + base_dir=os.path.join(module_name, fname, 'node1'), + initdb_params=['--data-checksums']) + node1.cleanup() + + node.pgbench_init(scale=5) + + # FULL backup + backup_id = self.backup_node(backup_dir, 'node', node) + + pgbench = node.pgbench( + stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + options=['-T', '10', '-c', '1']) + + backup_id = self.backup_node(backup_dir, 'node', node, backup_type='delta') + + node.safe_psql( + 'postgres', + 'reindex index pg_type_oid_index') + + backup_id = self.backup_node( + backup_dir, 'node', node, backup_type='delta') + + # incremental restore into node1 + node.cleanup() + + self.restore_node(backup_dir, 'node', node) + node.slow_start() + + node.safe_psql( + 'postgres', + 'select 1') + + # Clean after yourself + self.del_test_dir(module_name, fname) diff --git a/tests/incr_restore.py b/tests/incr_restore.py index 6246ae19..4595cbb3 100644 --- a/tests/incr_restore.py +++ b/tests/incr_restore.py @@ -2392,7 +2392,9 @@ class IncrRestoreTest(ProbackupTest, unittest.TestCase): self.del_test_dir(module_name, fname, [node2]) def test_incremental_pg_filenode_map(self): - """""" + """ + https://github.com/postgrespro/pg_probackup/issues/320 + """ fname = self.id().split('.')[3] backup_dir = os.path.join(self.tmp_path, module_name, fname, 'backup') node = self.make_simple_node( @@ -2436,9 +2438,8 @@ class IncrRestoreTest(ProbackupTest, unittest.TestCase): node1.stop() - # incremental restore into node1 - self.restore_node(backup_dir, 'node', node1, options=["-I", "checksum", '--log-level-file=VERBOSE']) + self.restore_node(backup_dir, 'node', node1, options=["-I", "checksum"]) self.set_auto_conf(node1, {'port': node1.port}) node1.slow_start() @@ -2450,55 +2451,5 @@ class IncrRestoreTest(ProbackupTest, unittest.TestCase): # Clean after yourself self.del_test_dir(module_name, fname) - - def test_incr_backup_filenode_map(self): - """""" - fname = self.id().split('.')[3] - backup_dir = os.path.join(self.tmp_path, module_name, fname, 'backup') - node = self.make_simple_node( - base_dir=os.path.join(module_name, fname, 'node'), - initdb_params=['--data-checksums']) - - self.init_pb(backup_dir) - self.add_instance(backup_dir, 'node', node) - self.set_archiving(backup_dir, 'node', node) - node.slow_start() - - node1 = self.make_simple_node( - base_dir=os.path.join(module_name, fname, 'node1'), - initdb_params=['--data-checksums']) - node1.cleanup() - - node.pgbench_init(scale=5) - - # FULL backup - backup_id = self.backup_node(backup_dir, 'node', node) - - pgbench = node.pgbench( - stdout=subprocess.PIPE, stderr=subprocess.STDOUT, - options=['-T', '10', '-c', '1']) - - backup_id = self.backup_node(backup_dir, 'node', node, backup_type='delta') - - node.safe_psql( - 'postgres', - 'reindex index pg_type_oid_index') - - backup_id = self.backup_node( - backup_dir, 'node', node, backup_type='delta', options=['--log-level-file=VERBOSE']) - - # incremental restore into node1 - node.cleanup() - - self.restore_node(backup_dir, 'node', node) - node.slow_start() - - node.safe_psql( - 'postgres', - 'select 1') - - # Clean after yourself - self.del_test_dir(module_name, fname) - # check that MinRecPoint and BackupStartLsn are correctly used in case of --incrementa-lsn # incremental restore + partial restore. diff --git a/tests/merge.py b/tests/merge.py index 44652066..bf21405b 100644 --- a/tests/merge.py +++ b/tests/merge.py @@ -2829,5 +2829,57 @@ class MergeTest(ProbackupTest, unittest.TestCase): self.del_test_dir(module_name, fname) + def test_merge_pg_filenode_map(self): + """ + https://github.com/postgrespro/pg_probackup/issues/320 + """ + fname = self.id().split('.')[3] + backup_dir = os.path.join(self.tmp_path, module_name, fname, 'backup') + node = self.make_simple_node( + base_dir=os.path.join(module_name, fname, 'node'), + initdb_params=['--data-checksums']) + + self.init_pb(backup_dir) + self.add_instance(backup_dir, 'node', node) + self.set_archiving(backup_dir, 'node', node) + node.slow_start() + + node1 = self.make_simple_node( + base_dir=os.path.join(module_name, fname, 'node1'), + initdb_params=['--data-checksums']) + node1.cleanup() + + node.pgbench_init(scale=5) + + # FULL backup + self.backup_node(backup_dir, 'node', node) + + pgbench = node.pgbench( + stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + options=['-T', '10', '-c', '1']) + + self.backup_node(backup_dir, 'node', node, backup_type='delta') + + node.safe_psql( + 'postgres', + 'reindex index pg_type_oid_index') + + backup_id = self.backup_node( + backup_dir, 'node', node, backup_type='delta') + + self.merge_backup(backup_dir, 'node', backup_id) + + node.cleanup() + + self.restore_node(backup_dir, 'node', node) + node.slow_start() + + node.safe_psql( + 'postgres', + 'select 1') + + # Clean after yourself + self.del_test_dir(module_name, fname) + # 1. Need new test with corrupted FULL backup # 2. different compression levels From def4cb4ae27d40881a713eafa7265173f61b525f Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Mon, 8 Feb 2021 22:29:21 +0300 Subject: [PATCH 06/20] [Issue #320] minor cleanup in "incr_restore" module --- tests/incr_restore.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/incr_restore.py b/tests/incr_restore.py index 4595cbb3..1d3a52df 100644 --- a/tests/incr_restore.py +++ b/tests/incr_restore.py @@ -10,7 +10,6 @@ import hashlib import shutil import json from testgres import QueryException -from distutils.dir_util import copy_tree module_name = 'incr_restore' @@ -2452,4 +2451,3 @@ class IncrRestoreTest(ProbackupTest, unittest.TestCase): self.del_test_dir(module_name, fname) # check that MinRecPoint and BackupStartLsn are correctly used in case of --incrementa-lsn -# incremental restore + partial restore. From 02a3665375f5e578f390b6dc4d60ed824d9bfcb2 Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Tue, 9 Feb 2021 17:32:27 +0300 Subject: [PATCH 07/20] [Issue #311] Release shared locks at proc exit --- src/catalog.c | 441 +++++++++++++++++++++++++++++++---------------- tests/locking.py | 59 ++++++- 2 files changed, 344 insertions(+), 156 deletions(-) diff --git a/src/catalog.c b/src/catalog.c index 8ac3e579..e1b5d770 100644 --- a/src/catalog.c +++ b/src/catalog.c @@ -26,12 +26,22 @@ static pgBackup *readBackupControlFile(const char *path); static time_t create_backup_dir(pgBackup *backup, const char *backup_instance_path); static bool backup_lock_exit_hook_registered = false; -static parray *lock_files = NULL; +static parray *locks = NULL; -static int lock_backup_exclusive(pgBackup *backup, bool strict); -static bool lock_backup_internal(pgBackup *backup, bool exclusive); -static bool lock_backup_read_only(pgBackup *backup); -static bool wait_read_only_owners(pgBackup *backup); +static int grab_excl_lock_file(const char *backup_dir, const char *backup_id, bool strict); +static int grab_shared_lock_file(pgBackup *backup); +static int wait_shared_owners(pgBackup *backup); + +static void unlock_backup(const char *backup_dir, const char *backup_id, bool exclusive); +static void release_excl_lock_file(const char *backup_dir); +static void release_shared_lock_file(const char *backup_dir); + +typedef struct LockInfo +{ + char backup_id[10]; + char backup_dir[MAXPGPATH]; + bool exclusive; +} LockInfo; static timelineInfo * timelineInfoNew(TimeLineID tli) @@ -66,28 +76,24 @@ timelineInfoFree(void *tliInfo) pfree(tliInfo); } -/* Iterate over locked backups and delete locks files */ +/* Iterate over locked backups and unlock them */ static void unlink_lock_atexit(void) { - int i; + int i; - if (lock_files == NULL) + if (locks == NULL) return; - for (i = 0; i < parray_num(lock_files); i++) + for (i = 0; i < parray_num(locks); i++) { - char *lock_file = (char *) parray_get(lock_files, i); - int res; - - res = fio_unlink(lock_file, FIO_BACKUP_HOST); - if (res != 0 && errno != ENOENT) - elog(WARNING, "%s: %s", lock_file, strerror(errno)); + LockInfo *lock = (LockInfo *) parray_get(locks, i); + unlock_backup(lock->backup_dir, lock->backup_dir, lock->exclusive); } - parray_walk(lock_files, pfree); - parray_free(lock_files); - lock_files = NULL; + parray_walk(locks, pg_free); + parray_free(locks); + locks = NULL; } /* @@ -147,39 +153,39 @@ write_backup_status(pgBackup *backup, BackupStatus status, } /* - * Lock backup in either exclusive or non-exclusive (read-only) mode. + * Lock backup in either exclusive or shared mode. * "strict" flag allows to ignore "out of space" errors and should be * used only by DELETE command to free disk space on filled up * filesystem. * - * Only read only tasks (validate, restore) are allowed to take non-exclusive locks. + * Only read only tasks (validate, restore) are allowed to take shared locks. * Changing backup metadata must be done with exclusive lock. * * Only one process can hold exclusive lock at any time. * Exlusive lock - PID of process, holding the lock - is placed in * lock file: BACKUP_LOCK_FILE. * - * Multiple proccess are allowed to take non-exclusive locks simultaneously. - * Non-exclusive locks - PIDs of proccesses, holding the lock - are placed in + * Multiple proccess are allowed to take shared locks simultaneously. + * Shared locks - PIDs of proccesses, holding the lock - are placed in * separate lock file: BACKUP_RO_LOCK_FILE. - * When taking RO lock, a brief exclusive lock is taken. + * When taking shared lock, a brief exclusive lock is taken. + * + * -> exclusive -> grab exclusive lock file and wait until all shared lockers are gone, return + * -> shared -> grab exclusive lock file, grab shared lock file, release exclusive lock file, return * * TODO: lock-timeout as parameter - * TODO: we must think about more fine grain unlock mechanism - separate unlock_backup() function. - * TODO: more accurate naming - * -> exclusive lock -> acquire HW_LATCH and wait until all LW_LATCH`es are clear - * -> shared lock -> acquire HW_LATCH, acquire LW_LATCH, release HW_LATCH */ bool lock_backup(pgBackup *backup, bool strict, bool exclusive) { - int rc; - char lock_file[MAXPGPATH]; - bool enospc_detected = false; + int rc; + char lock_file[MAXPGPATH]; + bool enospc_detected = false; + LockInfo *lock = NULL; join_path_components(lock_file, backup->root_dir, BACKUP_LOCK_FILE); - rc = lock_backup_exclusive(backup, strict); + rc = grab_excl_lock_file(backup->root_dir, base36enc(backup->start_time), strict); if (rc == 1) return false; @@ -188,54 +194,62 @@ lock_backup(pgBackup *backup, bool strict, bool exclusive) enospc_detected = true; if (strict) return false; + + /* + * If we failed to take exclusive lock due to ENOSPC, + * then in lax mode treat such condition as if lock was taken. + */ } /* * We have exclusive lock, now there are following scenarios: * - * 1. If we are for exlusive lock, then we must open the RO lock file + * 1. If we are for exlusive lock, then we must open the shared lock file * and check if any of the processes listed there are still alive. * If some processes are alive and are not going away in lock_timeout, * then return false. * * 2. If we are here for non-exlusive lock, then write the pid - * into RO lock list and release the exclusive lock. + * into shared lock file and release the exclusive lock. */ - if (lock_backup_internal(backup, exclusive)) - { - if (!exclusive) - { - /* release exclusive lock */ - if (fio_unlink(lock_file, FIO_BACKUP_HOST) < 0) - elog(ERROR, "Could not remove exclusive lock file \"%s\": %s", - lock_file, strerror(errno)); - - /* we are done */ - return true; - } - - /* When locking backup in lax exclusive mode, - * we should wait until all RO locks owners are gone. - */ - if (!strict && enospc_detected) - { - /* We are in lax mode and EONSPC was encountered: once again try to grab exclusive lock, - * because there is a chance that lock_backup_read_only may have freed some space on filesystem, - * thanks to unlinking of BACKUP_RO_LOCK_FILE. - * If somebody concurrently acquired exclusive lock first, then we should give up. - */ - if (lock_backup_exclusive(backup, strict) == 1) - return false; - - return true; - } - } + if (exclusive) + rc = wait_shared_owners(backup); else + rc = grab_shared_lock_file(backup); + + if (rc != 0) + { + /* + * Failed to grab shared lock or (in case of exclusive mode) shared lock owners + * are not going away in time, release the exclusive lock file and return in shame. + */ + release_excl_lock_file(backup->root_dir); return false; + } + + if (!exclusive) + { + /* Shared lock file is grabbed, now we can release exclusive lock file */ + release_excl_lock_file(backup->root_dir); + } + + if (exclusive && !strict && enospc_detected) + { + /* We are in lax exclusive mode and EONSPC was encountered: + * once again try to grab exclusive lock file, + * because there is a chance that release of shared lock file in wait_shared_owners may have + * freed some space on filesystem, thanks to unlinking of BACKUP_RO_LOCK_FILE. + * If somebody concurrently acquired exclusive lock file first, then we should give up. + */ + if (grab_excl_lock_file(backup->root_dir, base36enc(backup->start_time), strict) == 1) + return false; + + return true; + } /* - * Arrange to unlink the lock file(s) at proc_exit. + * Arrange the unlocking at proc_exit. */ if (!backup_lock_exit_hook_registered) { @@ -243,10 +257,16 @@ lock_backup(pgBackup *backup, bool strict, bool exclusive) backup_lock_exit_hook_registered = true; } - /* Use parray so that the lock files are unlinked in a loop */ - if (lock_files == NULL) - lock_files = parray_new(); - parray_append(lock_files, pgut_strdup(lock_file)); + /* save lock metadata for later unlocking */ + lock = pgut_malloc(sizeof(LockInfo)); + snprintf(lock->backup_id, 10, "%s", base36enc(backup->backup_id)); + snprintf(lock->backup_dir, MAXPGPATH, "%s", backup->root_dir); + lock->exclusive = exclusive; + + /* Use parray for lock release */ + if (locks == NULL) + locks = parray_new(); + parray_append(locks, lock); return true; } @@ -258,7 +278,7 @@ lock_backup(pgBackup *backup, bool strict, bool exclusive) * 2 Failed to acquire lock due to ENOSPC */ int -lock_backup_exclusive(pgBackup *backup, bool strict) +grab_excl_lock_file(const char *root_dir, const char *backup_id, bool strict) { char lock_file[MAXPGPATH]; int fd = 0; @@ -268,7 +288,7 @@ lock_backup_exclusive(pgBackup *backup, bool strict) int len; int encoded_pid; - join_path_components(lock_file, backup->root_dir, BACKUP_LOCK_FILE); + join_path_components(lock_file, root_dir, BACKUP_LOCK_FILE); /* * We need a loop here because of race conditions. But don't loop forever @@ -280,8 +300,7 @@ lock_backup_exclusive(pgBackup *backup, bool strict) FILE *fp_out = NULL; if (interrupted) - elog(ERROR, "Interrupted while locking backup %s", - base36enc(backup->start_time)); + elog(ERROR, "Interrupted while locking backup %s", backup_id); /* * Try to create the lock file --- O_EXCL makes this atomic. @@ -344,7 +363,7 @@ lock_backup_exclusive(pgBackup *backup, bool strict) if ((empty_tries % LOG_FREQ) == 0) elog(WARNING, "Waiting %u seconds on empty exclusive lock for backup %s", - empty_tries, base36enc(backup->start_time)); + empty_tries, backup_id); sleep(1); /* @@ -372,34 +391,32 @@ lock_backup_exclusive(pgBackup *backup, bool strict) */ if (encoded_pid == my_pid) return 0; + + if (kill(encoded_pid, 0) == 0) + { + /* complain every fifth interval */ + if ((ntries % LOG_FREQ) == 0) + { + elog(WARNING, "Process %d is using backup %s, and is still running", + encoded_pid, backup_id); + + elog(WARNING, "Waiting %u seconds on exclusive lock for backup %s", + ntries, backup_id); + } + + sleep(1); + + /* try again */ + continue; + } else { - if (kill(encoded_pid, 0) == 0) - { - /* complain every fifth interval */ - if ((ntries % LOG_FREQ) == 0) - { - elog(WARNING, "Process %d is using backup %s, and is still running", - encoded_pid, base36enc(backup->start_time)); - - elog(WARNING, "Waiting %u seconds on exclusive lock for backup %s", - ntries, base36enc(backup->start_time)); - } - - sleep(1); - - /* try again */ - continue; - } + if (errno == ESRCH) + elog(WARNING, "Process %d which used backup %s no longer exists", + encoded_pid, backup_id); else - { - if (errno == ESRCH) - elog(WARNING, "Process %d which used backup %s no longer exists", - encoded_pid, base36enc(backup->start_time)); - else - elog(ERROR, "Failed to send signal 0 to a process %d: %s", - encoded_pid, strerror(errno)); - } + elog(ERROR, "Failed to send signal 0 to a process %d: %s", + encoded_pid, strerror(errno)); } grab_lock: @@ -484,11 +501,14 @@ grab_lock: return 0; } -/* Wait until all read-only lock owners are gone */ -bool -wait_read_only_owners(pgBackup *backup) +/* Wait until all shared lock owners are gone + * 0 - successs + * 1 - fail + */ +int +wait_shared_owners(pgBackup *backup) { - FILE *fp = NULL; + FILE *fp = NULL; char buffer[256]; pid_t encoded_pid; int ntries = LOCK_TIMEOUT; @@ -500,7 +520,7 @@ wait_read_only_owners(pgBackup *backup) if (fp == NULL && errno != ENOENT) elog(ERROR, "Cannot open lock file \"%s\": %s", lock_file, strerror(errno)); - /* iterate over pids in lock file */ + /* iterate over pids in lock file */ while (fp && fgets(buffer, sizeof(buffer), fp)) { encoded_pid = atoi(buffer); @@ -510,47 +530,42 @@ wait_read_only_owners(pgBackup *backup) continue; } - /* wait until RO lock owners go away */ + /* wait until shared lock owners go away */ do { if (interrupted) elog(ERROR, "Interrupted while locking backup %s", base36enc(backup->start_time)); - if (encoded_pid != my_pid) + if (encoded_pid == my_pid) + break; + + /* check if lock owner is still alive */ + if (kill(encoded_pid, 0) == 0) { - if (kill(encoded_pid, 0) == 0) + /* complain from time to time */ + if ((ntries % LOG_FREQ) == 0) { - if ((ntries % LOG_FREQ) == 0) - { - elog(WARNING, "Process %d is using backup %s in read only mode, and is still running", - encoded_pid, base36enc(backup->start_time)); + elog(WARNING, "Process %d is using backup %s in shared mode, and is still running", + encoded_pid, base36enc(backup->start_time)); - elog(WARNING, "Waiting %u seconds on lock for backup %s", ntries, - base36enc(backup->start_time)); - } - - sleep(1); - - /* try again */ - continue; + elog(WARNING, "Waiting %u seconds on lock for backup %s", ntries, + base36enc(backup->start_time)); } - else if (errno != ESRCH) - elog(ERROR, "Failed to send signal 0 to a process %d: %s", - encoded_pid, strerror(errno)); + + sleep(1); + + /* try again */ + continue; } + else if (errno != ESRCH) + elog(ERROR, "Failed to send signal 0 to a process %d: %s", + encoded_pid, strerror(errno)); /* locker is dead */ break; } while (ntries--); - - if (ntries <= 0) - { - elog(WARNING, "Cannot to lock backup %s in exclusive mode, because process %u owns read-only lock", - base36enc(backup->start_time), encoded_pid); - return false; - } } if (fp && ferror(fp)) @@ -559,22 +574,26 @@ wait_read_only_owners(pgBackup *backup) if (fp) fclose(fp); - /* unlink RO lock list */ + /* some shared owners are still alive */ + if (ntries <= 0) + { + elog(WARNING, "Cannot to lock backup %s in exclusive mode, because process %u owns shared lock", + base36enc(backup->start_time), encoded_pid); + return 1; + } + + /* unlink shared lock file */ fio_unlink(lock_file, FIO_BACKUP_HOST); - return true; + return 0; } -bool -lock_backup_internal(pgBackup *backup, bool exclusive) -{ - if (exclusive) - return wait_read_only_owners(backup); - else - return lock_backup_read_only(backup); -} - -bool -lock_backup_read_only(pgBackup *backup) +/* + * Lock backup in shared mode + * 0 - successs + * 1 - fail + */ +int +grab_shared_lock_file(pgBackup *backup) { FILE *fp_in = NULL; FILE *fp_out = NULL; @@ -604,20 +623,20 @@ lock_backup_read_only(pgBackup *backup) continue; } - if (encoded_pid != my_pid) + if (encoded_pid == my_pid) + continue; + + if (kill(encoded_pid, 0) == 0) { - if (kill(encoded_pid, 0) == 0) - { - /* - * Somebody is still using this backup in RO mode, - * copy this pid into a new file. - */ - buffer_len += snprintf(buffer+buffer_len, 4096, "%u\n", encoded_pid); - } - else if (errno != ESRCH) - elog(ERROR, "Failed to send signal 0 to a process %d: %s", - encoded_pid, strerror(errno)); + /* + * Somebody is still using this backup in shared mode, + * copy this pid into a new file. + */ + buffer_len += snprintf(buffer+buffer_len, 4096, "%u\n", encoded_pid); } + else if (errno != ESRCH) + elog(ERROR, "Failed to send signal 0 to a process %d: %s", + encoded_pid, strerror(errno)); } if (fp_in) @@ -647,7 +666,123 @@ lock_backup_read_only(pgBackup *backup) elog(ERROR, "Cannot rename file \"%s\" to \"%s\": %s", lock_file_tmp, lock_file, strerror(errno)); - return true; + return 0; +} + +void +unlock_backup(const char *backup_dir, const char *backup_id, bool exclusive) +{ + if (exclusive) + { + release_excl_lock_file(backup_dir); + return; + } + + /* To remove shared lock, we must briefly obtain exclusive lock, ... */ + if (grab_excl_lock_file(backup_dir, backup_id, false) != 0) + /* ... if it's not possible then leave shared lock */ + return; + + release_shared_lock_file(backup_dir); + release_excl_lock_file(backup_dir); +} + +void +release_excl_lock_file(const char *backup_dir) +{ + char lock_file[MAXPGPATH]; + + join_path_components(lock_file, backup_dir, BACKUP_LOCK_FILE); + + /* TODO Sanity check: maybe we should check, that pid in lock file is my_pid */ + + /* unlink pid file */ + fio_unlink(lock_file, FIO_BACKUP_HOST); +} + +void +release_shared_lock_file(const char *backup_dir) +{ + FILE *fp_in = NULL; + FILE *fp_out = NULL; + char buf_in[256]; + pid_t encoded_pid; + char lock_file[MAXPGPATH]; + + char buffer[8192]; /*TODO: should be enough, but maybe malloc+realloc is better ? */ + char lock_file_tmp[MAXPGPATH]; + int buffer_len = 0; + + join_path_components(lock_file, backup_dir, BACKUP_RO_LOCK_FILE); + snprintf(lock_file_tmp, MAXPGPATH, "%s%s", lock_file, "tmp"); + + /* open lock file */ + fp_in = fopen(lock_file, "r"); + if (fp_in == NULL) + { + if (errno == ENOENT) + return; + else + elog(ERROR, "Cannot open lock file \"%s\": %s", lock_file, strerror(errno)); + } + + /* read PIDs of owners */ + while (fgets(buf_in, sizeof(buf_in), fp_in)) + { + encoded_pid = atoi(buf_in); + + if (encoded_pid <= 0) + { + elog(WARNING, "Bogus data in lock file \"%s\": \"%s\"", lock_file, buf_in); + continue; + } + + /* remove my pid */ + if (encoded_pid == my_pid) + continue; + + if (kill(encoded_pid, 0) == 0) + { + /* + * Somebody is still using this backup in shared mode, + * copy this pid into a new file. + */ + buffer_len += snprintf(buffer+buffer_len, 4096, "%u\n", encoded_pid); + } + else if (errno != ESRCH) + elog(ERROR, "Failed to send signal 0 to a process %d: %s", + encoded_pid, strerror(errno)); + } + + if (ferror(fp_in)) + elog(ERROR, "Cannot read from lock file: \"%s\"", lock_file); + fclose(fp_in); + + /* if there is no active pid left, then there is nothing to do */ + if (buffer_len == 0) + { + fio_unlink(lock_file, FIO_BACKUP_HOST); + return; + } + + fp_out = fopen(lock_file_tmp, "w"); + if (fp_out == NULL) + elog(ERROR, "Cannot open temp lock file \"%s\": %s", lock_file_tmp, strerror(errno)); + + /* write out the collected PIDs to temp lock file */ + fwrite(buffer, 1, buffer_len, fp_out); + + if (ferror(fp_out)) + elog(ERROR, "Cannot write to lock file: \"%s\"", lock_file_tmp); + + if (fclose(fp_out) != 0) + elog(ERROR, "Cannot close temp lock file \"%s\": %s", lock_file_tmp, strerror(errno)); + + if (rename(lock_file_tmp, lock_file) < 0) + elog(ERROR, "Cannot rename file \"%s\" to \"%s\": %s", + lock_file_tmp, lock_file, strerror(errno)); + + return; } /* diff --git a/tests/locking.py b/tests/locking.py index 54000783..ef7aa1f2 100644 --- a/tests/locking.py +++ b/tests/locking.py @@ -581,6 +581,59 @@ class LockingTest(ProbackupTest, unittest.TestCase): # Clean after yourself self.del_test_dir(module_name, fname) -# TODO: -# test that concurrent validation and restore are not locking each other -# check that quick exclusive lock, when taking RO-lock, is really quick + def test_shared_lock(self): + """ + Make sure that shared lock leaves no files with pids + """ + fname = self.id().split('.')[3] + node = self.make_simple_node( + base_dir=os.path.join(module_name, fname, 'node'), + initdb_params=['--data-checksums']) + + backup_dir = os.path.join(self.tmp_path, module_name, fname, 'backup') + self.init_pb(backup_dir) + self.add_instance(backup_dir, 'node', node) + self.set_archiving(backup_dir, 'node', node) + node.slow_start() + + # Fill with data + node.pgbench_init(scale=1) + + # FULL + backup_id = self.backup_node(backup_dir, 'node', node) + + lockfile_excl = os.path.join(backup_dir, 'backups', 'node', backup_id, 'backup.pid') + lockfile_shr = os.path.join(backup_dir, 'backups', 'node', backup_id, 'backup_ro.pid') + + self.validate_pb(backup_dir, 'node', backup_id) + + self.assertFalse( + os.path.exists(lockfile_excl), + "File should not exist: {0}".format(lockfile_excl)) + + self.assertFalse( + os.path.exists(lockfile_shr), + "File should not exist: {0}".format(lockfile_shr)) + + gdb = self.validate_pb(backup_dir, 'node', backup_id, gdb=True) + + gdb.set_breakpoint('validate_one_page') + gdb.run_until_break() + gdb.kill() + + self.assertTrue( + os.path.exists(lockfile_shr), + "File should exist: {0}".format(lockfile_shr)) + + self.validate_pb(backup_dir, 'node', backup_id) + + self.assertFalse( + os.path.exists(lockfile_excl), + "File should not exist: {0}".format(lockfile_excl)) + + self.assertFalse( + os.path.exists(lockfile_shr), + "File should not exist: {0}".format(lockfile_shr)) + + # Clean after yourself + self.del_test_dir(module_name, fname) From c30628cd1da24f85da8f9fc9316bd8115348efd2 Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Wed, 10 Feb 2021 14:11:57 +0300 Subject: [PATCH 08/20] [Issue #237] Ignore EROFS when locking backup in shared mode --- src/catalog.c | 66 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 19 deletions(-) diff --git a/src/catalog.c b/src/catalog.c index e1b5d770..b9d384b6 100644 --- a/src/catalog.c +++ b/src/catalog.c @@ -36,6 +36,11 @@ static void unlock_backup(const char *backup_dir, const char *backup_id, bool ex static void release_excl_lock_file(const char *backup_dir); static void release_shared_lock_file(const char *backup_dir); +#define LOCK_OK 0 +#define LOCK_FAIL_TIMEOUT 1 +#define LOCK_FAIL_ENOSPC 2 +#define LOCK_FAIL_EROFS 3 + typedef struct LockInfo { char backup_id[10]; @@ -187,18 +192,26 @@ lock_backup(pgBackup *backup, bool strict, bool exclusive) rc = grab_excl_lock_file(backup->root_dir, base36enc(backup->start_time), strict); - if (rc == 1) + if (rc == LOCK_FAIL_TIMEOUT) return false; - else if (rc == 2) + else if (rc == LOCK_FAIL_ENOSPC) { - enospc_detected = true; - if (strict) - return false; - /* * If we failed to take exclusive lock due to ENOSPC, * then in lax mode treat such condition as if lock was taken. */ + + enospc_detected = true; + if (strict) + return false; + } + else if (rc == LOCK_FAIL_EROFS) + { + /* + * If we failed to take exclusive lock due to EROFS, + * then in shared mode treat such condition as if lock was taken. + */ + return !exclusive; } /* @@ -242,7 +255,7 @@ lock_backup(pgBackup *backup, bool strict, bool exclusive) * freed some space on filesystem, thanks to unlinking of BACKUP_RO_LOCK_FILE. * If somebody concurrently acquired exclusive lock file first, then we should give up. */ - if (grab_excl_lock_file(backup->root_dir, base36enc(backup->start_time), strict) == 1) + if (grab_excl_lock_file(backup->root_dir, base36enc(backup->start_time), strict) == LOCK_FAIL_TIMEOUT) return false; return true; @@ -271,18 +284,20 @@ lock_backup(pgBackup *backup, bool strict, bool exclusive) return true; } -/* Lock backup in exclusive mode +/* + * Lock backup in exclusive mode * Result codes: - * 0 Success - * 1 Failed to acquire lock in lock_timeout time - * 2 Failed to acquire lock due to ENOSPC + * LOCK_OK Success + * LOCK_FAIL_TIMEOUT Failed to acquire lock in lock_timeout time + * LOCK_FAIL_ENOSPC Failed to acquire lock due to ENOSPC + * LOCK_FAIL_EROFS Failed to acquire lock due to EROFS */ int grab_excl_lock_file(const char *root_dir, const char *backup_id, bool strict) { char lock_file[MAXPGPATH]; int fd = 0; - char buffer[MAXPGPATH * 2 + 256]; + char buffer[256]; int ntries = LOCK_TIMEOUT; int empty_tries = LOCK_STALE_TIMEOUT; int len; @@ -312,6 +327,14 @@ grab_excl_lock_file(const char *root_dir, const char *backup_id, bool strict) if (fd >= 0) break; /* Success; exit the retry loop */ + /* read-only fs is a special case */ + if (errno == EROFS) + { + elog(WARNING, "Could not create lock file \"%s\": %s", + lock_file, strerror(errno)); + return LOCK_FAIL_EROFS; + } + /* * Couldn't create the pid file. Probably it already exists. * If file already exists or we have some permission problem (???), @@ -390,7 +413,7 @@ grab_excl_lock_file(const char *root_dir, const char *backup_id, bool strict) * exist. */ if (encoded_pid == my_pid) - return 0; + return LOCK_OK; if (kill(encoded_pid, 0) == 0) { @@ -437,7 +460,7 @@ grab_lock: /* Failed to acquire exclusive lock in time */ if (fd <= 0) - return 1; + return LOCK_FAIL_TIMEOUT; /* * Successfully created the file, now fill it. @@ -457,7 +480,7 @@ grab_lock: * Only delete command should be run in lax mode. */ if (!strict && save_errno == ENOSPC) - return 2; + return LOCK_FAIL_ENOSPC; else elog(ERROR, "Could not write lock file \"%s\": %s", lock_file, strerror(save_errno)); @@ -475,7 +498,7 @@ grab_lock: * Only delete command should be run in lax mode. */ if (!strict && save_errno == ENOSPC) - return 2; + return LOCK_FAIL_ENOSPC; else elog(ERROR, "Could not flush lock file \"%s\": %s", lock_file, strerror(save_errno)); @@ -488,7 +511,7 @@ grab_lock: fio_unlink(lock_file, FIO_BACKUP_HOST); if (!strict && errno == ENOSPC) - return 2; + return LOCK_FAIL_ENOSPC; else elog(ERROR, "Could not close lock file \"%s\": %s", lock_file, strerror(save_errno)); @@ -498,7 +521,7 @@ grab_lock: // base36enc(backup->start_time), // LOCK_TIMEOUT - ntries + LOCK_STALE_TIMEOUT - empty_tries); - return 0; + return LOCK_OK; } /* Wait until all shared lock owners are gone @@ -648,7 +671,12 @@ grab_shared_lock_file(pgBackup *backup) fp_out = fopen(lock_file_tmp, "w"); if (fp_out == NULL) + { + if (errno == EROFS) + return 0; + elog(ERROR, "Cannot open temp lock file \"%s\": %s", lock_file_tmp, strerror(errno)); + } /* add my own pid */ buffer_len += snprintf(buffer+buffer_len, sizeof(buffer), "%u\n", my_pid); @@ -679,7 +707,7 @@ unlock_backup(const char *backup_dir, const char *backup_id, bool exclusive) } /* To remove shared lock, we must briefly obtain exclusive lock, ... */ - if (grab_excl_lock_file(backup_dir, backup_id, false) != 0) + if (grab_excl_lock_file(backup_dir, backup_id, false) != LOCK_OK) /* ... if it's not possible then leave shared lock */ return; From 4a4af6270fc3a06195e604064cccc359080526a4 Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Wed, 10 Feb 2021 14:25:13 +0300 Subject: [PATCH 09/20] bugfix: for PG>=12 instance with missing postgresql.auto.conf restore now correctly recreates postgresql.auto.conf --- src/restore.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/restore.c b/src/restore.c index b3b9965e..adc20bb6 100644 --- a/src/restore.c +++ b/src/restore.c @@ -1543,6 +1543,7 @@ update_recovery_options(pgBackup *backup, if (errno != ENOENT) elog(ERROR, "cannot stat file \"%s\": %s", postgres_auto_path, strerror(errno)); + st.st_size = 0; } /* Kludge for 0-sized postgresql.auto.conf file. TODO: make something more intelligent */ From 63d79e2b4fac043e29ea4d54589fbb8788a18ffd Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Wed, 10 Feb 2021 15:10:15 +0300 Subject: [PATCH 10/20] [Issue #323] stable remote agent API --- src/pg_probackup.h | 3 +++ src/utils/remote.c | 5 +++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/pg_probackup.h b/src/pg_probackup.h index a766e705..c15e6efd 100644 --- a/src/pg_probackup.h +++ b/src/pg_probackup.h @@ -307,7 +307,10 @@ typedef enum ShowFormat #define FILE_NOT_FOUND (-2) /* file disappeared during backup */ #define BLOCKNUM_INVALID (-1) #define PROGRAM_VERSION "2.4.9" + +/* update when remote agent API or behaviour changes */ #define AGENT_PROTOCOL_VERSION 20409 +#define AGENT_PROTOCOL_VERSION_STR "2.4.9" /* update only when changing storage format */ #define STORAGE_FORMAT_VERSION "2.4.4" diff --git a/src/utils/remote.c b/src/utils/remote.c index 88b1ce7a..2bfd24d1 100644 --- a/src/utils/remote.c +++ b/src/utils/remote.c @@ -247,8 +247,9 @@ bool launch_agent(void) (agent_version / 100) % 100, agent_version % 100); - elog(ERROR, "Remote agent version %s does not match local program version %s", - agent_version_str, PROGRAM_VERSION); + elog(ERROR, "Remote agent protocol version %s does not match local program protocol version %s, " + "consider to upgrade pg_probackup binary", + agent_version_str, AGENT_PROTOCOL_VERSION_STR); } return true; From 6dcf64a90705411e2764800ff8dd2ccb5fc1a4d3 Mon Sep 17 00:00:00 2001 From: anastasia Date: Wed, 10 Feb 2021 16:22:40 +0300 Subject: [PATCH 11/20] Tests. Fix cfs_backup tests compatibility with ptrack 2.1 --- tests/cfs_backup.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/cfs_backup.py b/tests/cfs_backup.py index 5a366551..3d2fa8de 100644 --- a/tests/cfs_backup.py +++ b/tests/cfs_backup.py @@ -20,9 +20,9 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): self.node = self.make_simple_node( base_dir="{0}/{1}/node".format(module_name, self.fname), set_replication=True, + ptrack_enable=True, initdb_params=['--data-checksums'], pg_options={ - 'ptrack_enable': 'on', 'cfs_encryption': 'off', 'max_wal_senders': '2', 'shared_buffers': '200MB' @@ -35,6 +35,11 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): self.node.slow_start() + if self.node.major_version >= 12: + self.node.safe_psql( + "postgres", + "CREATE EXTENSION ptrack") + self.create_tblspace_in_node(self.node, tblspace_name, cfs=True) tblspace = self.node.safe_psql( From 2305d1fdda20fc16af053a06805416de0d2e69de Mon Sep 17 00:00:00 2001 From: anastasia Date: Wed, 10 Feb 2021 19:44:38 +0300 Subject: [PATCH 12/20] Tests. Expect failure in test_validate_target_lsn, because it requires multi-timeline WAL validation, which is not implemented yet --- tests/validate.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/validate.py b/tests/validate.py index 6777b92a..cfec234d 100644 --- a/tests/validate.py +++ b/tests/validate.py @@ -3451,7 +3451,8 @@ class ValidateTest(ProbackupTest, unittest.TestCase): # Clean after yourself self.del_test_dir(module_name, fname) - # @unittest.expectedFailure + #TODO fix the test + @unittest.expectedFailure # @unittest.skip("skip") def test_validate_target_lsn(self): """ From bdcc28d36cd227635d8a6dba8e2498d91069d27b Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Thu, 11 Feb 2021 09:51:38 +0300 Subject: [PATCH 13/20] tests: minor fixes for backup and merge modules --- tests/backup.py | 1 + tests/merge.py | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/backup.py b/tests/backup.py index 2ee5b9ac..70c4dc13 100644 --- a/tests/backup.py +++ b/tests/backup.py @@ -5,6 +5,7 @@ from .helpers.ptrack_helpers import ProbackupTest, ProbackupException import shutil from distutils.dir_util import copy_tree from testgres import ProcessType +import subprocess module_name = 'backup' diff --git a/tests/merge.py b/tests/merge.py index bf21405b..29d60433 100644 --- a/tests/merge.py +++ b/tests/merge.py @@ -7,6 +7,7 @@ from testgres import QueryException import shutil from datetime import datetime, timedelta import time +import subprocess module_name = "merge" From 5207900fb843572398513998b6ec158d74473d23 Mon Sep 17 00:00:00 2001 From: anastasia Date: Thu, 11 Feb 2021 12:01:13 +0300 Subject: [PATCH 14/20] Update tests/Readme.md. Add note about python3. --- tests/Readme.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/Readme.md b/tests/Readme.md index 3adf0c01..adcf5380 100644 --- a/tests/Readme.md +++ b/tests/Readme.md @@ -1,7 +1,11 @@ -[см wiki](https://confluence.postgrespro.ru/display/DEV/pg_probackup) +[see wiki](https://confluence.postgrespro.ru/display/DEV/pg_probackup) ``` -Note: For now these are works on Linux and "kinda" works on Windows +Note: For now these tests work on Linux and "kinda" work on Windows +``` + +``` +Note: tests require python3 to work properly. ``` ``` From 71661abc3f4930d5d960f85dda0deae71a31fc63 Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Thu, 11 Feb 2021 19:28:42 +0300 Subject: [PATCH 15/20] tests: fix "cfs_backup" module --- tests/cfs_backup.py | 76 ++++++++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/tests/cfs_backup.py b/tests/cfs_backup.py index 3d2fa8de..b7ebff0f 100644 --- a/tests/cfs_backup.py +++ b/tests/cfs_backup.py @@ -45,13 +45,17 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): tblspace = self.node.safe_psql( "postgres", "SELECT * FROM pg_tablespace WHERE spcname='{0}'".format( - tblspace_name) - ) - self.assertTrue( - tblspace_name in tblspace and "compression=true" in tblspace, + tblspace_name)) + + self.assertIn( + tblspace_name, str(tblspace), "ERROR: The tablespace not created " - "or it create without compressions" - ) + "or it create without compressions") + + self.assertIn( + "compression=true", str(tblspace), + "ERROR: The tablespace not created " + "or it create without compressions") self.assertTrue( find_by_name( @@ -743,12 +747,14 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): # CHECK FULL BACKUP self.node.stop() self.node.cleanup() - shutil.rmtree( - self.get_tblspace_path(self.node, tblspace_name), - ignore_errors=True) + shutil.rmtree(self.get_tblspace_path(self.node, tblspace_name)) self.restore_node( - self.backup_dir, 'node', self.node, - backup_id=backup_id_full, options=["-j", "4"]) + self.backup_dir, 'node', self.node, backup_id=backup_id_full, + options=[ + "-j", "4", + "--recovery-target=immediate", + "--recovery-target-action=promote"]) + self.node.slow_start() self.assertEqual( full_result, @@ -762,8 +768,12 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): self.get_tblspace_path(self.node, tblspace_name), ignore_errors=True) self.restore_node( - self.backup_dir, 'node', self.node, - backup_id=backup_id_page, options=["-j", "4"]) + self.backup_dir, 'node', self.node, backup_id=backup_id_page, + options=[ + "-j", "4", + "--recovery-target=immediate", + "--recovery-target-action=promote"]) + self.node.slow_start() self.assertEqual( page_result, @@ -791,8 +801,7 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): "AS SELECT i AS id, MD5(i::text) AS text, " "MD5(repeat(i::text,10))::tsvector AS tsvector " "FROM generate_series(0,1005000) i".format( - 't_heap_1', tblspace_name_1) - ) + 't_heap_1', tblspace_name_1)) self.node.safe_psql( "postgres", @@ -800,8 +809,7 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): "AS SELECT i AS id, MD5(i::text) AS text, " "MD5(repeat(i::text,10))::tsvector AS tsvector " "FROM generate_series(0,1005000) i".format( - 't_heap_2', tblspace_name_2) - ) + 't_heap_2', tblspace_name_2)) full_result_1 = self.node.safe_psql( "postgres", "SELECT * FROM t_heap_1") @@ -869,21 +877,16 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): # CHECK FULL BACKUP self.node.stop() - self.node.cleanup() - shutil.rmtree( - self.get_tblspace_path(self.node, tblspace_name), - ignore_errors=True) - shutil.rmtree( - self.get_tblspace_path(self.node, tblspace_name_1), - ignore_errors=True) - shutil.rmtree( - self.get_tblspace_path(self.node, tblspace_name_2), - ignore_errors=True) self.restore_node( self.backup_dir, 'node', self.node, - backup_id=backup_id_full, options=["-j", "4"]) + backup_id=backup_id_full, + options=[ + "-j", "4", "--incremental-mode=checksum", + "--recovery-target=immediate", + "--recovery-target-action=promote"]) self.node.slow_start() + self.assertEqual( full_result_1, self.node.safe_psql("postgres", "SELECT * FROM t_heap_1"), @@ -895,21 +898,16 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): # CHECK PAGE BACKUP self.node.stop() - self.node.cleanup() - shutil.rmtree( - self.get_tblspace_path(self.node, tblspace_name), - ignore_errors=True) - shutil.rmtree( - self.get_tblspace_path(self.node, tblspace_name_1), - ignore_errors=True) - shutil.rmtree( - self.get_tblspace_path(self.node, tblspace_name_2), - ignore_errors=True) self.restore_node( self.backup_dir, 'node', self.node, - backup_id=backup_id_page, options=["-j", "4"]) + backup_id=backup_id_page, + options=[ + "-j", "4", "--incremental-mode=checksum", + "--recovery-target=immediate", + "--recovery-target-action=promote"]) self.node.slow_start() + self.assertEqual( page_result_1, self.node.safe_psql("postgres", "SELECT * FROM t_heap_1"), From f324b081809b2f27c28f66b1cf2f878f55131c25 Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Thu, 11 Feb 2021 20:27:47 +0300 Subject: [PATCH 16/20] tests: more fixes for "cfs_backup" module --- tests/cfs_backup.py | 8 ++++---- tests/helpers/cfs_helpers.py | 4 +++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/cfs_backup.py b/tests/cfs_backup.py index b7ebff0f..2e686d46 100644 --- a/tests/cfs_backup.py +++ b/tests/cfs_backup.py @@ -482,7 +482,7 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): ) # --- Section: Incremental from fill tablespace --- # - @unittest.expectedFailure + # @unittest.expectedFailure # @unittest.skip("skip") @unittest.skipUnless(ProbackupTest.enterprise, 'skip') def test_fullbackup_after_create_table_ptrack_after_create_table(self): @@ -546,7 +546,7 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): ) ) - @unittest.expectedFailure + # @unittest.expectedFailure # @unittest.skip("skip") @unittest.skipUnless(ProbackupTest.enterprise, 'skip') def test_fullbackup_after_create_table_ptrack_after_create_table_stream(self): @@ -612,7 +612,7 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): ) ) - @unittest.expectedFailure + # @unittest.expectedFailure # @unittest.skip("skip") @unittest.skipUnless(ProbackupTest.enterprise, 'skip') def test_fullbackup_after_create_table_page_after_create_table(self): @@ -917,7 +917,7 @@ class CfsBackupNoEncTest(ProbackupTest, unittest.TestCase): self.node.safe_psql("postgres", "SELECT * FROM t_heap_2"), 'Lost data after restore') - @unittest.expectedFailure + # @unittest.expectedFailure # @unittest.skip("skip") @unittest.skipUnless(ProbackupTest.enterprise, 'skip') def test_fullbackup_after_create_table_page_after_create_table_stream(self): diff --git a/tests/helpers/cfs_helpers.py b/tests/helpers/cfs_helpers.py index 67e2b331..31af76f2 100644 --- a/tests/helpers/cfs_helpers.py +++ b/tests/helpers/cfs_helpers.py @@ -88,4 +88,6 @@ def corrupt_file(filename): def random_string(n): a = string.ascii_letters + string.digits - return ''.join([random.choice(a) for i in range(int(n)+1)]) \ No newline at end of file + random_str = ''.join([random.choice(a) for i in range(int(n)+1)]) + return str.encode(random_str) +# return ''.join([random.choice(a) for i in range(int(n)+1)]) From 31a8ea3ff04c226a12ed93db3aa82b4fd959367b Mon Sep 17 00:00:00 2001 From: anastasia Date: Fri, 12 Feb 2021 13:08:12 +0300 Subject: [PATCH 17/20] tests: Skip test_incr_lsn_long_xact_1, when testing with PostgresPro Enterprise --- tests/incr_restore.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/incr_restore.py b/tests/incr_restore.py index 1d3a52df..a228bdd7 100644 --- a/tests/incr_restore.py +++ b/tests/incr_restore.py @@ -1711,6 +1711,9 @@ class IncrRestoreTest(ProbackupTest, unittest.TestCase): # @unittest.skip("skip") # @unittest.expectedFailure + # This test will pass with Enterprise + # because it has checksums enabled by default + @unittest.skipIf(ProbackupTest.enterprise, 'skip') def test_incr_lsn_long_xact_1(self): """ """ From 8691e08e220804248dd1c241279a98fb7f050d50 Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Fri, 12 Feb 2021 15:32:51 +0300 Subject: [PATCH 18/20] tests: some minor fixes for "page" module --- tests/page.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/tests/page.py b/tests/page.py index 323c0a6d..c0dbb69f 100644 --- a/tests/page.py +++ b/tests/page.py @@ -757,8 +757,6 @@ class PageTest(ProbackupTest, unittest.TestCase): self.output, self.cmd)) except ProbackupException as e: self.assertTrue( - 'INFO: Wait for WAL segment' in e.message and - 'to be archived' in e.message and 'Could not read WAL record at' in e.message and 'is absent' in e.message, '\n Unexpected Error Message: {0}\n CMD: {1}'.format( @@ -782,8 +780,6 @@ class PageTest(ProbackupTest, unittest.TestCase): self.output, self.cmd)) except ProbackupException as e: self.assertTrue( - 'INFO: Wait for WAL segment' in e.message and - 'to be archived' in e.message and 'Could not read WAL record at' in e.message and 'is absent' in e.message, '\n Unexpected Error Message: {0}\n CMD: {1}'.format( @@ -872,8 +868,6 @@ class PageTest(ProbackupTest, unittest.TestCase): self.output, self.cmd)) except ProbackupException as e: self.assertTrue( - 'INFO: Wait for WAL segment' in e.message and - 'to be archived' in e.message and 'Could not read WAL record at' in e.message and 'Possible WAL corruption. Error has occured during reading WAL segment' in e.message, '\n Unexpected Error Message: {0}\n CMD: {1}'.format( @@ -896,8 +890,6 @@ class PageTest(ProbackupTest, unittest.TestCase): self.output, self.cmd)) except ProbackupException as e: self.assertTrue( - 'INFO: Wait for WAL segment' in e.message and - 'to be archived' in e.message and 'Could not read WAL record at' in e.message and 'Possible WAL corruption. Error has occured during reading WAL segment "{0}"'.format( file) in e.message, @@ -997,8 +989,6 @@ class PageTest(ProbackupTest, unittest.TestCase): self.output, self.cmd)) except ProbackupException as e: self.assertTrue( - 'INFO: Wait for WAL segment' in e.message and - 'to be archived' in e.message and 'Could not read WAL record at' in e.message and 'Possible WAL corruption. Error has occured during reading WAL segment' in e.message, '\n Unexpected Error Message: {0}\n CMD: {1}'.format( @@ -1020,8 +1010,6 @@ class PageTest(ProbackupTest, unittest.TestCase): "Output: {0} \n CMD: {1}".format( self.output, self.cmd)) except ProbackupException as e: - self.assertIn('INFO: Wait for WAL segment', e.message) - self.assertIn('to be archived', e.message) self.assertIn('Could not read WAL record at', e.message) self.assertIn('WAL file is from different database system: ' 'WAL file database system identifier is', e.message) From 212fad4f931e1269d0eb290e33c5a57435ff9582 Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Fri, 12 Feb 2021 16:48:51 +0300 Subject: [PATCH 19/20] Version 2.4.10 --- src/pg_probackup.h | 2 +- tests/expected/option_version.out | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pg_probackup.h b/src/pg_probackup.h index c15e6efd..1e001bd0 100644 --- a/src/pg_probackup.h +++ b/src/pg_probackup.h @@ -306,7 +306,7 @@ typedef enum ShowFormat #define BYTES_INVALID (-1) /* file didn`t changed since previous backup, DELTA backup do not rely on it */ #define FILE_NOT_FOUND (-2) /* file disappeared during backup */ #define BLOCKNUM_INVALID (-1) -#define PROGRAM_VERSION "2.4.9" +#define PROGRAM_VERSION "2.4.10" /* update when remote agent API or behaviour changes */ #define AGENT_PROTOCOL_VERSION 20409 diff --git a/tests/expected/option_version.out b/tests/expected/option_version.out index 1481e32d..b5204e46 100644 --- a/tests/expected/option_version.out +++ b/tests/expected/option_version.out @@ -1 +1 @@ -pg_probackup 2.4.9 \ No newline at end of file +pg_probackup 2.4.10 \ No newline at end of file From 8e930b79abc2f2a0440eb5e1528fe562f2a971c9 Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Sat, 13 Feb 2021 04:03:04 +0300 Subject: [PATCH 20/20] make compiler happy --- src/catalog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/catalog.c b/src/catalog.c index b9d384b6..423e83f7 100644 --- a/src/catalog.c +++ b/src/catalog.c @@ -533,7 +533,7 @@ wait_shared_owners(pgBackup *backup) { FILE *fp = NULL; char buffer[256]; - pid_t encoded_pid; + pid_t encoded_pid = 0; int ntries = LOCK_TIMEOUT; char lock_file[MAXPGPATH];