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