code cleanup

src/backup.c

@@ -47,6 +47,7 @@ const char *progname = "pg_probackup";
 
 /* list of files contained in backup */
 static parray *backup_files_list = NULL;
+/* list of indexes for use in checkdb --amcheck */
 static parray *index_list = NULL;
 
 /* We need critical section for datapagemap_add() in case of using threads */
@@ -95,7 +96,6 @@ static bool pg_stop_backup_is_sent = false;
  */
 static void backup_cleanup(bool fatal, void *userdata);
 static void backup_disconnect(bool fatal, void *userdata);
-//static void threads_conn_disconnect(bool fatal, void *userdata);
 
 static void pgdata_basic_setup(void);
 
@@ -941,6 +941,7 @@ do_backup_instance(void)
 	backup_files_list = NULL;
 }
 
+/* collect list of files and run threads to check files in the instance */
 static void
 do_block_validation(void)
 {
@@ -1022,18 +1023,19 @@ do_block_validation(void)
 			check_isok = false;
 	}
 
-	/* TODO write better info message */
-	if (check_isok)
-		elog(INFO, "Data files are valid");
-	else
-		elog(ERROR, "Checkdb failed");
-
+	/* cleanup */
 	if (backup_files_list)
 	{
 		parray_walk(backup_files_list, pgFileFree);
 		parray_free(backup_files_list);
 		backup_files_list = NULL;
 	}
+
+	/* TODO write better info message */
+	if (check_isok)
+		elog(INFO, "Data files are valid");
+	else
+		elog(ERROR, "Checkdb failed");
 }
 
 static void
@@ -1113,9 +1115,7 @@ do_amcheck(void)
 		for (j = 0; j < num_threads; j++)
 		{
 			backup_files_arg *arg = &(threads_args[j]);
-
 			elog(VERBOSE, "Start thread num: %i", j);
-
 			pthread_create(&threads[j], NULL, check_indexes, arg);
 		}
 
@@ -1143,13 +1143,14 @@ do_amcheck(void)
 	else
 		elog(INFO, "Checkdb --amcheck executed");
 
+	/* FIXME We already wrote message about skipped databases.
+	 * Maybe we shouldn't check db_skipped here ?
+	 */
 	if (check_isok && !interrupted && !db_skipped)
 		elog(INFO, "Indexes are valid");
 }
 
 /* Entry point of pg_probackup CHECKDB subcommand. */
-/* TODO consider moving some code common with do_backup_instance
- * to separate function ot to pgdata_basic_setup */
 int
 do_checkdb(bool need_amcheck)
 {
@@ -1173,7 +1174,6 @@ do_checkdb(bool need_amcheck)
  * Common code for CHECKDB and BACKUP commands.
  * Ensure that we're able to connect to the instance
  * check compatibility and fill basic info.
- * TODO maybe move it to pg_probackup.c
  */
 static void
 pgdata_basic_setup(void)
@@ -1441,7 +1441,6 @@ check_system_identifiers(void)
 		elog(ERROR, "Backup data directory was initialized for system id " UINT64_FORMAT ", "
 					"but connected instance system id is " UINT64_FORMAT,
 			 instance_config.system_identifier, system_id_conn);
-
 	if (system_id_pgdata != instance_config.system_identifier)
 		elog(ERROR, "Backup data directory was initialized for system id " UINT64_FORMAT ", "
 					"but target backup directory system id is " UINT64_FORMAT,
@@ -2542,23 +2541,11 @@ backup_disconnect(bool fatal, void *userdata)
 }
 
 /*
- * Disconnect checkdb connections created in threads during quit pg_probackup.
+ * Check files in PGDATA.
+ * Read all files listed in backup_files_list.
+ * If the file is 'datafile' (regular relation's main fork), read it page by page,
+ * verify checksum and copy.
  */
-//static void
-//threads_conn_disconnect(bool fatal, void *userdata)
-//{
-//
-//	backup_files_arg *arguments = (backup_files_arg *) userdata;
-//
-//	elog(VERBOSE, "threads_conn_disconnect, num_threads %d", arguments->thread_num);
-//
-//	if (arguments->backup_conn)
-//	{
-//		pgut_cancel(arguments->backup_conn);
-//		pgut_disconnect(arguments->backup_conn);
-//	}
-//}
-
 static void *
 check_files(void *arg)
 {
@@ -2605,7 +2592,7 @@ check_files(void *arg)
 			else
 			{
 				elog(ERROR,
-					"can't stat file to backup \"%s\": %s",
+					"can't stat file to check \"%s\": %s",
 					file->path, strerror(errno));
 			}
 		}
@@ -2625,19 +2612,21 @@ check_files(void *arg)
 									 file->path + strlen(arguments->from_root) + 1);
 
 				if (!check_data_file(arguments, file))
