comments cleanup

archive.c

@@ -18,7 +18,7 @@
  * --wal-file-path %p --wal-file-name %f', to move backups into arclog_path.
  * Where archlog_path is $BACKUP_PATH/wal/system_id.
  * Currently it just copies wal files to the new location.
- * TODO Planned options: compress, list the arclog content,
+ * TODO: Planned options: compress, list the arclog content,
  * compute and validate checksums.
  */
 int

backup.c

@@ -654,7 +654,7 @@ pg_switch_wal(void)
 
 /*
  * Check if the instance supports ptrack
- * TODO Implement check of ptrack_version() instead of existing one
+ * TODO Maybe we should rather check ptrack_version()?
  */
 static bool
 pg_ptrack_support(void)
@@ -959,7 +959,6 @@ pg_stop_backup(pgBackup *backup)
 		fwrite(PQgetvalue(res, 0, 1), 1, strlen(PQgetvalue(res, 0, 1)), fp);
 		fclose(fp);
 
-		/* TODO What for do we save the file into backup_list? */
 		/*
 		 * It's vital to check if backup_files_list is initialized,
 		 * because we could get here because the backup was interrupted
@@ -1213,7 +1212,6 @@ backup_compressed_file_partially(pgFile *file, void *arg, size_t *skip_size)
  * verify checksum and copy.
  * In incremental backup mode, copy only files or datafiles' pages changed after
  * previous backup.
- * TODO review
  */
 static void
 backup_files(void *arg)
