Re: [HACKERS] Expending the use of xlog_internal.h's macros
On Thu, Jul 2, 2015 at 10:37 AM, Fujii Masao wrote: > On Wed, Jul 1, 2015 at 9:53 PM, Fujii Masao wrote: > > +/* 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. >> >> We can refactor the code that way, but which looks a bit messy >> at least to me. The original coding looks simpler and easier-readable, >> so I'd like to adopt the original one here. >> >>> It seems also safer to use MAXFNAMELEN instead of MAXPGPATH >>> for exclusiveCleanupFileName in pg_standby.c and pg_archivecleanup.c. >> >> Yep. > > Pushed. Thanks! Thanks! I was going to send a patch with what you wanted but you just beat me at it. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Expending the use of xlog_internal.h's macros
On Wed, Jul 1, 2015 at 9:53 PM, Fujii Masao wrote: > On Wed, Jul 1, 2015 at 8:58 PM, Michael Paquier > wrote: >> 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 MAXFNAMELEN64 +#define MAXFNAMELEN64 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. > > We can refactor the code that way, but which looks a bit messy > at least to me. The original coding looks simpler and easier-readable, > so I'd like to adopt the original one here. > >> It seems also safer to use MAXFNAMELEN instead of MAXPGPATH >> for exclusiveCleanupFileName in pg_standby.c and pg_archivecleanup.c. > > Yep. Pushed. Thanks! Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Expending the use of xlog_internal.h's macros
On Wed, Jul 1, 2015 at 8:58 PM, Michael Paquier wrote: > 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 MAXFNAMELEN64 >>> +#define MAXFNAMELEN64 >>> >>> 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. We can refactor the code that way, but which looks a bit messy at least to me. The original coding looks simpler and easier-readable, so I'd like to adopt the original one here. > It seems also safer to use MAXFNAMELEN instead of MAXPGPATH > for exclusiveCleanupFileName in pg_standby.c and pg_archivecleanup.c. Yep. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Expending the use of xlog_internal.h's macros
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 MAXFNAMELEN64
>> +#define MAXFNAMELEN64
>>
>> 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
Re: [HACKERS] Expending the use of xlog_internal.h's macros
On Wed, Jul 1, 2015 at 8:16 PM, Fujii Masao wrote:
> On Wed, Jun 10, 2015 at 2:41 PM, Michael Paquier
> 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 MAXFNAMELEN64
> +#define MAXFNAMELEN64
>
> 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
Re: [HACKERS] Expending the use of xlog_internal.h's macros
On Wed, Jun 10, 2015 at 2:41 PM, Michael Paquier 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 MAXFNAMELEN64 +#define MAXFNAMELEN64 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Expending the use of xlog_internal.h's macros
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.
Regards,
--
Michael
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 2f9f2b4..4b78209 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 */
@@ -113,9 +115,8 @@ 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) \
+#define XLogFileNameExtended(fname, tli, log, seg) \
snprintf(fname, XLOG_DATA_FNAME_LEN + 1, "%08X%08X%08X", tli, log, seg)
/*
@@ -182,10 +183,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 +259,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 +363,7 @@ SetWALFileNameForCleanup(void)
}
}
- XLogFileName(exclusiveCleanupFileName, tli, log, seg);
+ XLogFileNameExtended(exclusiveCleanupFileName, tli, log, seg);
return cleanup;
}
@@ -740,10 +737,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..ded36cc 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 */
@@ -51,11 +53,9 @@ 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) \
+#define XLogFileNameExtended(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.
@@ -129,8 +129,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 +201,12 @@ SetWALFileNameForCleanup(void)
* 00010010.0020.backup is after
* 00010010.
*/
- 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 +223,7 @@ SetWALFileNameForCleanup(void)
* Use just the prefix of the filename, ignore everything after
* first period
*/
- XLogFileName(exclusiveCleanupFileName, tli, log, seg);
+ XLogFileNameExtended
