On 1st February 2014, MauMau Wrote:
> I reviewed the patch content. I find this fix useful.
>
> I'd like to suggest some code improvements. I'll apply and test the
> patch when I receive your reply.
Thanks for reviewing the patch.
> (1)
> I think it is appropriate to place find_my_abs_path() in path.c rather
> than
> exec.c. Please look at the comments at the beginning of those files.
> exec.c handles functions related to executables, while path.c handles
> general functions handling paths.
I have moved this function to path.c
> It's better to rename the function to follow the naming of other
> functions
> in path.c, something like get_absolute_path() or so. Unfortunately, we
> cannot use make_absolute_path() as the name because it is used in
> src/backend/utils/init/miscinit.c, which conflicts in the backend
> module.
Renamed the function as get_absolute_path.
> (2)
> In pg_ctl.c, dbpath can be better named as datadir, because it holds
> data
> directory location. dbpath is used to mean some different location in
> other
> source files.
Renamed as dataDir.
> (3)
> find_my_abs_path() had better not call make_native_path() because the
> role
> of this function should be to just return an absolute path. pg_ctl.c
> can
> instead call make_native_path() after find_my_abs_path().
Changed as per suggestion.
> (4)
> find_my_abs_path() should not call resolve_symlinks(). For reference,
> look
> at make_absolute_path() in src/backend/utils/init/miscinit.c and
> src/test/regress/pg_regress.c. I guess the reason is that if the user
> changed to the directory through a symbolic link, we should retain the
> symbolic link name.
Changed as per suggestion.
> (5)
> Change "file/path" in the comment of find_my_abs_path() to "path",
> because
> file is also a path.
Changed as per suggestion.
Please find the attached revised patch.
Thanks and Regards,
Kumar Rajeev Rastogi
