Re: [review] PostgreSQL Service on Windows does not start if data directory given is relative path

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