Thread: 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
Attachment
On Wed, Jun 10, 2015 at 2:41 PM, Michael Paquier <michael.paquier@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
On Wed, Jul 1, 2015 at 8:16 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Wed, Jun 10, 2015 at 2:41 PM, Michael Paquier > <michael.paquier@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
Attachment
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
Attachment
On Wed, Jul 1, 2015 at 8:58 PM, Michael Paquier <michael.paquier@gmail.com> 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 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. 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
On Wed, Jul 1, 2015 at 9:53 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Wed, Jul 1, 2015 at 8:58 PM, Michael Paquier > <michael.paquier@gmail.com> 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 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. > > 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
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