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