On Sat, Dec 13, 2014 at 10:48 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > > Andrew Dunstan <and...@dunslane.net> writes: > > On 11/20/2014 02:27 AM, Amit Kapila wrote: > >> Now the part where I would like to receive feedback before revising the > >> patch is on the coding style. It seems to me from Tom's comments that > >> he is not happy with the code, now I am not sure which part of the patch > >> he thinks needs change. Tom if possible, could you be slightly more > >> specific about your concern w.r.t code? > > > In view of the request above for comments from Tom, I have moved this > > back to "Needs Review". > > Sorry, I was not paying very close attention to this thread and missed > the request for comments. A few such: > > 1. The patch is completely naive about what might be in the symlink > path string; eg embedded spaces in the path would break it. On at > least some platforms, newlines could be in the path as well. I'm not > sure about how to guard against this while maintaining human readability > of the file. >
I will look into this and see what best can be done. > 2. There seems to be more going on here than what is advertised, eg > why do we need to add an option to the BASE_BACKUP command This is to ensure that symlink file is generated only for tar format; server is not aware of whether the backup is generated for plain format or tar format. We don't want to do it for plain format as for that client (pg_basebackup) can update the symlinks via -T option and backing up symlink file during that operation can lead to spurious symlinks after archive recovery. I have given the reason why we want to accomplish it only for tar format in my initial mail. > (and if > we do need it, doesn't it need to be documented in protocol.sgml)? I shall take care of it in next version of patch. > And why is the RelationCacheInitFileRemove call relocated? > Because it assumes that tablespace directory pg_tblspc is in place and it tries to remove the files by reading pg_tblspc directory as well. Now as we setup the symlinks in pg_tblspc after reading symlink file, so we should remove relcache init file once the symlinks are setup in pg_tblspc directory. > 3. Not terribly happy with the changes made to the API of > do_pg_start_backup, eg having to be able to parse "DIR *" in its > arguments seems like a lot of #include creep. xlog.h is pretty > central so I'm not happy about plastering more #includes in it. > The reason of adding new include in xlog.c is for use of tablespaceinfo structure which I have now kept in basebackup.h. The reason why I have done this way is because do_pg_start_backup has some functionality common to both non-exclusive and exclusive backups and for this feature we have to do some work common for both non-exclusive and exclusive backup which is to generate the symlink label file for non-exclusive backups and write the symlink label file for exclusive backups using that information. Doing this way seems right to me as we are already doing something like that for backup label file. Another possible way could be to write a new function in xlogutils.c to do the symlink label stuff and then use the same in xlog.c, I think that way we could avoid any new include in xlog.c. However for this we need to have include in xlogutils.c to make it aware of tablespaceinfo structure. > 4. In the same vein, publicly declaring a struct with a name as > generic as "tablespaceinfo" doesn't seem like a great idea, when > its usage is far from generic. > This is related to above point, we need to use this for both non-exclusive and exclusive backups and the work for exclusive backups is done outside of basebackup.c due to which we need to expose the same. Any other better idea to address points 3 and 4? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
