From 2f8617dff11f4d65d43ff3687d3899610ade44c2 Mon Sep 17 00:00:00 2001 From: "Mikhail A. Kulagin" Date: Tue, 18 May 2021 13:15:38 +0300 Subject: [PATCH 01/11] cosmetic changes --- src/backup.c | 36 ++++++++++++++++++++---------------- src/pg_probackup.h | 5 +++-- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/backup.c b/src/backup.c index 46e3ba482..fa065998f 100644 --- a/src/backup.c +++ b/src/backup.c @@ -38,19 +38,23 @@ bool exclusive_backup = false; /* Is pg_start_backup() was executed */ static bool backup_in_progress = false; -struct pg_stop_backup_result { +typedef struct PGStopBackupResult +{ /* * We will use values of snapshot_xid and invocation_time if there are * no transactions between start_lsn and stop_lsn. */ TransactionId snapshot_xid; time_t invocation_time; + /* + * Fields that store pg_catalog.pg_stop_backup() result + */ XLogRecPtr lsn; size_t backup_label_content_len; char *backup_label_content; size_t tablespace_map_content_len; char *tablespace_map_content; -}; +} PGStopBackupResult; /* * Backup routines @@ -90,12 +94,12 @@ static void check_server_version(PGconn *conn, PGNodeInfo *nodeInfo); static void confirm_block_size(PGconn *conn, const char *name, int blcksz); static void set_cfs_datafiles(parray *files, const char *root, char *relative, size_t i); -static StopBackupCallbackState stop_callback_state; +static StopBackupCallbackParams stop_callback_params; static void backup_stopbackup_callback(bool fatal, void *userdata) { - StopBackupCallbackState *st = (StopBackupCallbackState *) userdata; + StopBackupCallbackParams *st = (StopBackupCallbackParams *) userdata; /* * If backup is in progress, notify stop of backup to PostgreSQL */ @@ -214,11 +218,11 @@ do_backup_pg(InstanceState *instanceState, PGconn *backup_conn, if (prev_backup) { - if (parse_program_version(prev_backup->program_version) > parse_program_version(PROGRAM_VERSION)) - elog(ERROR, "pg_probackup binary version is %s, but backup %s version is %s. " - "pg_probackup do not guarantee to be forward compatible. " - "Please upgrade pg_probackup binary.", - PROGRAM_VERSION, base36enc(prev_backup->start_time), prev_backup->program_version); + if (parse_program_version(prev_backup->program_version) > parse_program_version(PROGRAM_VERSION)) + elog(ERROR, "pg_probackup binary version is %s, but backup %s version is %s. " + "pg_probackup do not guarantee to be forward compatible. " + "Please upgrade pg_probackup binary.", + PROGRAM_VERSION, base36enc(prev_backup->start_time), prev_backup->program_version); elog(INFO, "Parent backup: %s", base36enc(prev_backup->start_time)); @@ -1088,9 +1092,9 @@ pg_start_backup(InstanceState *instanceState, const char *label, bool smooth, pg * is necessary to call pg_stop_backup() in backup_cleanup(). */ backup_in_progress = true; - stop_callback_state.conn = conn; - stop_callback_state.server_version = nodeInfo->server_version; - pgut_atexit_push(backup_stopbackup_callback, &stop_callback_state); + stop_callback_params.conn = conn; + stop_callback_params.server_version = nodeInfo->server_version; + pgut_atexit_push(backup_stopbackup_callback, &stop_callback_params); /* Extract timeline and LSN from results of pg_start_backup() */ XLogDataFromLSN(PQgetvalue(res, 0, 0), &lsn_hi, &lsn_lo); @@ -1573,7 +1577,7 @@ pg_stop_backup_send(PGconn *conn, int server_version, bool is_started_on_replica elog(ERROR, "Failed to send pg_stop_backup query"); /* After we have sent pg_stop_backup, we don't need this callback anymore */ - pgut_atexit_pop(backup_stopbackup_callback, &stop_callback_state); + pgut_atexit_pop(backup_stopbackup_callback, &stop_callback_params); if (query_text) *query_text = pgut_strdup(stop_backup_query); @@ -1589,7 +1593,7 @@ pg_stop_backup_send(PGconn *conn, int server_version, bool is_started_on_replica static void pg_stop_backup_consume(PGconn *conn, int server_version, bool is_exclusive, uint32 timeout, const char *query_text, - struct pg_stop_backup_result *result) + PGStopBackupResult *result) { PGresult *query_result; uint32 pg_stop_backup_timeout = 0; @@ -1738,7 +1742,7 @@ pg_stop_backup_write_file_helper(const char *path, const char *filename, const c error_msg_filename, full_filename, strerror(errno)); /* - * It's vital to check if backup_files_list is initialized, + * It's vital to check if files_list is initialized, * because we could get here because the backup was interrupted */ if (file_list) @@ -1766,7 +1770,7 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb { PGconn *conn; bool stop_lsn_exists = false; - struct pg_stop_backup_result stop_backup_result; + PGStopBackupResult stop_backup_result; char *xlog_path,stream_xlog_path[MAXPGPATH]; /* kludge against some old bug in archive_timeout. TODO: remove in 3.0.0 */ int timeout = (instance_config.archive_timeout > 0) ? diff --git a/src/pg_probackup.h b/src/pg_probackup.h index d02bbb033..4da12d654 100644 --- a/src/pg_probackup.h +++ b/src/pg_probackup.h @@ -679,10 +679,11 @@ typedef struct BackupPageHeader2 uint16 checksum; } BackupPageHeader2; -typedef struct StopBackupCallbackState { +typedef struct StopBackupCallbackParams +{ PGconn *conn; int server_version; -} StopBackupCallbackState; +} StopBackupCallbackParams; /* Special value for compressed_size field */ #define PageIsOk 0 From 61f167c094aa3898eafa5776307165bae5f364ee Mon Sep 17 00:00:00 2001 From: "Mikhail A. Kulagin" Date: Tue, 18 May 2021 14:42:08 +0300 Subject: [PATCH 02/11] rename pg_checksum_enable() to pg_is_checksum_enabled --- src/backup.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/backup.c b/src/backup.c index fa065998f..50aa80939 100644 --- a/src/backup.c +++ b/src/backup.c @@ -87,7 +87,7 @@ static parray *get_database_map(PGconn *pg_startbackup_conn); static bool pgpro_support(PGconn *conn); /* Check functions */ -static bool pg_checksum_enable(PGconn *conn); +static bool pg_is_checksum_enabled(PGconn *conn); static bool pg_is_in_recovery(PGconn *conn); static bool pg_is_superuser(PGconn *conn); static void check_server_version(PGconn *conn, PGNodeInfo *nodeInfo); @@ -732,7 +732,7 @@ pgdata_basic_setup(ConnectionOptions conn_opt, PGNodeInfo *nodeInfo) /* Confirm that this server version is supported */ check_server_version(cur_conn, nodeInfo); - if (pg_checksum_enable(cur_conn)) + if (pg_is_checksum_enabled(cur_conn)) current.checksum_version = 1; else current.checksum_version = 0; @@ -1222,7 +1222,7 @@ get_database_map(PGconn *conn) /* Check if ptrack is enabled in target instance */ static bool -pg_checksum_enable(PGconn *conn) +pg_is_checksum_enabled(PGconn *conn) { PGresult *res_db; From 30543b22ef80132b0ec72f84103c33642ffb7fb2 Mon Sep 17 00:00:00 2001 From: "Mikhail A. Kulagin" Date: Mon, 24 May 2021 07:30:36 +0300 Subject: [PATCH 03/11] remove unused instanceState from pg_start_backup() --- src/backup.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/backup.c b/src/backup.c index 50aa80939..b55406507 100644 --- a/src/backup.c +++ b/src/backup.c @@ -66,7 +66,7 @@ static void *backup_files(void *arg); static void do_backup_pg(InstanceState *instanceState, PGconn *backup_conn, PGNodeInfo *nodeInfo, bool no_sync, bool backup_logs); -static void pg_start_backup(InstanceState *instanceState, const char *label, bool smooth, pgBackup *backup, +static void pg_start_backup(const char *label, bool smooth, pgBackup *backup, PGNodeInfo *nodeInfo, PGconn *conn); static void pg_switch_wal(PGconn *conn); static void pg_silent_client_messages(PGconn *conn); @@ -162,7 +162,7 @@ do_backup_pg(InstanceState *instanceState, PGconn *backup_conn, strlen(" with pg_probackup")); /* Call pg_start_backup function in PostgreSQL connect */ - pg_start_backup(instanceState, label, smooth_checkpoint, ¤t, nodeInfo, backup_conn); + pg_start_backup(label, smooth_checkpoint, ¤t, nodeInfo, backup_conn); /* Obtain current timeline */ #if PG_VERSION_NUM >= 90600 @@ -1063,7 +1063,7 @@ confirm_block_size(PGconn *conn, const char *name, int blcksz) * Notify start of backup to PostgreSQL server. */ static void -pg_start_backup(InstanceState *instanceState, const char *label, bool smooth, pgBackup *backup, +pg_start_backup(const char *label, bool smooth, pgBackup *backup, PGNodeInfo *nodeInfo, PGconn *conn) { PGresult *res; From 1657fee7b929375db1a95d52f0fb486ee5620ab4 Mon Sep 17 00:00:00 2001 From: "Mikhail A. Kulagin" Date: Mon, 24 May 2021 09:07:05 +0300 Subject: [PATCH 04/11] Refactor wait_wal_lsn(): remove unused pgBackup * parameter and replace InstanceState * with simple directory string --- src/backup.c | 76 +++++++++++++++++++--------------------------------- 1 file changed, 27 insertions(+), 49 deletions(-) diff --git a/src/backup.c b/src/backup.c index b55406507..317688f99 100644 --- a/src/backup.c +++ b/src/backup.c @@ -75,9 +75,9 @@ static void pg_create_restore_point(PGconn *conn, time_t backup_start_time); static void pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startbackup_conn, PGNodeInfo *nodeInfo); static void pg_stop_backup_send(PGconn *conn, int server_version, bool is_started_on_replica, bool is_exclusive, char **query_text); -static XLogRecPtr wait_wal_lsn(InstanceState *instanceState, XLogRecPtr lsn, bool is_start_lsn, TimeLineID tli, +static XLogRecPtr wait_wal_lsn(const char *wal_segment_dir, XLogRecPtr lsn, bool is_start_lsn, TimeLineID tli, bool in_prev_segment, bool segment_only, - int timeout_elevel, bool in_stream_dir, pgBackup *backup); + int timeout_elevel, bool in_stream_dir); static void check_external_for_tablespaces(parray *external_list, PGconn *backup_conn); @@ -298,7 +298,7 @@ do_backup_pg(InstanceState *instanceState, PGconn *backup_conn, * Because WAL streaming will start after pg_start_backup() in stream * mode. */ - wait_wal_lsn(instanceState, current.start_lsn, true, current.tli, false, true, ERROR, false, ¤t); + wait_wal_lsn(instanceState->instance_wal_subdir_path, current.start_lsn, true, current.tli, false, true, ERROR, false); } /* start stream replication */ @@ -314,7 +314,7 @@ do_backup_pg(InstanceState *instanceState, PGconn *backup_conn, * PAGE backup in stream mode is waited twice, first for * segment in WAL archive and then for streamed segment */ - wait_wal_lsn(instanceState, current.start_lsn, true, current.tli, false, true, ERROR, true, ¤t); + wait_wal_lsn(dst_backup_path, current.start_lsn, true, current.tli, false, true, ERROR, true); } /* initialize backup's file list */ @@ -1298,14 +1298,12 @@ pg_is_superuser(PGconn *conn) * Returns InvalidXLogRecPtr if 'segment_only' flag is used. */ static XLogRecPtr -wait_wal_lsn(InstanceState *instanceState, XLogRecPtr target_lsn, bool is_start_lsn, TimeLineID tli, +wait_wal_lsn(const char *wal_segment_dir, XLogRecPtr target_lsn, bool is_start_lsn, TimeLineID tli, bool in_prev_segment, bool segment_only, - int timeout_elevel, bool in_stream_dir, pgBackup *backup) + int timeout_elevel, bool in_stream_dir) { XLogSegNo targetSegNo; - char pg_wal_dir[MAXPGPATH]; char wal_segment_path[MAXPGPATH], - *wal_segment_dir, wal_segment[MAXFNAMELEN]; bool file_exists = false; uint32 try_count = 0, @@ -1323,6 +1321,7 @@ wait_wal_lsn(InstanceState *instanceState, XLogRecPtr target_lsn, bool is_start_ GetXLogFileName(wal_segment, tli, targetSegNo, instance_config.xlog_seg_size); + join_path_components(wal_segment_path, wal_segment_dir, wal_segment); /* * In pg_start_backup we wait for 'target_lsn' in 'pg_wal' directory if it is * stream and non-page backup. Page backup needs archived WAL files, so we @@ -1330,17 +1329,6 @@ wait_wal_lsn(InstanceState *instanceState, XLogRecPtr target_lsn, bool is_start_ * * In pg_stop_backup it depends only on stream_wal. */ - if (in_stream_dir) - { - join_path_components(pg_wal_dir, backup->database_dir, PG_XLOG_DIR); - join_path_components(wal_segment_path, pg_wal_dir, wal_segment); - wal_segment_dir = pg_wal_dir; - } - else - { - join_path_components(wal_segment_path, instanceState->instance_wal_subdir_path, wal_segment); - wal_segment_dir = instanceState->instance_wal_subdir_path; - } /* TODO: remove this in 3.0 (it is a cludge against some old bug with archive_timeout) */ if (instance_config.archive_timeout > 0) @@ -1771,7 +1759,7 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb PGconn *conn; bool stop_lsn_exists = false; PGStopBackupResult stop_backup_result; - char *xlog_path,stream_xlog_path[MAXPGPATH]; + char *xlog_path, stream_xlog_path[MAXPGPATH]; /* kludge against some old bug in archive_timeout. TODO: remove in 3.0.0 */ int timeout = (instance_config.archive_timeout > 0) ? instance_config.archive_timeout : ARCHIVE_TIMEOUT_DEFAULT; @@ -1803,13 +1791,22 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb */ pg_stop_backup_consume(conn, nodeInfo->server_version, exclusive_backup, timeout, query_text, &stop_backup_result); + if (stream_wal) + { + snprintf(stream_xlog_path, lengthof(stream_xlog_path), + "%s/%s/%s/%s", instanceState->instance_backup_subdir_path, + base36enc(backup->start_time), + DATABASE_DIR, PG_XLOG_DIR); + xlog_path = stream_xlog_path; + } + else + xlog_path = instanceState->instance_wal_subdir_path; + /* It is ok for replica to return invalid STOP LSN * UPD: Apparently it is ok even for a master. */ if (!XRecOffIsValid(stop_backup_result.lsn)) { - char *xlog_path, - stream_xlog_path[MAXPGPATH]; XLogSegNo segno = 0; XLogRecPtr lsn_tmp = InvalidXLogRecPtr; @@ -1828,17 +1825,6 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb //elog(WARNING, "New Invalid stop_backup_lsn value %X/%X", // (uint32) (stop_backup_result.lsn >> 32), (uint32) (stop_backup_result.lsn)); - if (stream_wal) - { - snprintf(stream_xlog_path, lengthof(stream_xlog_path), - "%s/%s/%s/%s", instanceState->instance_backup_subdir_path, - base36enc(backup->start_time), - DATABASE_DIR, PG_XLOG_DIR); - xlog_path = stream_xlog_path; - } - else - xlog_path = instanceState->instance_wal_subdir_path; - GetXLogSegNo(stop_backup_result.lsn, segno, instance_config.xlog_seg_size); /* @@ -1862,8 +1848,8 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb if (stop_backup_result.lsn % instance_config.xlog_seg_size == 0) { /* Wait for segment with current stop_lsn, it is ok for it to never arrive */ - wait_wal_lsn(instanceState, stop_backup_result.lsn, false, backup->tli, - false, true, WARNING, stream_wal, backup); + wait_wal_lsn(xlog_path, stop_backup_result.lsn, false, backup->tli, + false, true, WARNING, stream_wal); /* Get the first record in segment with current stop_lsn */ lsn_tmp = get_first_record_lsn(xlog_path, segno, backup->tli, @@ -1890,8 +1876,8 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb /* Despite looking for previous record there is not guarantee of success * because previous record can be the contrecord. */ - lsn_tmp = wait_wal_lsn(instanceState, stop_backup_result.lsn, false, backup->tli, - true, false, ERROR, stream_wal, backup); + lsn_tmp = wait_wal_lsn(xlog_path, stop_backup_result.lsn, false, backup->tli, + true, false, ERROR, stream_wal); /* sanity */ if (!XRecOffIsValid(lsn_tmp) || XLogRecPtrIsInvalid(lsn_tmp)) @@ -1904,8 +1890,8 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb else if (stop_backup_result.lsn % XLOG_BLCKSZ == 0) { /* Wait for segment with current stop_lsn */ - wait_wal_lsn(instanceState, stop_backup_result.lsn, false, backup->tli, - false, true, ERROR, stream_wal, backup); + wait_wal_lsn(xlog_path, stop_backup_result.lsn, false, backup->tli, + false, true, ERROR, stream_wal); /* Get the next closest record in segment with current stop_lsn */ lsn_tmp = get_next_record_lsn(xlog_path, segno, backup->tli, @@ -1967,8 +1953,8 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb * look for previous record with endpoint >= STOP_LSN. */ if (!stop_lsn_exists) - stop_backup_lsn = wait_wal_lsn(instanceState, stop_backup_result.lsn, false, backup->tli, - false, false, ERROR, stream_wal, backup); + stop_backup_lsn = wait_wal_lsn(xlog_path, stop_backup_result.lsn, false, backup->tli, + false, false, ERROR, stream_wal); if (stream_wal) { @@ -1976,15 +1962,7 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb * to the passed filelist */ if(wait_WAL_streaming_end(backup_files_list)) elog(ERROR, "WAL streaming failed"); - - snprintf(stream_xlog_path, lengthof(stream_xlog_path), "%s/%s/%s/%s", - instanceState->instance_backup_subdir_path, base36enc(backup->start_time), - DATABASE_DIR, PG_XLOG_DIR); - - xlog_path = stream_xlog_path; } - else - xlog_path = instanceState->instance_wal_subdir_path; backup->stop_lsn = stop_backup_lsn; backup->recovery_xid = stop_backup_result.snapshot_xid; From fffa8b14fd66be46db17e0cc7151cad66939215e Mon Sep 17 00:00:00 2001 From: "Mikhail A. Kulagin" Date: Mon, 24 May 2021 09:14:39 +0300 Subject: [PATCH 05/11] Refactor pg_stop_backup(): remove useless conn variable --- src/backup.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/backup.c b/src/backup.c index 317688f99..1ef50a22a 100644 --- a/src/backup.c +++ b/src/backup.c @@ -1756,7 +1756,6 @@ static void pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startbackup_conn, PGNodeInfo *nodeInfo) { - PGconn *conn; bool stop_lsn_exists = false; PGStopBackupResult stop_backup_result; char *xlog_path, stream_xlog_path[MAXPGPATH]; @@ -1769,9 +1768,7 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb if (!backup_in_progress) elog(ERROR, "backup is not in progress"); - conn = pg_startbackup_conn; - - pg_silent_client_messages(conn); + pg_silent_client_messages(pg_startbackup_conn); /* Create restore point * Only if backup is from master. @@ -1780,16 +1777,16 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb if (!backup->from_replica && !(nodeInfo->server_version < 90600 && !nodeInfo->is_superuser)) //TODO: check correctness - pg_create_restore_point(conn, backup->start_time); + pg_create_restore_point(pg_startbackup_conn, backup->start_time); /* Execute pg_stop_backup using PostgreSQL connection */ - pg_stop_backup_send(conn, nodeInfo->server_version, current.from_replica, exclusive_backup, &query_text); + pg_stop_backup_send(pg_startbackup_conn, nodeInfo->server_version, current.from_replica, exclusive_backup, &query_text); /* * Wait for the result of pg_stop_backup(), but no longer than * archive_timeout seconds */ - pg_stop_backup_consume(conn, nodeInfo->server_version, exclusive_backup, timeout, query_text, &stop_backup_result); + pg_stop_backup_consume(pg_startbackup_conn, nodeInfo->server_version, exclusive_backup, timeout, query_text, &stop_backup_result); if (stream_wal) { From ed5d71e6b3ce8b999251ee443dcb23ff3016766c Mon Sep 17 00:00:00 2001 From: "Mikhail A. Kulagin" Date: Mon, 24 May 2021 10:22:18 +0300 Subject: [PATCH 06/11] Make some functions and variables (from backup.c) accessible from other compilation units --- src/backup.c | 43 ++++++++----------------------------------- src/pg_probackup.h | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 36 deletions(-) diff --git a/src/backup.c b/src/backup.c index 1ef50a22a..74ac59780 100644 --- a/src/backup.c +++ b/src/backup.c @@ -27,7 +27,7 @@ //const char *progname = "pg_probackup"; /* list of files contained in backup */ -static parray *backup_files_list = NULL; +parray *backup_files_list = NULL; /* We need critical section for datapagemap_add() in case of using threads */ static pthread_mutex_t backup_pagemap_mutex = PTHREAD_MUTEX_INITIALIZER; @@ -36,25 +36,7 @@ static pthread_mutex_t backup_pagemap_mutex = PTHREAD_MUTEX_INITIALIZER; bool exclusive_backup = false; /* Is pg_start_backup() was executed */ -static bool backup_in_progress = false; - -typedef struct PGStopBackupResult -{ - /* - * We will use values of snapshot_xid and invocation_time if there are - * no transactions between start_lsn and stop_lsn. - */ - TransactionId snapshot_xid; - time_t invocation_time; - /* - * Fields that store pg_catalog.pg_stop_backup() result - */ - XLogRecPtr lsn; - size_t backup_label_content_len; - char *backup_label_content; - size_t tablespace_map_content_len; - char *tablespace_map_content; -} PGStopBackupResult; +bool backup_in_progress = false; /* * Backup routines @@ -66,18 +48,9 @@ static void *backup_files(void *arg); static void do_backup_pg(InstanceState *instanceState, PGconn *backup_conn, PGNodeInfo *nodeInfo, bool no_sync, bool backup_logs); -static void pg_start_backup(const char *label, bool smooth, pgBackup *backup, - PGNodeInfo *nodeInfo, PGconn *conn); static void pg_switch_wal(PGconn *conn); -static void pg_silent_client_messages(PGconn *conn); -static void pg_create_restore_point(PGconn *conn, time_t backup_start_time); static void pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startbackup_conn, PGNodeInfo *nodeInfo); -static void pg_stop_backup_send(PGconn *conn, int server_version, bool is_started_on_replica, bool is_exclusive, char **query_text); - -static XLogRecPtr wait_wal_lsn(const char *wal_segment_dir, XLogRecPtr lsn, bool is_start_lsn, TimeLineID tli, - bool in_prev_segment, bool segment_only, - int timeout_elevel, bool in_stream_dir); static void check_external_for_tablespaces(parray *external_list, PGconn *backup_conn); @@ -1062,7 +1035,7 @@ confirm_block_size(PGconn *conn, const char *name, int blcksz) /* * Notify start of backup to PostgreSQL server. */ -static void +void pg_start_backup(const char *label, bool smooth, pgBackup *backup, PGNodeInfo *nodeInfo, PGconn *conn) { @@ -1297,7 +1270,7 @@ pg_is_superuser(PGconn *conn) * Returns target LSN if such is found, failing that returns LSN of record prior to target LSN. * Returns InvalidXLogRecPtr if 'segment_only' flag is used. */ -static XLogRecPtr +XLogRecPtr wait_wal_lsn(const char *wal_segment_dir, XLogRecPtr target_lsn, bool is_start_lsn, TimeLineID tli, bool in_prev_segment, bool segment_only, int timeout_elevel, bool in_stream_dir) @@ -1459,7 +1432,7 @@ wait_wal_lsn(const char *wal_segment_dir, XLogRecPtr target_lsn, bool is_start_l } /* Remove annoying NOTICE messages generated by backend */ -static void +void pg_silent_client_messages(PGconn *conn) { PGresult *res; @@ -1468,7 +1441,7 @@ pg_silent_client_messages(PGconn *conn) PQclear(res); } -static void +void pg_create_restore_point(PGconn *conn, time_t backup_start_time) { PGresult *res; @@ -1578,7 +1551,7 @@ pg_stop_backup_send(PGconn *conn, int server_version, bool is_started_on_replica * parameters: * - */ -static void +void pg_stop_backup_consume(PGconn *conn, int server_version, bool is_exclusive, uint32 timeout, const char *query_text, PGStopBackupResult *result) @@ -1709,7 +1682,7 @@ pg_stop_backup_consume(PGconn *conn, int server_version, /* * helper routine used to write backup_label and tablespace_map in pg_stop_backup() */ -static void +void pg_stop_backup_write_file_helper(const char *path, const char *filename, const char *error_msg_filename, const void *data, size_t len, parray *file_list) { diff --git a/src/pg_probackup.h b/src/pg_probackup.h index 4da12d654..f662a25f5 100644 --- a/src/pg_probackup.h +++ b/src/pg_probackup.h @@ -600,7 +600,6 @@ typedef struct int ret; } backup_files_arg; - typedef struct timelineInfo timelineInfo; /* struct to collect info about timelines in WAL archive */ @@ -1260,4 +1259,42 @@ extern void start_WAL_streaming(PGconn *backup_conn, char *stream_dst_path, ConnectionOptions *conn_opt, XLogRecPtr startpos, TimeLineID starttli); extern int wait_WAL_streaming_end(parray *backup_files_list); + +/* external variables and functions, implemented in backup.c */ +typedef struct PGStopBackupResult +{ + /* + * We will use values of snapshot_xid and invocation_time if there are + * no transactions between start_lsn and stop_lsn. + */ + TransactionId snapshot_xid; + time_t invocation_time; + /* + * Fields that store pg_catalog.pg_stop_backup() result + */ + XLogRecPtr lsn; + size_t backup_label_content_len; + char *backup_label_content; + size_t tablespace_map_content_len; + char *tablespace_map_content; +} PGStopBackupResult; + +extern bool backup_in_progress; +extern parray *backup_files_list; + +extern void pg_start_backup(const char *label, bool smooth, pgBackup *backup, + PGNodeInfo *nodeInfo, PGconn *conn); +extern void pg_silent_client_messages(PGconn *conn); +extern void pg_create_restore_point(PGconn *conn, time_t backup_start_time); +extern void pg_stop_backup_send(PGconn *conn, int server_version, bool is_started_on_replica, bool is_exclusive, char **query_text); +extern void pg_stop_backup_consume(PGconn *conn, int server_version, + bool is_exclusive, uint32 timeout, const char *query_text, + PGStopBackupResult *result); +extern void pg_stop_backup_write_file_helper(const char *path, const char *filename, const char *error_msg_filename, + const void *data, size_t len, parray *file_list); +extern XLogRecPtr wait_wal_lsn(const char *wal_segment_dir, XLogRecPtr lsn, bool is_start_lsn, TimeLineID tli, + bool in_prev_segment, bool segment_only, + int timeout_elevel, bool in_stream_dir); + + #endif /* PG_PROBACKUP_H */ From f7196e89e37bfc5580f801ab5a4d065b721651c7 Mon Sep 17 00:00:00 2001 From: "Mikhail A. Kulagin" Date: Mon, 24 May 2021 11:12:00 +0300 Subject: [PATCH 07/11] Remove some references to global stream_wal variable --- src/backup.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/backup.c b/src/backup.c index 74ac59780..9765ac16a 100644 --- a/src/backup.c +++ b/src/backup.c @@ -259,7 +259,7 @@ do_backup_pg(InstanceState *instanceState, PGconn *backup_conn, write_backup(¤t, true); /* In PAGE mode or in ARCHIVE wal-mode wait for current segment */ - if (current.backup_mode == BACKUP_MODE_DIFF_PAGE || !stream_wal) + if (current.backup_mode == BACKUP_MODE_DIFF_PAGE || !current.stream) { /* Check that archive_dir can be reached */ if (fio_access(instanceState->instance_wal_subdir_path, F_OK, FIO_BACKUP_HOST) != 0) @@ -275,7 +275,7 @@ do_backup_pg(InstanceState *instanceState, PGconn *backup_conn, } /* start stream replication */ - if (stream_wal) + if (current.stream) { join_path_components(dst_backup_path, current.database_dir, PG_XLOG_DIR); fio_mkdir(dst_backup_path, DIR_PERMISSION, FIO_BACKUP_HOST); @@ -1076,7 +1076,7 @@ pg_start_backup(const char *label, bool smooth, pgBackup *backup, PQclear(res); - if ((!stream_wal || current.backup_mode == BACKUP_MODE_DIFF_PAGE) && + if ((!backup->stream || backup->backup_mode == BACKUP_MODE_DIFF_PAGE) && !backup->from_replica && !(nodeInfo->server_version < 90600 && !nodeInfo->is_superuser)) @@ -1261,7 +1261,7 @@ pg_is_superuser(PGconn *conn) * previous segment. * * Flag 'in_stream_dir' determine whether we looking for WAL in 'pg_wal' directory or - * in archive. Do note, that we cannot rely sorely on global variable 'stream_wal' because, + * in archive. Do note, that we cannot rely sorely on global variable 'stream_wal' (current.stream) because, * for example, PAGE backup must(!) look for start_lsn in archive regardless of wal_mode. * * 'timeout_elevel' determine the elevel for timeout elog message. If elevel lighter than @@ -1407,7 +1407,7 @@ wait_wal_lsn(const char *wal_segment_dir, XLogRecPtr target_lsn, bool is_start_l wal_delivery_str, wal_segment_path); } - if (!stream_wal && is_start_lsn && try_count == 30) + if (!current.stream && is_start_lsn && try_count == 30) elog(WARNING, "By default pg_probackup assume WAL delivery method to be ARCHIVE. " "If continuous archiving is not set up, use '--stream' option to make autonomous backup. " "Otherwise check that continuous archiving works correctly."); @@ -1753,7 +1753,7 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb pg_create_restore_point(pg_startbackup_conn, backup->start_time); /* Execute pg_stop_backup using PostgreSQL connection */ - pg_stop_backup_send(pg_startbackup_conn, nodeInfo->server_version, current.from_replica, exclusive_backup, &query_text); + pg_stop_backup_send(pg_startbackup_conn, nodeInfo->server_version, backup->from_replica, exclusive_backup, &query_text); /* * Wait for the result of pg_stop_backup(), but no longer than @@ -1761,7 +1761,7 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb */ pg_stop_backup_consume(pg_startbackup_conn, nodeInfo->server_version, exclusive_backup, timeout, query_text, &stop_backup_result); - if (stream_wal) + if (backup->stream) { snprintf(stream_xlog_path, lengthof(stream_xlog_path), "%s/%s/%s/%s", instanceState->instance_backup_subdir_path, @@ -1819,7 +1819,7 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb { /* Wait for segment with current stop_lsn, it is ok for it to never arrive */ wait_wal_lsn(xlog_path, stop_backup_result.lsn, false, backup->tli, - false, true, WARNING, stream_wal); + false, true, WARNING, backup->stream); /* Get the first record in segment with current stop_lsn */ lsn_tmp = get_first_record_lsn(xlog_path, segno, backup->tli, @@ -1847,7 +1847,7 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb * because previous record can be the contrecord. */ lsn_tmp = wait_wal_lsn(xlog_path, stop_backup_result.lsn, false, backup->tli, - true, false, ERROR, stream_wal); + true, false, ERROR, backup->stream); /* sanity */ if (!XRecOffIsValid(lsn_tmp) || XLogRecPtrIsInvalid(lsn_tmp)) @@ -1861,7 +1861,7 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb { /* Wait for segment with current stop_lsn */ wait_wal_lsn(xlog_path, stop_backup_result.lsn, false, backup->tli, - false, true, ERROR, stream_wal); + false, true, ERROR, backup->stream); /* Get the next closest record in segment with current stop_lsn */ lsn_tmp = get_next_record_lsn(xlog_path, segno, backup->tli, @@ -1924,9 +1924,9 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb */ if (!stop_lsn_exists) stop_backup_lsn = wait_wal_lsn(xlog_path, stop_backup_result.lsn, false, backup->tli, - false, false, ERROR, stream_wal); + false, false, ERROR, backup->stream); - if (stream_wal) + if (backup->stream) { /* This function will also add list of xlog files * to the passed filelist */ From bd9cd9f8a406c6204ba2979a8cf4f19961900c5a Mon Sep 17 00:00:00 2001 From: "Mikhail A. Kulagin" Date: Mon, 24 May 2021 11:38:53 +0300 Subject: [PATCH 08/11] remove unused variable externaldir --- src/pg_probackup.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/pg_probackup.c b/src/pg_probackup.c index 1b2e7f751..3150900b6 100644 --- a/src/pg_probackup.c +++ b/src/pg_probackup.c @@ -68,8 +68,6 @@ static char *backup_path = NULL; static CatalogState *catalogState = NULL; /* ================ catalogState (END) =========== */ -/* colon separated external directories list ("/path1:/path2") */ -char *externaldir = NULL; /* common options */ int num_threads = 1; bool stream_wal = false; From 1793c68e8dd7cf0e524ac13f41b049299d4db915 Mon Sep 17 00:00:00 2001 From: "Mikhail A. Kulagin" Date: Mon, 24 May 2021 14:35:35 +0300 Subject: [PATCH 09/11] Yet another split of pg_stop_backup(): separate verification of stop_lsn into wait_wal_and_calculate_stop_lsn() --- src/backup.c | 263 +++++++++++++++++++++++---------------------- src/pg_probackup.h | 2 +- 2 files changed, 138 insertions(+), 127 deletions(-) diff --git a/src/backup.c b/src/backup.c index 9765ac16a..7cb100fe2 100644 --- a/src/backup.c +++ b/src/backup.c @@ -1431,6 +1431,142 @@ wait_wal_lsn(const char *wal_segment_dir, XLogRecPtr target_lsn, bool is_start_l } } +/* + * Check stop_lsn (returned from pg_stop_backup()) and update backup->stop_lsn + */ +void +wait_wal_and_calculate_stop_lsn(const char *xlog_path, XLogRecPtr stop_lsn, pgBackup *backup) +{ + bool stop_lsn_exists = false; + + /* It is ok for replica to return invalid STOP LSN + * UPD: Apparently it is ok even for a master. + */ + if (!XRecOffIsValid(stop_lsn)) + { + XLogSegNo segno = 0; + XLogRecPtr lsn_tmp = InvalidXLogRecPtr; + + /* + * Even though the value is invalid, it's expected postgres behaviour + * and we're trying to fix it below. + */ + elog(LOG, "Invalid offset in stop_lsn value %X/%X, trying to fix", + (uint32) (stop_lsn >> 32), (uint32) (stop_lsn)); + + /* + * Note: even with gdb it is very hard to produce automated tests for + * contrecord + invalid LSN, so emulate it for manual testing. + */ + //lsn = lsn - XLOG_SEG_SIZE; + //elog(WARNING, "New Invalid stop_backup_lsn value %X/%X", + // (uint32) (stop_lsn >> 32), (uint32) (stop_lsn)); + + GetXLogSegNo(stop_lsn, segno, instance_config.xlog_seg_size); + + /* + * Note, that there is no guarantee that corresponding WAL file even exists. + * Replica may return LSN from future and keep staying in present. + * Or it can return invalid LSN. + * + * That's bad, since we want to get real LSN to save it in backup label file + * and to use it in WAL validation. + * + * So we try to do the following: + * 1. Wait 'archive_timeout' seconds for segment containing stop_lsn and + * look for the first valid record in it. + * It solves the problem of occasional invalid LSN on write-busy system. + * 2. Failing that, look for record in previous segment with endpoint + * equal or greater than stop_lsn. It may(!) solve the problem of invalid LSN + * on write-idle system. If that fails too, error out. + */ + + /* stop_lsn is pointing to a 0 byte of xlog segment */ + if (stop_lsn % instance_config.xlog_seg_size == 0) + { + /* Wait for segment with current stop_lsn, it is ok for it to never arrive */ + wait_wal_lsn(xlog_path, stop_lsn, false, backup->tli, + false, true, WARNING, backup->stream); + + /* Get the first record in segment with current stop_lsn */ + lsn_tmp = get_first_record_lsn(xlog_path, segno, backup->tli, + instance_config.xlog_seg_size, + instance_config.archive_timeout); + + /* Check that returned LSN is valid and greater than stop_lsn */ + if (XLogRecPtrIsInvalid(lsn_tmp) || + !XRecOffIsValid(lsn_tmp) || + lsn_tmp < stop_lsn) + { + /* Backup from master should error out here */ + if (!backup->from_replica) + elog(ERROR, "Failed to get next WAL record after %X/%X", + (uint32) (stop_lsn >> 32), + (uint32) (stop_lsn)); + + /* No luck, falling back to looking up for previous record */ + elog(WARNING, "Failed to get next WAL record after %X/%X, " + "looking for previous WAL record", + (uint32) (stop_lsn >> 32), + (uint32) (stop_lsn)); + + /* Despite looking for previous record there is not guarantee of success + * because previous record can be the contrecord. + */ + lsn_tmp = wait_wal_lsn(xlog_path, stop_lsn, false, backup->tli, + true, false, ERROR, backup->stream); + + /* sanity */ + if (!XRecOffIsValid(lsn_tmp) || XLogRecPtrIsInvalid(lsn_tmp)) + elog(ERROR, "Failed to get WAL record prior to %X/%X", + (uint32) (stop_lsn >> 32), + (uint32) (stop_lsn)); + } + } + /* stop lsn is aligned to xlog block size, just find next lsn */ + else if (stop_lsn % XLOG_BLCKSZ == 0) + { + /* Wait for segment with current stop_lsn */ + wait_wal_lsn(xlog_path, stop_lsn, false, backup->tli, + false, true, ERROR, backup->stream); + + /* Get the next closest record in segment with current stop_lsn */ + lsn_tmp = get_next_record_lsn(xlog_path, segno, backup->tli, + instance_config.xlog_seg_size, + instance_config.archive_timeout, + stop_lsn); + + /* sanity */ + if (!XRecOffIsValid(lsn_tmp) || XLogRecPtrIsInvalid(lsn_tmp)) + elog(ERROR, "Failed to get WAL record next to %X/%X", + (uint32) (stop_lsn >> 32), + (uint32) (stop_lsn)); + } + /* PostgreSQL returned something very illegal as STOP_LSN, error out */ + else + elog(ERROR, "Invalid stop_backup_lsn value %X/%X", + (uint32) (stop_lsn >> 32), (uint32) (stop_lsn)); + + /* Setting stop_backup_lsn will set stop point for streaming */ + stop_backup_lsn = lsn_tmp; + stop_lsn_exists = true; + } + + elog(LOG, "stop_lsn: %X/%X", + (uint32) (stop_lsn >> 32), (uint32) (stop_lsn)); + + /* + * Wait for stop_lsn to be archived or streamed. + * If replica returned valid STOP_LSN of not actually existing record, + * look for previous record with endpoint >= STOP_LSN. + */ + if (!stop_lsn_exists) + stop_backup_lsn = wait_wal_lsn(xlog_path, stop_lsn, false, backup->tli, + false, false, ERROR, backup->stream); + + backup->stop_lsn = stop_backup_lsn; +} + /* Remove annoying NOTICE messages generated by backend */ void pg_silent_client_messages(PGconn *conn) @@ -1729,7 +1865,6 @@ static void pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startbackup_conn, PGNodeInfo *nodeInfo) { - bool stop_lsn_exists = false; PGStopBackupResult stop_backup_result; char *xlog_path, stream_xlog_path[MAXPGPATH]; /* kludge against some old bug in archive_timeout. TODO: remove in 3.0.0 */ @@ -1772,121 +1907,7 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb else xlog_path = instanceState->instance_wal_subdir_path; - /* It is ok for replica to return invalid STOP LSN - * UPD: Apparently it is ok even for a master. - */ - if (!XRecOffIsValid(stop_backup_result.lsn)) - { - XLogSegNo segno = 0; - XLogRecPtr lsn_tmp = InvalidXLogRecPtr; - - /* - * Even though the value is invalid, it's expected postgres behaviour - * and we're trying to fix it below. - */ - elog(LOG, "Invalid offset in stop_lsn value %X/%X, trying to fix", - (uint32) (stop_backup_result.lsn >> 32), (uint32) (stop_backup_result.lsn)); - - /* - * Note: even with gdb it is very hard to produce automated tests for - * contrecord + invalid LSN, so emulate it for manual testing. - */ - //stop_backup_result.lsn = stop_backup_result.lsn - XLOG_SEG_SIZE; - //elog(WARNING, "New Invalid stop_backup_lsn value %X/%X", - // (uint32) (stop_backup_result.lsn >> 32), (uint32) (stop_backup_result.lsn)); - - GetXLogSegNo(stop_backup_result.lsn, segno, instance_config.xlog_seg_size); - - /* - * Note, that there is no guarantee that corresponding WAL file even exists. - * Replica may return LSN from future and keep staying in present. - * Or it can return invalid LSN. - * - * That's bad, since we want to get real LSN to save it in backup label file - * and to use it in WAL validation. - * - * So we try to do the following: - * 1. Wait 'archive_timeout' seconds for segment containing stop_lsn and - * look for the first valid record in it. - * It solves the problem of occasional invalid LSN on write-busy system. - * 2. Failing that, look for record in previous segment with endpoint - * equal or greater than stop_lsn. It may(!) solve the problem of invalid LSN - * on write-idle system. If that fails too, error out. - */ - - /* stop_lsn is pointing to a 0 byte of xlog segment */ - if (stop_backup_result.lsn % instance_config.xlog_seg_size == 0) - { - /* Wait for segment with current stop_lsn, it is ok for it to never arrive */ - wait_wal_lsn(xlog_path, stop_backup_result.lsn, false, backup->tli, - false, true, WARNING, backup->stream); - - /* Get the first record in segment with current stop_lsn */ - lsn_tmp = get_first_record_lsn(xlog_path, segno, backup->tli, - instance_config.xlog_seg_size, - instance_config.archive_timeout); - - /* Check that returned LSN is valid and greater than stop_lsn */ - if (XLogRecPtrIsInvalid(lsn_tmp) || - !XRecOffIsValid(lsn_tmp) || - lsn_tmp < stop_backup_result.lsn) - { - /* Backup from master should error out here */ - if (!backup->from_replica) - elog(ERROR, "Failed to get next WAL record after %X/%X", - (uint32) (stop_backup_result.lsn >> 32), - (uint32) (stop_backup_result.lsn)); - - /* No luck, falling back to looking up for previous record */ - elog(WARNING, "Failed to get next WAL record after %X/%X, " - "looking for previous WAL record", - (uint32) (stop_backup_result.lsn >> 32), - (uint32) (stop_backup_result.lsn)); - - /* Despite looking for previous record there is not guarantee of success - * because previous record can be the contrecord. - */ - lsn_tmp = wait_wal_lsn(xlog_path, stop_backup_result.lsn, false, backup->tli, - true, false, ERROR, backup->stream); - - /* sanity */ - if (!XRecOffIsValid(lsn_tmp) || XLogRecPtrIsInvalid(lsn_tmp)) - elog(ERROR, "Failed to get WAL record prior to %X/%X", - (uint32) (stop_backup_result.lsn >> 32), - (uint32) (stop_backup_result.lsn)); - } - } - /* stop lsn is aligned to xlog block size, just find next lsn */ - else if (stop_backup_result.lsn % XLOG_BLCKSZ == 0) - { - /* Wait for segment with current stop_lsn */ - wait_wal_lsn(xlog_path, stop_backup_result.lsn, false, backup->tli, - false, true, ERROR, backup->stream); - - /* Get the next closest record in segment with current stop_lsn */ - lsn_tmp = get_next_record_lsn(xlog_path, segno, backup->tli, - instance_config.xlog_seg_size, - instance_config.archive_timeout, - stop_backup_result.lsn); - - /* sanity */ - if (!XRecOffIsValid(lsn_tmp) || XLogRecPtrIsInvalid(lsn_tmp)) - elog(ERROR, "Failed to get WAL record next to %X/%X", - (uint32) (stop_backup_result.lsn >> 32), - (uint32) (stop_backup_result.lsn)); - } - /* PostgreSQL returned something very illegal as STOP_LSN, error out */ - else - elog(ERROR, "Invalid stop_backup_lsn value %X/%X", - (uint32) (stop_backup_result.lsn >> 32), (uint32) (stop_backup_result.lsn)); - - /* Setting stop_backup_lsn will set stop point for streaming */ - stop_backup_lsn = lsn_tmp; - stop_lsn_exists = true; - } - - elog(LOG, "stop_lsn: %X/%X", - (uint32) (stop_backup_result.lsn >> 32), (uint32) (stop_backup_result.lsn)); + wait_wal_and_calculate_stop_lsn(xlog_path, stop_backup_result.lsn, backup); /* Write backup_label and tablespace_map */ if (!exclusive_backup) @@ -1917,15 +1938,6 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb } } - /* - * Wait for stop_lsn to be archived or streamed. - * If replica returned valid STOP_LSN of not actually existing record, - * look for previous record with endpoint >= STOP_LSN. - */ - if (!stop_lsn_exists) - stop_backup_lsn = wait_wal_lsn(xlog_path, stop_backup_result.lsn, false, backup->tli, - false, false, ERROR, backup->stream); - if (backup->stream) { /* This function will also add list of xlog files @@ -1934,7 +1946,6 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb elog(ERROR, "WAL streaming failed"); } - backup->stop_lsn = stop_backup_lsn; backup->recovery_xid = stop_backup_result.snapshot_xid; elog(LOG, "Getting the Recovery Time from WAL"); diff --git a/src/pg_probackup.h b/src/pg_probackup.h index f662a25f5..60f1a4872 100644 --- a/src/pg_probackup.h +++ b/src/pg_probackup.h @@ -1295,6 +1295,6 @@ extern void pg_stop_backup_write_file_helper(const char *path, const char *filen extern XLogRecPtr wait_wal_lsn(const char *wal_segment_dir, XLogRecPtr lsn, bool is_start_lsn, TimeLineID tli, bool in_prev_segment, bool segment_only, int timeout_elevel, bool in_stream_dir); - +extern void wait_wal_and_calculate_stop_lsn(const char *xlog_path, XLogRecPtr stop_lsn, pgBackup *backup); #endif /* PG_PROBACKUP_H */ From 2c8b7e967ef7abe8a48841dcd1c7d7969cf12e33 Mon Sep 17 00:00:00 2001 From: "Mikhail A. Kulagin" Date: Wed, 26 May 2021 14:52:16 +0300 Subject: [PATCH 10/11] create pfilearray_clear_locks() helper function --- src/backup.c | 7 ++++--- src/checkdb.c | 3 ++- src/dir.c | 16 ++++++++++++++++ src/pg_probackup.h | 1 + src/restore.c | 8 ++++---- src/utils/parray.c | 2 +- src/validate.c | 6 +----- 7 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/backup.c b/src/backup.c index 7cb100fe2..f102de26b 100644 --- a/src/backup.c +++ b/src/backup.c @@ -430,7 +430,7 @@ do_backup_pg(InstanceState *instanceState, PGconn *backup_conn, } /* - * Make directories before backup and setup threads at the same time + * Make directories before backup */ for (i = 0; i < parray_num(backup_files_list); i++) { @@ -455,10 +455,11 @@ do_backup_pg(InstanceState *instanceState, PGconn *backup_conn, fio_mkdir(dirpath, DIR_PERMISSION, FIO_BACKUP_HOST); } - /* setup threads */ - pg_atomic_clear_flag(&file->lock); } + /* setup thread locks */ + pfilearray_clear_locks(backup_files_list); + /* Sort by size for load balancing */ parray_qsort(backup_files_list, pgFileCompareSize); /* Sort the array for binary search */ diff --git a/src/checkdb.c b/src/checkdb.c index 4ea1d0800..5d7d6652b 100644 --- a/src/checkdb.c +++ b/src/checkdb.c @@ -455,7 +455,6 @@ get_index_list(const char *dbname, bool first_db_with_amcheck, ind->heapallindexed_is_supported = heapallindexed_is_supported; ind->amcheck_nspname = pgut_malloc(strlen(amcheck_nspname) + 1); strcpy(ind->amcheck_nspname, amcheck_nspname); - pg_atomic_clear_flag(&ind->lock); if (index_list == NULL) index_list = parray_new(); @@ -463,6 +462,8 @@ get_index_list(const char *dbname, bool first_db_with_amcheck, parray_append(index_list, ind); } + pfilearray_clear_locks(index_list); + PQclear(res); return index_list; diff --git a/src/dir.c b/src/dir.c index 86848d8d5..dfcddd7d0 100644 --- a/src/dir.c +++ b/src/dir.c @@ -222,6 +222,8 @@ pgFileInit(const char *rel_path) /* Number of blocks backed up during backup */ file->n_headers = 0; + // May be add? + // pg_atomic_clear_flag(file->lock); return file; } @@ -1859,3 +1861,17 @@ cleanup_tablespace(const char *path) parray_walk(files, pgFileFree); parray_free(files); } + +/* + * Clear the synchronisation locks in a parray of (pgFile *)'s + */ +void +pfilearray_clear_locks(parray *file_list) +{ + int i; + for (i = 0; i < parray_num(file_list); i++) + { + pgFile *file = (pgFile *) parray_get(file_list, i); + pg_atomic_clear_flag(&file->lock); + } +} diff --git a/src/pg_probackup.h b/src/pg_probackup.h index 60f1a4872..a7979ed27 100644 --- a/src/pg_probackup.h +++ b/src/pg_probackup.h @@ -1061,6 +1061,7 @@ extern int pgFileCompareRelPathWithExternalDesc(const void *f1, const void *f2); extern int pgFileCompareLinked(const void *f1, const void *f2); extern int pgFileCompareSize(const void *f1, const void *f2); extern int pgCompareOid(const void *f1, const void *f2); +extern void pfilearray_clear_locks(parray *file_list); /* in data.c */ extern bool check_data_file(ConnectionArgs *arguments, pgFile *file, diff --git a/src/restore.c b/src/restore.c index b7071234a..4cef60005 100644 --- a/src/restore.c +++ b/src/restore.c @@ -824,7 +824,7 @@ restore_chain(pgBackup *dest_backup, parray *parent_chain, } /* - * Setup directory structure for external directories and file locks + * Setup directory structure for external directories */ for (i = 0; i < parray_num(dest_files); i++) { @@ -848,11 +848,11 @@ restore_chain(pgBackup *dest_backup, parray *parent_chain, elog(VERBOSE, "Create external directory \"%s\"", dirpath); fio_mkdir(dirpath, file->mode, FIO_DB_HOST); } - - /* setup threads */ - pg_atomic_clear_flag(&file->lock); } + /* setup threads */ + pfilearray_clear_locks(dest_files); + /* Get list of files in destination directory and remove redundant files */ if (params->incremental_mode != INCR_NONE || cleanup_pgdata) { diff --git a/src/utils/parray.c b/src/utils/parray.c index 31148ee9a..95b83365d 100644 --- a/src/utils/parray.c +++ b/src/utils/parray.c @@ -175,7 +175,7 @@ parray_rm(parray *array, const void *key, int(*compare)(const void *, const void size_t parray_num(const parray *array) { - return array->used; + return array!= NULL ? array->used : (size_t) 0; } void diff --git a/src/validate.c b/src/validate.c index f000698d0..4044ac158 100644 --- a/src/validate.c +++ b/src/validate.c @@ -130,11 +130,7 @@ pgBackupValidate(pgBackup *backup, pgRestoreParams *params) // params->partial_restore_type); /* setup threads */ - for (i = 0; i < parray_num(files); i++) - { - pgFile *file = (pgFile *) parray_get(files, i); - pg_atomic_clear_flag(&file->lock); - } + pfilearray_clear_locks(files); /* init thread args with own file lists */ threads = (pthread_t *) palloc(sizeof(pthread_t) * num_threads); From ebbf974066947925b780c4c192943fe2e8eaf576 Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Thu, 27 May 2021 03:08:52 +0300 Subject: [PATCH 11/11] [PR #387] minor refactoring --- src/backup.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/backup.c b/src/backup.c index f102de26b..21df1d95e 100644 --- a/src/backup.c +++ b/src/backup.c @@ -1899,10 +1899,7 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb if (backup->stream) { - snprintf(stream_xlog_path, lengthof(stream_xlog_path), - "%s/%s/%s/%s", instanceState->instance_backup_subdir_path, - base36enc(backup->start_time), - DATABASE_DIR, PG_XLOG_DIR); + join_path_components(stream_xlog_path, backup->database_dir, PG_XLOG_DIR); xlog_path = stream_xlog_path; } else @@ -1913,14 +1910,10 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb /* Write backup_label and tablespace_map */ if (!exclusive_backup) { - char path[MAXPGPATH]; - Assert(stop_backup_result.backup_label_content != NULL); - snprintf(path, lengthof(path), "%s/%s/%s", instanceState->instance_backup_subdir_path, - base36enc(backup->start_time), DATABASE_DIR); /* Write backup_label */ - pg_stop_backup_write_file_helper(path, PG_BACKUP_LABEL_FILE, "backup label", + pg_stop_backup_write_file_helper(backup->database_dir, PG_BACKUP_LABEL_FILE, "backup label", stop_backup_result.backup_label_content, stop_backup_result.backup_label_content_len, backup_files_list); free(stop_backup_result.backup_label_content); @@ -1930,7 +1923,7 @@ pg_stop_backup(InstanceState *instanceState, pgBackup *backup, PGconn *pg_startb /* Write tablespace_map */ if (stop_backup_result.tablespace_map_content != NULL) { - pg_stop_backup_write_file_helper(path, PG_TABLESPACE_MAP_FILE, "tablespace map", + pg_stop_backup_write_file_helper(backup->database_dir, PG_TABLESPACE_MAP_FILE, "tablespace map", stop_backup_result.tablespace_map_content, stop_backup_result.tablespace_map_content_len, backup_files_list); free(stop_backup_result.tablespace_map_content);