Andrew Dunstan <andrew@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.
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 (and if
we do need it, doesn't it need to be documented in protocol.sgml)?
And why is the RelationCacheInitFileRemove call relocated?
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.
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.
regards, tom lane
