On Wed, Jul 1, 2015 at 8:18 PM, Fujii Masao wrote:
> On Wed, Jul 1, 2015 at 8:16 PM, Fujii Masao wrote:
>> I updated the patch as follows. Patch attached.
>>
>> +#define XLogFileNameExtended(fname, tli, log, seg)
>>
>> Move this macro to xlog_internal.h because it's used both in
>> pg_standby and pg_archivecleanup. There seems no need to
>> define it independently.
OK for me.
>> -#define MAXFNAMELEN 64
>> +#define MAXFNAMELEN 64
>>
>> Revert this unnecessary change.
Yes, thanks.
>>
>> +/* Length of XLog file name */
>> +#define XLOG_DATA_FNAME_LEN 24
>>
>> Shorten the name of this macro variable, to XLOG_FNAME_LEN,
>> for more code readability.
Thanks. You have more talent for naming than I do.
>> Comments?
Just reading it again, I think that XLogFileNameById should use
MAXFNAMELEN, and that XLogFileName should call directly
XLogFileNameById as both are doing the same operation like in the
attached. It seems also safer to use MAXFNAMELEN instead of MAXPGPATH
for exclusiveCleanupFileName in pg_standby.c and pg_archivecleanup.c.
--
Michael
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 2f9f2b4..861caea 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -32,6 +32,8 @@
#include "pg_getopt.h"
+#include "access/xlog_internal.h"
+
const char *progname;
/* Options and defaults */
@@ -57,7 +59,7 @@ char *restartWALFileName; /* the file from which we can restart restore */
char *priorWALFileName; /* the file we need to get from archive */
char WALFilePath[MAXPGPATH]; /* the file path including archive */
char restoreCommand[MAXPGPATH]; /* run this to restore */
-char exclusiveCleanupFileName[MAXPGPATH]; /* the file we need to
+char exclusiveCleanupFileName[MAXFNAMELEN]; /* the file we need to
* get from archive */
/*
@@ -113,11 +115,6 @@ struct stat stat_buf;
* folded in to later versions of this program.
*/
-#define XLOG_DATA_FNAME_LEN 24
-/* Reworked from access/xlog_internal.h */
-#define XLogFileName(fname, tli, log, seg) \
- snprintf(fname, XLOG_DATA_FNAME_LEN + 1, "%08X%08X%08X", tli, log, seg)
-
/*
* Initialize allows customized commands into the warm standby program.
*
@@ -182,10 +179,7 @@ CustomizableNextWALFileReady()
* If it's a backup file, return immediately. If it's a regular file
* return only if it's the right size already.
*/
- if (strlen(nextWALFileName) > 24 &&
- strspn(nextWALFileName, "0123456789ABCDEF") == 24 &&
- strcmp(nextWALFileName + strlen(nextWALFileName) - strlen(".backup"),
- ".backup") == 0)
+ if (IsBackupHistoryFileName(nextWALFileName))
{
nextWALFileType = XLOG_BACKUP_LABEL;
return true;
@@ -261,8 +255,7 @@ CustomizableCleanupPriorWALFiles(void)
* are not removed in the order they were originally written,
* in case this worries you.
*/
- if (strlen(xlde->d_name) == XLOG_DATA_FNAME_LEN &&
- strspn(xlde->d_name, "0123456789ABCDEF") == XLOG_DATA_FNAME_LEN &&
+ if (IsXLogFileName(xlde->d_name) &&
strcmp(xlde->d_name + 8, exclusiveCleanupFileName + 8) < 0)
{
#ifdef WIN32
@@ -366,7 +359,7 @@ SetWALFileNameForCleanup(void)
}
}
- XLogFileName(exclusiveCleanupFileName, tli, log, seg);
+ XLogFileNameById(exclusiveCleanupFileName, tli, log, seg);
return cleanup;
}
@@ -740,10 +733,7 @@ main(int argc, char **argv)
* Check for initial history file: always the first file to be requested
* It's OK if the file isn't there - all other files need to wait
*/
- if (strlen(nextWALFileName) > 8 &&
- strspn(nextWALFileName, "0123456789ABCDEF") == 8 &&
- strcmp(nextWALFileName + strlen(nextWALFileName) - strlen(".history"),
- ".history") == 0)
+ if (IsTLHistoryFileName(nextWALFileName))
{
nextWALFileType = XLOG_HISTORY;
if (RestoreWALFileForRecovery())
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index ba6e242..579a9bb 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -21,6 +21,8 @@
#include "pg_getopt.h"
+#include "access/xlog_internal.h"
+
const char *progname;
/* Options and defaults */
@@ -31,7 +33,7 @@ char *additional_ext = NULL; /* Extension to remove from filenames */
char *archiveLocation; /* where to find the archive? */
char *restartWALFileName; /* the file from which we can restart restore */
char WALFilePath[MAXPGPATH]; /* the file path including archive */
-char exclusiveCleanupFileName[MAXPGPATH]; /* the oldest file we
+char exclusiveCleanupFileName[MAXFNAMELEN]; /* the oldest file we
* want to remain in
* archive */
@@ -51,12 +53,6 @@ char exclusiveCleanupFileName[MAXPGPATH]; /* the oldest file we
* folded in to later versions of this program.
*/
-#define XLOG_DATA_FNAME_LEN 24
-/* Reworked from access/xlog_internal.h */
-#define XLogFileName(fname, tli, log, seg) \
- snprintf(fname, XLOG_DATA_FNAME_LEN + 1, "%08X%08X%08X", tli, log, seg)
-#define XLOG_BACKUP_FNAME_LEN 40
-
/*
* Initialize allows customized commands into the archive cleanup program.
*
@@ -110,7 +106,7 @@ CleanupPriorWALFiles(void)
{
/*
* Truncation is essentially harmless, because we skip names of
- * length other than XLOG_DATA_FNAME_LEN. (In principle, one
+ * length other than XLOG_FNAME_LEN. (In principle, one
* could use a 1000-character additional_ext and get trouble.)
*/
strlcpy(walfile, xlde->d_name, MAXPGPATH);
@@ -129,8 +125,7 @@ CleanupPriorWALFiles(void)
* file. Note that this means files are not removed in the order
* they were originally written, in case this worries you.
*/
- if (strlen(walfile) == XLOG_DATA_FNAME_LEN &&
- strspn(walfile, "0123456789ABCDEF") == XLOG_DATA_FNAME_LEN &&
+ if (IsXLogFileName(walfile) &&
strcmp(walfile + 8, exclusiveCleanupFileName + 8) < 0)
{
/*
@@ -202,13 +197,12 @@ SetWALFileNameForCleanup(void)
* 000000010000000000000010.00000020.backup is after
* 000000010000000000000010.
*/
- if (strlen(restartWALFileName) == XLOG_DATA_FNAME_LEN &&
- strspn(restartWALFileName, "0123456789ABCDEF") == XLOG_DATA_FNAME_LEN)
+ if (IsXLogFileName(restartWALFileName))
{
strcpy(exclusiveCleanupFileName, restartWALFileName);
fnameOK = true;
}
- else if (strlen(restartWALFileName) == XLOG_BACKUP_FNAME_LEN)
+ else if (IsBackupHistoryFileName(restartWALFileName))
{
int args;
uint32 tli = 1,
@@ -225,7 +219,7 @@ SetWALFileNameForCleanup(void)
* Use just the prefix of the filename, ignore everything after
* first period
*/
- XLogFileName(exclusiveCleanupFileName, tli, log, seg);
+ XLogFileNameById(exclusiveCleanupFileName, tli, log, seg);
}
}
diff --git a/src/bin/pg_resetxlog/pg_resetxlog.c b/src/bin/pg_resetxlog/pg_resetxlog.c
index 6ffe795..e19a72b 100644
--- a/src/bin/pg_resetxlog/pg_resetxlog.c
+++ b/src/bin/pg_resetxlog/pg_resetxlog.c
@@ -261,7 +261,7 @@ main(int argc, char *argv[])
break;
case 'l':
- if (strspn(optarg, "01234567890ABCDEFabcdef") != 24)
+ if (strspn(optarg, "01234567890ABCDEFabcdef") != XLOG_FNAME_LEN)
{
fprintf(stderr, _("%s: invalid argument for option %s\n"), progname, "-l");
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
@@ -976,8 +976,7 @@ KillExistingXLOG(void)
while (errno = 0, (xlde = readdir(xldir)) != NULL)
{
- if (strlen(xlde->d_name) == 24 &&
- strspn(xlde->d_name, "0123456789ABCDEF") == 24)
+ if (IsXLogFileName(xlde->d_name))
{
snprintf(path, MAXPGPATH, "%s/%s", XLOGDIR, xlde->d_name);
if (unlink(path) < 0)
@@ -1027,9 +1026,9 @@ KillExistingArchiveStatus(void)
while (errno = 0, (xlde = readdir(xldir)) != NULL)
{
- if (strspn(xlde->d_name, "0123456789ABCDEF") == 24 &&
- (strcmp(xlde->d_name + 24, ".ready") == 0 ||
- strcmp(xlde->d_name + 24, ".done") == 0))
+ if (strspn(xlde->d_name, "0123456789ABCDEF") == XLOG_FNAME_LEN &&
+ (strcmp(xlde->d_name + XLOG_FNAME_LEN, ".ready") == 0 ||
+ strcmp(xlde->d_name + XLOG_FNAME_LEN, ".done") == 0))
{
snprintf(path, MAXPGPATH, "%s/%s", ARCHSTATDIR, xlde->d_name);
if (unlink(path) < 0)
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index fbf9324..c09650e 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -137,13 +137,20 @@ typedef XLogLongPageHeaderData *XLogLongPageHeader;
*/
#define MAXFNAMELEN 64
+/* Length of XLog file name */
+#define XLOG_FNAME_LEN 24
+
+#define XLogFileNameById(fname, tli, log, seg) \
+ snprintf(fname, MAXFNAMELEN, "%08X%08X%08X", tli, log, seg)
+
#define XLogFileName(fname, tli, logSegNo) \
- snprintf(fname, MAXFNAMELEN, "%08X%08X%08X", tli, \
- (uint32) ((logSegNo) / XLogSegmentsPerXLogId), \
- (uint32) ((logSegNo) % XLogSegmentsPerXLogId))
+ XLogFileNameById(fname, tli, \
+ (uint32) ((logSegNo) / XLogSegmentsPerXLogId), \
+ (uint32) ((logSegNo) % XLogSegmentsPerXLogId))
#define IsXLogFileName(fname) \
- (strlen(fname) == 24 && strspn(fname, "0123456789ABCDEF") == 24)
+ (strlen(fname) == XLOG_FNAME_LEN && \
+ strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN)
/*
* XLOG segment with .partial suffix. Used by pg_receivexlog and at end of
@@ -151,9 +158,9 @@ typedef XLogLongPageHeaderData *XLogLongPageHeader;
* be complete yet.
*/
#define IsPartialXLogFileName(fname) \
- (strlen(fname) == 24 + strlen(".partial") && \
- strspn(fname, "0123456789ABCDEF") == 24 && \
- strcmp((fname) + 24, ".partial") == 0)
+ (strlen(fname) == XLOG_FNAME_LEN + strlen(".partial") && \
+ strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN && \
+ strcmp((fname) + XLOG_FNAME_LEN, ".partial") == 0)
#define XLogFromFileName(fname, tli, logSegNo) \
do { \
@@ -188,8 +195,8 @@ typedef XLogLongPageHeaderData *XLogLongPageHeader;
(uint32) ((logSegNo) % XLogSegmentsPerXLogId), offset)
#define IsBackupHistoryFileName(fname) \
- (strlen(fname) > 24 && \
- strspn(fname, "0123456789ABCDEF") == 24 && \
+ (strlen(fname) > XLOG_FNAME_LEN && \
+ strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN && \
strcmp((fname) + strlen(fname) - strlen(".backup"), ".backup") == 0)
#define BackupHistoryFilePath(path, tli, logSegNo, offset) \
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