@@ -1268,31 +1266,11 @@ backup_files(void *arg)
 
 		if (S_ISREG(buf.st_mode))
 		{
-			/* skip files which have not been modified since last backup */
-			/* TODO Implement: compare oldfile and newfile checksum. Now it's just a stub */
-			if (arguments->prev_backup_filelist)
-			{
-				pgFile *prev_file = NULL;
-				pgFile **p = (pgFile **) parray_bsearch(arguments->prev_backup_filelist,
-														file, pgFileComparePath);
-				if (p)
-					prev_file = *p;
-
-				if (prev_file && false)
-				{
-					file->write_size = BYTES_INVALID;
-					elog(LOG, "File \"%s\" has not changed since previous backup",
-						 file->path);
-					continue;
-				}
-			}
-
 			/* copy the file into backup */
 			if (file->is_datafile)
 			{
 				if (is_compressed_data_file(file))
 				{
-					/* TODO review */
 					size_t skip_size = 0;
 					if (backup_compressed_file_partially(file, arguments, &skip_size))
 					{
@@ -1345,7 +1323,6 @@ backup_files(void *arg)
 
 /*
  * Append files to the backup list array.
- * TODO review
  */
 static void
 add_pgdata_files(parray *files, const char *root)
@@ -1370,7 +1347,7 @@ add_pgdata_files(parray *files, const char *root)
 		/* data files are under "base", "global", or "pg_tblspc" */
 		relative = GetRelativePath(file->path, root);
 		if (!path_is_prefix_of_path("base", relative) &&
-			/*!path_is_prefix_of_path("global", relative) &&*/ //TODO What's wrong with this line?
+			!path_is_prefix_of_path("global", relative) &&
 			!path_is_prefix_of_path(PG_TBLSPC_DIR, relative))
 			continue;
 
@@ -1600,7 +1577,10 @@ process_block_change(ForkNumber forknum, RelFileNode rnode, BlockNumber blkno)
 	pg_free(rel_path);
 }
 
-/* TODO review it */
+/*
+ * Given a list of files in the instance to backup, build a pagemap for each
+ * data file that has ptrack. Result is saved in the pagemap field of pgFile.
+ */
 static void
 make_pagemap_from_ptrack(parray *files)
 {
@@ -1622,6 +1602,8 @@ make_pagemap_from_ptrack(parray *files)
 			size_t		ptrack_nonparsed_size = 0;
 			size_t		start_addr;
 
+			/* Compute db_oid and rel_oid of the relation from the path */
+
 			tablespace = strstr(p->ptrack_path, PG_TBLSPC_DIR);
 
 			if (tablespace)
@@ -1647,19 +1629,25 @@ make_pagemap_from_ptrack(parray *files)
 					 p->path);
 
 			sscanf(p->path + sep_iter + 1, "%u/%u", &db_oid, &rel_oid);
-			
+
+			/* get ptrack map for all segments of the relation in a raw format */
 			ptrack_nonparsed = pg_ptrack_get_and_clear(tablespace_oid, db_oid,
 											   rel_oid, &ptrack_nonparsed_size);
 
-			/* TODO What is 8? */
+			/*
-			start_addr = (RELSEG_SIZE/8)*p->segno;
+			 * FIXME When do we cut VARHDR from ptrack_nonparsed?
-			if (start_addr + RELSEG_SIZE/8 > ptrack_nonparsed_size)
+			 * Compute the beginning of the ptrack map related to this segment
+			 */
+			start_addr = (RELSEG_SIZE/HEAPBLOCKS_PER_BYTE)*p->segno;
+
+			if (start_addr + RELSEG_SIZE/HEAPBLOCKS_PER_BYTE > ptrack_nonparsed_size)
 				p->pagemap.bitmapsize = ptrack_nonparsed_size - start_addr;
 			else
-				p->pagemap.bitmapsize =	RELSEG_SIZE/8;
+				p->pagemap.bitmapsize =	RELSEG_SIZE/HEAPBLOCKS_PER_BYTE;
 
 			p->pagemap.bitmap = pg_malloc(p->pagemap.bitmapsize);
 			memcpy(p->pagemap.bitmap, ptrack_nonparsed+start_addr, p->pagemap.bitmapsize);
+
 			pg_free(ptrack_nonparsed);
 		}
 	}
@@ -1815,7 +1803,7 @@ StreamLog(void *arg)
  * cfs_mmap() and cfs_munmap() function definitions mirror ones
  * from cfs.h, but doesn't use atomic variables, since they are
  * not allowed in frontend code.
- * TODO Is it so?
+ *
  * Since we cannot take atomic lock on files compressed by CFS,
  * it should care about not changing files while backup is running.
  */

data.c

@@ -147,12 +147,6 @@ backup_data_page(pgFile *file, XLogRecPtr prev_backup_start_lsn,
 		 * If after several attempts page header is still invalid, throw an error.
 		 * The same idea is applied to checksum verification.
 		 */
-
-		/*
-		 * TODO Should we show a hint about possible false positives suggesting to
-		 * decrease concurrent load? Or we can just copy this page and rely on
-		 * xlog recovery, marking backup as untrusted.
-		 */
 		if (!parse_page(&page, &page_lsn))
 		{
 			int i;
@@ -346,6 +340,7 @@ backup_data_file(const char *from_root, const char *to_root,
 			n_blocks_read++;
 		}
 
+		pg_free(&file->pagemap);
 		pg_free(iter);
 	}
 
@@ -364,7 +359,7 @@ backup_data_file(const char *from_root, const char *to_root,
 
 	FIN_CRC32C(file->crc);
 
-	/* Treat empty file as not-datafile. TODO Why? */
+	/* Treat empty file as not-datafile. */
 	if (file->read_size == 0)
 		file->is_datafile = false;
 
@@ -385,7 +380,6 @@ backup_data_file(const char *from_root, const char *to_root,
 
 /*
  * Restore compressed file that was backed up partly.
- * TODO review
  */
 static void
 restore_file_partly(const char *from_root,const char *to_root, pgFile *file)
@@ -563,8 +557,7 @@ restore_data_file(const char *from_root,
 		if (header.block < blknum)
 			elog(ERROR, "backup is broken at block %u", blknum);
 
-		/* TODO fix this assert */
+		Assert(header.compressed_size <= BLCKSZ);
-		Assert (header.compressed_size <= BLCKSZ);
 
 		read_len = fread(compressed_page.data, 1,
 			MAXALIGN(header.compressed_size), in);

dir.c

@@ -345,7 +345,7 @@ dir_list_file(parray *files, const char *root, bool exclude, bool omit_symlink,
 }
 
 /*
- * TODO Add comment, review
+ * TODO Add comment
  */
 static void
 dir_list_file_internal(parray *files, const char *root, bool exclude,
@@ -679,7 +679,6 @@ print_file_list(FILE *out, const parray *files, const char *root)
 		if (file->is_datafile)
 			fprintf(out, ",\"segno\":\"%d\"", file->segno);
 
-		/* TODO What for do we write it to file? */
 		if (S_ISLNK(file->mode))
 			fprintf(out, ",\"linked\":\"%s\"", file->linked);
 

pg_probackup.h

@@ -33,6 +33,12 @@
 
 #include "datapagemap.h"
 
+/*
+ * Macro needed to parse ptrack.
+ * NOTE Keep those values syncronised with definitions in ptrack.h
+ */
+#define PTRACK_BITS_PER_HEAPBLOCK 1
+#define HEAPBLOCKS_PER_BYTE (BITS_PER_BYTE / PTRACK_BITS_PER_HEAPBLOCK)
 
 /* Directory/File names */
 #define DATABASE_DIR			"database"

restore.c

@@ -219,7 +219,8 @@ do_restore_or_validate(time_t target_backup_id,
 
 	/*
 	 * Validate corresponding WAL files.
-	 * TODO Shouldn't we pass recovery_target_timeline as last argument?
+	 * We pass base_full_backup timeline as last argument to this function,
+	 * because it's needed to form the name of xlog file.
 	 */
 	validate_wal(dest_backup, arclog_path, rt->recovery_target_time,
 						 rt->recovery_target_xid, base_full_backup->tli);
@@ -246,7 +247,7 @@ do_restore_or_validate(time_t target_backup_id,
 		if (dest_backup->backup_mode != BACKUP_MODE_FULL)
 			remove_deleted_files(dest_backup);
 
-		/* TODO Add comment */
+		/* Create recovery.conf with given recovery target parameters */
 		if (!dest_backup->stream
 			|| (target_time != NULL || target_xid != NULL))
 		{
@@ -867,7 +868,6 @@ satisfy_timeline(const parray *timelines, const pgBackup *backup)
 /*
  * Get recovery options in the string format, parse them
  * and fill up the pgRecoveryTarget structure.
- * TODO move arguments parsing and validation to getopt.
  */
 pgRecoveryTarget *
 parseRecoveryTargetOptions(const char *target_time,

show.c

@@ -162,7 +162,6 @@ pretty_size(int64 size, char *buf, size_t len)
 	}
 }
 
-/* TODO Add comment */
 static TimeLineID
 get_parent_tli(TimeLineID child_tli)
 {

util.c

@@ -75,7 +75,9 @@ digestControlFile(ControlFileData *ControlFile, char *src, size_t size)
 	checkControlFile(ControlFile);
 }
 
-/* TODO Add comment */
+/*
+ * Get lsn of the moment when ptrack was enabled the last time.
+ */
 XLogRecPtr
 get_last_ptrack_lsn(void)
 {