Skip to content

Commit 8e03b8d

Browse files
kulaginmkulaginm
committed
Review answer #1 
1 parent 271cf16 commit 8e03b8d

File tree

3 files changed

+26
-18
lines changed

3 files changed

+26
-18
lines changed

‎src/catchup.c‎

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,9 @@ catchup_collect_info(PGNodeInfo *source_node_info, const char *source_pgdata, co
7272
/* Get WAL segments size and system ID of source PG instance */
7373
instance_config.xlog_seg_size = get_xlog_seg_size(source_pgdata);
7474
instance_config.system_identifier = get_system_identifier(source_pgdata);
75+
#if PG_VERSION_NUM < 90600
76+
instance_config.pgdata = source_pgdata;
77+
#endif
7578
current.start_time = time(NULL);
7679

7780
StrNCpy(current.program_version, PROGRAM_VERSION, sizeof(current.program_version));
@@ -113,7 +116,10 @@ catchup_collect_info(PGNodeInfo *source_node_info, const char *source_pgdata, co
113116
return source_conn;
114117
}
115118

116-
//REVIEW Please add a comment to this function.
119+
/*
120+
* Check that catchup can be performed on source and dest
121+
* this function is for checks, that can be performed without modification of data on disk
122+
*/
117123
static void
118124
catchup_preflight_checks(PGNodeInfo *source_node_info, PGconn *source_conn,
119125
const char *source_pgdata, const char *dest_pgdata, bool dest_pgdata_is_empty)
@@ -164,16 +170,13 @@ do_catchup_instance(const char *source_pgdata, const char *dest_pgdata, PGconn *
164170
RedoParams dest_redo = { 0, InvalidXLogRecPtr, 0 };
165171
pgFile *source_pg_control_file = NULL;
166172

167-
//REVIEW please adjust this comment.
168-
/* arrays with meta info for multi threaded backup */
173+
/* arrays with meta info for multi threaded catchup */
169174
pthread_t *threads;
170175
catchup_thread_runner_arg *threads_args;
171176
bool catchup_isok = true;
172177

173178
parray *source_filelist = NULL;
174179
parray *dest_filelist = NULL;
175-
//REVIEW We don't handle external_dirs in catchup, do we? Let's clean this up.
176-
parray *external_dirs = NULL;
177180

178181
//REVIEW FIXME Let's fix it before release. It can cause some obscure bugs.
179182
/* TODO: in case of timeline mistmatch, check that source PG timeline descending from dest PG timeline */
@@ -199,8 +202,6 @@ do_catchup_instance(const char *source_pgdata, const char *dest_pgdata, PGconn *
199202
strncat(label, " with pg_probackup", lengthof(label) -
200203
strlen(" with pg_probackup"));
201204

202-
//REVIEW FIXME Let' do that.
203-
204205
/* Call pg_start_backup function in PostgreSQL connect */
205206
pg_start_backup(label, smooth_checkpoint, &current, source_node_info, source_conn);
206207
elog(LOG, "pg_start_backup START LSN %X/%X", (uint32) (current.start_lsn >> 32), (uint32) (current.start_lsn));
@@ -231,8 +232,8 @@ do_catchup_instance(const char *source_pgdata, const char *dest_pgdata, PGconn *
231232

232233
// new ptrack is more robust and checks Start LSN
233234
if (ptrack_lsn > dest_redo.lsn || ptrack_lsn == InvalidXLogRecPtr)
234-
elog(ERROR, "LSN from ptrack_control %X/%X is greater than checkpoint LSN %X/%X.\n"
235-
"Create new full backup before an incremental one.",
235+
elog(ERROR, "LSN from ptrack_control in source %X/%X is greater than checkpoint LSN in destination %X/%X.\n"
236+
"You can perform only FULL catchup.",
236237
(uint32) (ptrack_lsn >> 32), (uint32) (ptrack_lsn),
237238
(uint32) (dest_redo.lsn >> 32),
238239
(uint32) (dest_redo.lsn));
@@ -255,8 +256,6 @@ do_catchup_instance(const char *source_pgdata, const char *dest_pgdata, PGconn *
255256
current.start_lsn, current.tli);
256257
}
257258

258-
//REVIEW please adjust the comment.
259-
/* initialize backup list */
260259
source_filelist = parray_new();
261260

262261
/* list files with the logical path. omit $PGDATA */
@@ -471,14 +470,14 @@ do_catchup_instance(const char *source_pgdata, const char *dest_pgdata, PGconn *
471470
//REVIEW Hmm. Why do we need this at all?
472471
//I'd expect that we init pgfile with unset lock...
473472
//Not related to this patch, though.
473+
//REVIEW_ANSWER initialization in the pgFileInit function was proposed but was not accepted (see 2c8b7e9)
474474
/* clear file locks */
475475
pfilearray_clear_locks(source_filelist);
476476

477477
/* Sort by size for load balancing */
478478
parray_qsort(source_filelist, pgFileCompareSize);
479479

480-
//REVIEW. This comment looks a bit misleading, since all theads share same filelist.
481-
/* init thread args with own file lists */
480+
/* init thread args */
482481
threads = (pthread_t *) palloc(sizeof(pthread_t) * num_threads);
483482
threads_args = (catchup_thread_runner_arg *) palloc(sizeof(catchup_thread_runner_arg) * num_threads);
484483

@@ -539,6 +538,7 @@ do_catchup_instance(const char *source_pgdata, const char *dest_pgdata, PGconn *
539538
pretty_time);
540539

541540
//REVIEW The comment looks unrelated to the function. Do I miss something?
541+
//REVIEW_ANSWER because it is a part of pg_stop_backup() calling
542542
/* Notify end of backup */
543543
pg_silent_client_messages(source_conn);
544544

@@ -681,12 +681,9 @@ do_catchup_instance(const char *source_pgdata, const char *dest_pgdata, PGconn *
681681
parray_walk(dest_filelist, pgFileFree);
682682
parray_free(dest_filelist);
683683
}
684-
685684
parray_walk(source_filelist, pgFileFree);
686685
parray_free(source_filelist);
687686
pgFileFree(source_pg_control_file);
688-
//REVIEW Huh?
689-
// где закрывается conn?
690687
}
691688

692689
/*

‎src/utils/file.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2001,7 +2001,7 @@ fio_copy_pages(const char *to_fullpath, const char *from_fullpath, pgFile *file,
20012001

20022002
COMP_FILE_CRC32(true, file->crc, buf, hdr.size);
20032003

2004-
elog(INFO, "Copy block %u with size %u of %s", blknum, hdr.size - sizeof(BackupPageHeader), to_fullpath);
2004+
elog(INFO, "Copy block %u with size %lu of %s", blknum, hdr.size - sizeof(BackupPageHeader), to_fullpath);
20052005
if (fio_fseek(out, blknum * BLCKSZ) < 0)
20062006
{
20072007
elog(ERROR, "Cannot seek block %u of \"%s\": %s",

‎tests/catchup.py‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,9 @@ def test_multithread_local_transfer(self):
1414
"""
1515
fname = self.id().split('.')[3]
1616

17-
source_pg = self.make_simple_node(base_dir = os.path.join(module_name, fname, 'src'))
17+
source_pg = self.make_simple_node(
18+
base_dir = os.path.join(module_name, fname, 'src'),
19+
set_replication=True)
1820
source_pg.slow_start()
1921
source_pg.safe_psql(
2022
"postgres",
@@ -140,6 +142,9 @@ def test_remote_ptrack_catchup(self):
140142
generate some load on master, insert some test data on master,
141143
catchup copy, start and select test data
142144
"""
145+
if not self.ptrack:
146+
return unittest.skip('Skipped because ptrack support is disabled')
147+
143148
fname = self.id().split('.')[3]
144149

145150
# prepare master
@@ -263,6 +268,9 @@ def test_remote_delta_catchup(self):
263268
def test_table_drop(self):
264269
"""
265270
"""
271+
if not self.ptrack:
272+
return unittest.skip('Skipped because ptrack support is disabled')
273+
266274
fname = self.id().split('.')[3]
267275

268276
source_pg = self.make_simple_node(
@@ -317,6 +325,9 @@ def test_table_drop(self):
317325
def test_tablefile_truncation(self):
318326
"""
319327
"""
328+
if not self.ptrack:
329+
return unittest.skip('Skipped because ptrack support is disabled')
330+
320331
fname = self.id().split('.')[3]
321332

322333
source_pg = self.make_simple_node(

0 commit comments

Comments
 (0)