- Notifications
You must be signed in to change notification settings - Fork86
Closed
Milestone
Description
Hi,
I find there are some unnecessary comparison for unsigned type, for example
size_tpos=ftell(out);if (pos<0)elog(ERROR,"Cannot get position in destination file \"%s\": %s",to_fullpath,strerror(errno));
Since thepos
issize_t
, which is unsigned, so the following if condition alway false.
I checked that theftell()
returnslong
, which is singed.
Thedo_delete()
anddo_delete_status()
have the same problems. we declaration
thesize_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.cindex 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.cindex 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_tsize_to_delete = 0;+int64size_to_delete = 0; charsize_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, sincewal_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.cindex 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?
Metadata
Metadata
Assignees
Labels
No labels