On Tue, Oct 21, 2014 at 7:35 AM, Brightwell, Adam < adam.brightw...@crunchydatasolutions.com> wrote:
> Julien, > > The following is an initial review: > > Thanks for the review. > * Applies cleanly to master (f330a6d). > * Regression tests updated and pass, including 'check-world'. > * Documentation updated and builds successfully. > * Might want to consider replacing the following magic number with a > constant or perhaps calculated value. > > + int basenamelen = (int) strlen(rlde->d_name) - 6; > > * Wouldn't it be easier, or perhaps more reliable to use "strrchr()" with > the following instead? > > + strcmp(rlde->d_name + basenamelen, ".ready") == 0) > > char *extension = strrchr(ride->d_name, '.'); > ... > strcmp(extension, ".ready") == 0) > > I think this approach might also help to resolve the magic number above. > For example: > > char *extension = strrchr(ride->d_name, '.'); > int basenamelen = (int) strlen(ride->d_name) - strlen(extension); > > Actually, I used the same loop as the archiver one (see backend/postmaster/pgarch.c, function pgarch_readyXlog) to get the exact same number of files. If we change it in this patch, it would be better to change it everywhere. What do you think ? -Adam > > -- > Adam Brightwell - adam.brightw...@crunchydatasolutions.com > Database Engineer - www.crunchydatasolutions.com >