-					arguments->ret = 2;
+					arguments->ret = 2; /* FIXME what does 2 mean? */
 			}
 		}
 		else
 			elog(WARNING, "unexpected file type %d", buf.st_mode);
 	}
 
+	/* FIXME why do we reset return code here? */
 	if (arguments->ret == 1)
 		arguments->ret = 0;
 
 	return NULL;
 }
 
+/* Check indexes with amcheck */
 static void *
 check_indexes(void *arg)
 {
@@ -2645,7 +2634,6 @@ check_indexes(void *arg)
 	backup_files_arg *arguments = (backup_files_arg *) arg;
 	int			n_indexes = 0;
 
-	/* Check indexes with amcheck */
 	if (arguments->index_list)
 		n_indexes = parray_num(arguments->index_list);
 

src/data.c

@@ -1612,6 +1612,12 @@ validate_one_page(Page page, pgFile *file,
 
 }
 
+/*
+ * Valiate pages of datafile in PGDATA one by one.
+ *
+ * returns true if the file is valid
+ * also returns true if the file was not found
+ */
 bool
 check_data_file(backup_files_arg* arguments,
 				pgFile *file)
@@ -1622,14 +1628,8 @@ check_data_file(backup_files_arg* arguments,
 	int			n_blocks_skipped = 0;
 	int			page_state;
 	char		curr_page[BLCKSZ];
+	bool 		is_valid = true;
 
-	bool is_valid = true;
-
-	/* reset size summary */
-	file->read_size = 0;
-	file->write_size = 0;
-
-	/* open backup mode file for read */
 	in = fopen(file->path, PG_BINARY_R);
 	if (in == NULL)
 	{
@@ -1662,7 +1662,7 @@ check_data_file(backup_files_arg* arguments,
 
 	for (blknum = 0; blknum < nblocks; blknum++)
 	{
-		page_state = prepare_page(arguments, file, InvalidXLogRecPtr, //0 = InvalidXLogRecPtr
+		page_state = prepare_page(arguments, file, InvalidXLogRecPtr,
 									blknum, nblocks, in, &n_blocks_skipped,
 									BACKUP_MODE_FULL, curr_page, false);
 
@@ -1672,6 +1672,7 @@ check_data_file(backup_files_arg* arguments,
 		if (page_state == PageIsCorrupted)
 		{
 			/* Page is corrupted */
+			// TODO why this message is commented?
 			//elog(WARNING, "File %s, block %u is CORRUPTED.",
 			//		file->path, blknum);
 			is_valid = false;
@@ -1686,7 +1687,6 @@ check_data_file(backup_files_arg* arguments,
 			/* Page is corrupted */
 			is_valid = false;
 		}
-
 	}
 
 	fclose(in);

src/pg_probackup.h

@@ -139,7 +139,6 @@ typedef struct pg_indexEntry
 	Oid indexrelid;
 	char *name;
 	char *dbname;
-	char *snapshot; /* snapshot for index check */
 	char *amcheck_nspname; /* schema where amcheck extention is located */
 	volatile pg_atomic_flag lock;	/* lock for synchronization of parallel threads  */
 } pg_indexEntry;

src/utils/pgut.c

@@ -43,7 +43,6 @@ static void on_interrupt(void);
 static void on_cleanup(void);
 static pqsigfunc oldhandler = NULL;
 
-//bool is_result_ready(PGconn *conn);
 void discard_response(PGconn *conn);
 
 void
@@ -405,17 +404,6 @@ pgut_execute_parallel(PGconn* conn,
 								*/
 								(text_result) ? 0 : 1);
 
-		/* wait for processing */
-//		while(!is_result_ready(conn))
-//		{
-//			if (interrupted)
-//			{
-//				pgut_cancel(conn);
-//				pgut_disconnect(conn);
-//				elog(ERROR, "Interrupted");
-//			}
-//		}
-
 		/* wait for processing, TODO: timeout */
 		for (;;)
 		{
@@ -1052,38 +1040,3 @@ discard_response(PGconn *conn)
 			PQclear(res);
 	} while (res);
 }
-
-//bool is_result_ready(PGconn * conn)
-//{
-//   int                 sock;
-//   struct timeval      timeout;
-//   fd_set              read_mask;
-//
-//   if (!PQisBusy(conn))
-//       return true;
-//
-//   sock = PQsocket(conn);
-//
-//   timeout.tv_sec  = (time_t)1;
-//   timeout.tv_usec = 0;
-//
-//   FD_ZERO(&read_mask);
-//   FD_SET(sock, &read_mask);
-//
-//   if (select(sock + 1, &read_mask, NULL, NULL, &timeout) == 0)
-//       return false;
-//   else if (FD_ISSET(sock, &read_mask))
-//   {
-//       if (PQconsumeInput(conn))
-//       {
-//           if (PQisBusy(conn))
-//               return false;
-//           else
-//               return true;
-//       }
-//       else
-//           return false;
-//   }
-//   else
-//       return false;
-//}