Comparison unsigned expression less then zero

[japinli]

Hi,

I find there are some unnecessary comparison for unsigned type, for example

		size_t pos = ftell(out);

		if (pos < 0)
			elog(ERROR, "Cannot get position in destination file \"%s\": %s",
				 to_fullpath, strerror(errno));

Since the pos is size_t, which is unsigned, so the following if condition alway false.
I checked that the ftell() returns long, which is singed.

The do_delete() and do_delete_status() have the same problems. we declaration
the size_to_delete to unsigned, however, the check condition always return true.

Here is a patch for those.

diff --git a/src/data.c b/src/data.c
index f02e3fd1..ec42813a 100644
--- a/src/data.c
+++ b/src/data.c
@@ -2321,7 +2321,7 @@ copy_pages(const char *to_fullpath, const char *from_fullpath,
 		elog(ERROR, "Cannot seek to end of file position in destination file \"%s\": %s",
 			 to_fullpath, strerror(errno));
 	{
-		size_t pos = ftell(out);
+		long pos = ftell(out);
 
 		if (pos < 0)
 			elog(ERROR, "Cannot get position in destination file \"%s\": %s",
diff --git a/src/delete.c b/src/delete.c
index 6c70ff81..6335482f 100644
--- a/src/delete.c
+++ b/src/delete.c
@@ -36,7 +36,7 @@ do_delete(InstanceState *instanceState, time_t backup_id)
 	parray	   *backup_list,
 			   *delete_list;
 	pgBackup   *target_backup = NULL;
-	size_t		size_to_delete = 0;
+	int64		size_to_delete = 0;
 	char		size_to_delete_pretty[20];
 
 	/* Get complete list of backups */
@@ -1027,7 +1027,7 @@ do_delete_status(InstanceState *instanceState, InstanceConfig *instance_config,
 	parray     *backup_list, *delete_list;
 	const char *pretty_status;
 	int         n_deleted = 0, n_found = 0;
-	size_t      size_to_delete = 0;
+	int64       size_to_delete = 0;
 	char        size_to_delete_pretty[20];
 	pgBackup   *backup;

instance_config.wal_depth >= 0 also always true, since wal_depth is unsigned. Should we remove the condition check, or change it to signed?

Here is a patch to fix this by removing the condition check.

diff --git a/src/delete.c b/src/delete.c
index 6c70ff81..9467c8dc 100644
--- a/src/delete.c
+++ b/src/delete.c
@@ -682,12 +682,11 @@ do_retention_wal(InstanceState *instanceState, bool dry_run)
                 * at least one backup and no file should be removed.
                 * Unless wal-depth is enabled.
                 */
-               if ((tlinfo->closest_backup) && instance_config.wal_depth <= 0)
+               if ((tlinfo->closest_backup) && instance_config.wal_depth == 0)
                        continue;

                /* WAL retention keeps this timeline from purge */
-               if (instance_config.wal_depth >= 0 && tlinfo->anchor_tli > 0 &&
-                       tlinfo->anchor_tli != tlinfo->tli)
+               if (tlinfo->anchor_tli > 0 && tlinfo->anchor_tli != tlinfo->tli)
                        continue;

                /*
@@ -701,7 +700,7 @@ do_retention_wal(InstanceState *instanceState, bool dry_run)
                 */
                if (tlinfo->oldest_backup)
                {
-                       if (instance_config.wal_depth >= 0 && !(XLogRecPtrIsInvalid(tlinfo->anchor_lsn)))
+                       if (!(XLogRecPtrIsInvalid(tlinfo->anchor_lsn)))
                        {
                                delete_walfiles_in_tli(instanceState, tlinfo->anchor_lsn,
                                                                tlinfo, instance_config.xlog_seg_size, dry_run);
@@ -714,7 +713,7 @@ do_retention_wal(InstanceState *instanceState, bool dry_run)
                }
                else
                {
-                       if (instance_config.wal_depth >= 0 && !(XLogRecPtrIsInvalid(tlinfo->anchor_lsn)))
+                       if (!(XLogRecPtrIsInvalid(tlinfo->anchor_lsn)))
                                delete_walfiles_in_tli(instanceState, tlinfo->anchor_lsn,
                                                                tlinfo, instance_config.xlog_seg_size, dry_run);
                        else
@@ -942,7 +941,7 @@ delete_walfiles_in_tli(InstanceState *instanceState, XLogRecPtr keep_lsn, timeli
                        join_path_components(wal_fullpath, instanceState->instance_wal_subdir_path, wal_file->file.name);

                        /* save segment from purging */
-                       if (instance_config.wal_depth >= 0 && wal_file->keep)
+                       if (wal_file->keep)
                        {
                                elog(VERBOSE, "Retain WAL segment \"%s\"", wal_fullpath);
                                continue;

Any suggestions?

[dlepikhova]

Thanks for your observation! Your comments about unsigned parameters seem to be correct. Could you attach all fixes as a pull request instead of text so we can look at the test results?

[japinli]

Thanks for your observation! Your comments about unsigned parameters seem to be correct. Could you attach all fixes as a pull request instead of text so we can look at the test results?

Create a pull request #483. After some review, I find the ftello() use %lu formator, It might be not correct, since off_t is signed type, right? After some search, I found the off_t might be long long int or long int, but I'm not sure. IMO, we should use %ld instead of %lu. Anyway, a signed formator is more correct than an unsigned formator, isn't it?

[dlepikhova]

Yes, you are right about signed long int type. As I found in commit history, this formator stayed from previous redaction of this part of code, then the block number was used here. So, using %ld might well be justified for these two error messages. I think, it's worth adding this fix to the pull request.