On Wed, Jul 1, 2015 at 8:16 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
> On Wed, Jun 10, 2015 at 2:41 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
>> Hi all,
>>
>> While looking at the code of pg_archivecleanup.c, I noticed that there
>> is some code present to detect if a given string has the format of a
>> WAL segment file name or of a backup file.
>> The recent commit 179cdd09 has introduced in xlog_internal.h a set of
>> macros to facilitate checks of pg_xlog's name format:
>> IsPartialXLogFileName(), IsXLogFileName() and IsTLHistoryFileName().
>>
>> And by looking at the code, there are some utilities where we could
>> make use of that, like pg_resetxlog, pg_archivecleanup and pg_standby.
>>
>> Attached is a small refactoring patch doing so for HEAD.
>
> Thanks for the patch!
>
> 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.
>
> -#define MAXFNAMELEN 64
> +#define MAXFNAMELEN 64
>
> Revert this unnecessary change.
>
> +/* 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.
>
> Comments?
>
> Regards,
>
> --
> Fujii Masao
Patch attached.
Regards,
--
Fujii Masao
*** a/contrib/pg_standby/pg_standby.c
--- b/contrib/pg_standby/pg_standby.c
***************
*** 32,37 ****
--- 32,39 ----
#include "pg_getopt.h"
+ #include "access/xlog_internal.h"
+
const char *progname;
/* Options and defaults */
***************
*** 113,123 **** 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.
*
--- 115,120 ----
***************
*** 182,191 **** 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)
{
nextWALFileType = XLOG_BACKUP_LABEL;
return true;
--- 179,185 ----
* If it's a backup file, return immediately. If it's a regular file
* return only if it's the right size already.
*/
! if (IsBackupHistoryFileName(nextWALFileName))
{
nextWALFileType = XLOG_BACKUP_LABEL;
return true;
***************
*** 261,268 **** 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 &&
strcmp(xlde->d_name + 8, exclusiveCleanupFileName + 8) < 0)
{
#ifdef WIN32
--- 255,261 ----
* are not removed in the order they were originally written,
* in case this worries you.
*/
! if (IsXLogFileName(xlde->d_name) &&
strcmp(xlde->d_name + 8, exclusiveCleanupFileName + 8) < 0)
{
#ifdef WIN32
***************
*** 366,372 **** SetWALFileNameForCleanup(void)
}
}
! XLogFileName(exclusiveCleanupFileName, tli, log, seg);
return cleanup;
}
--- 359,365 ----
}
}
! XLogFileNameById(exclusiveCleanupFileName, tli, log, seg);
return cleanup;
}
***************
*** 740,749 **** 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)
{
nextWALFileType = XLOG_HISTORY;
if (RestoreWALFileForRecovery())
--- 733,739 ----
* 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 (IsTLHistoryFileName(nextWALFileName))
{
nextWALFileType = XLOG_HISTORY;
if (RestoreWALFileForRecovery())
*** a/src/bin/pg_archivecleanup/pg_archivecleanup.c
--- b/src/bin/pg_archivecleanup/pg_archivecleanup.c
***************
*** 21,26 ****
--- 21,28 ----
#include "pg_getopt.h"
+ #include "access/xlog_internal.h"
+
const char *progname;
/* Options and defaults */
***************
*** 51,62 **** 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.
*
--- 53,58 ----
***************
*** 110,116 **** CleanupPriorWALFiles(void)
{
/*
* Truncation is essentially harmless, because we skip names of
! * length other than XLOG_DATA_FNAME_LEN. (In principle, one
* could use a 1000-character additional_ext and get trouble.)
*/
strlcpy(walfile, xlde->d_name, MAXPGPATH);
--- 106,112 ----
{
/*
* Truncation is essentially harmless, because we skip names of
! * 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,136 **** 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 &&
strcmp(walfile + 8, exclusiveCleanupFileName + 8) < 0)
{
/*
--- 125,131 ----
* file. Note that this means files are not removed in the order
* they were originally written, in case this worries you.
*/
! if (IsXLogFileName(walfile) &&
strcmp(walfile + 8, exclusiveCleanupFileName + 8) < 0)
{
/*
***************
*** 202,214 **** SetWALFileNameForCleanup(void)
* 000000010000000000000010.00000020.backup is after
* 000000010000000000000010.
*/
! if (strlen(restartWALFileName) == XLOG_DATA_FNAME_LEN &&
! strspn(restartWALFileName, "0123456789ABCDEF") == XLOG_DATA_FNAME_LEN)
{
strcpy(exclusiveCleanupFileName, restartWALFileName);
fnameOK = true;
}
! else if (strlen(restartWALFileName) == XLOG_BACKUP_FNAME_LEN)
{
int args;
uint32 tli = 1,
--- 197,208 ----
* 000000010000000000000010.00000020.backup is after
* 000000010000000000000010.
*/
! if (IsXLogFileName(restartWALFileName))
{
strcpy(exclusiveCleanupFileName, restartWALFileName);
fnameOK = true;
}
! else if (IsBackupHistoryFileName(restartWALFileName))
{
int args;
uint32 tli = 1,
***************
*** 225,231 **** SetWALFileNameForCleanup(void)
* Use just the prefix of the filename, ignore everything after
* first period
*/
! XLogFileName(exclusiveCleanupFileName, tli, log, seg);
}
}
--- 219,225 ----
* Use just the prefix of the filename, ignore everything after
* first period
*/
! XLogFileNameById(exclusiveCleanupFileName, tli, log, seg);
}
}
*** a/src/bin/pg_resetxlog/pg_resetxlog.c
--- b/src/bin/pg_resetxlog/pg_resetxlog.c
***************
*** 261,267 **** main(int argc, char *argv[])
break;
case 'l':
! if (strspn(optarg, "01234567890ABCDEFabcdef") != 24)
{
fprintf(stderr, _("%s: invalid argument for option %s\n"), progname, "-l");
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
--- 261,267 ----
break;
case 'l':
! 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,983 **** KillExistingXLOG(void)
while (errno = 0, (xlde = readdir(xldir)) != NULL)
{
! if (strlen(xlde->d_name) == 24 &&
! strspn(xlde->d_name, "0123456789ABCDEF") == 24)
{
snprintf(path, MAXPGPATH, "%s/%s", XLOGDIR, xlde->d_name);
if (unlink(path) < 0)
--- 976,982 ----
while (errno = 0, (xlde = readdir(xldir)) != NULL)
{
! if (IsXLogFileName(xlde->d_name))
{
snprintf(path, MAXPGPATH, "%s/%s", XLOGDIR, xlde->d_name);
if (unlink(path) < 0)
***************
*** 1027,1035 **** 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))
{
snprintf(path, MAXPGPATH, "%s/%s", ARCHSTATDIR, xlde->d_name);
if (unlink(path) < 0)
--- 1026,1034 ----
while (errno = 0, (xlde = readdir(xldir)) != NULL)
{
! 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)
*** a/src/include/access/xlog_internal.h
--- b/src/include/access/xlog_internal.h
***************
*** 137,149 **** typedef XLogLongPageHeaderData *XLogLongPageHeader;
*/
#define MAXFNAMELEN 64
#define XLogFileName(fname, tli, logSegNo) \
snprintf(fname, MAXFNAMELEN, "%08X%08X%08X", tli, \
(uint32) ((logSegNo) / XLogSegmentsPerXLogId), \
(uint32) ((logSegNo) % XLogSegmentsPerXLogId))
#define IsXLogFileName(fname) \
! (strlen(fname) == 24 && strspn(fname, "0123456789ABCDEF") == 24)
/*
* XLOG segment with .partial suffix. Used by pg_receivexlog and at end of
--- 137,156 ----
*/
#define MAXFNAMELEN 64
+ /* Length of XLog file name */
+ #define XLOG_FNAME_LEN 24
+
#define XLogFileName(fname, tli, logSegNo) \
snprintf(fname, MAXFNAMELEN, "%08X%08X%08X", tli, \
(uint32) ((logSegNo) / XLogSegmentsPerXLogId), \
(uint32) ((logSegNo) % XLogSegmentsPerXLogId))
+ #define XLogFileNameById(fname, tli, log, seg) \
+ snprintf(fname, XLOG_FNAME_LEN + 1, "%08X%08X%08X", tli, log, seg)
+
#define IsXLogFileName(fname) \
! (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,159 **** typedef XLogLongPageHeaderData *XLogLongPageHeader;
* be complete yet.
*/
#define IsPartialXLogFileName(fname) \
! (strlen(fname) == 24 + strlen(".partial") && \
! strspn(fname, "0123456789ABCDEF") == 24 && \
! strcmp((fname) + 24, ".partial") == 0)
#define XLogFromFileName(fname, tli, logSegNo) \
do { \
--- 158,166 ----
* be complete yet.
*/
#define IsPartialXLogFileName(fname) \
! (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,195 **** typedef XLogLongPageHeaderData *XLogLongPageHeader;
(uint32) ((logSegNo) % XLogSegmentsPerXLogId), offset)
#define IsBackupHistoryFileName(fname) \
! (strlen(fname) > 24 && \
! strspn(fname, "0123456789ABCDEF") == 24 && \
strcmp((fname) + strlen(fname) - strlen(".backup"), ".backup") == 0)
#define BackupHistoryFilePath(path, tli, logSegNo, offset) \
--- 195,202 ----
(uint32) ((logSegNo) % XLogSegmentsPerXLogId), offset)
#define IsBackupHistoryFileName(fname) \
! (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
