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

Hi Rajeev san,

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.

(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.

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.

(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.

(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().

(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.

(5)
Change "file/path" in the comment of find_my_abs_path() to "path", because
file is also a path.

Regards
MauMau

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

From: "Rajeev rastogi" <rajeev(dot)rastogi(at)huawei(dot)com>
> Please find the attached revised patch.

Thanks, your patch looks good. I confirmed the following:

* Applied cleanly to the latest source tree, and built on Linux and Windows
(with MSVC) without any warnings.

* Changed to D:\ and ran "pg_ctl register -N pg -D pgdata", and checked the
registry value ImagePath with regedit. It contained -D "D:\pgdata".

* Changed to D:\pgdata and ran "pg_ctl register -N pg -D ..\pgdata", and
checked the registry value ImagePath with regedit. It contained -D
"D:\pgdata".

Finally, please change the function description comment of
get_absolute_path() so that the style follows other function in path.c.
That is:

/*
* <function_name>
*
* Function description
*/

Then update the CommitFest entry with the latest patch and let me know of
that. Then, I'll change the patch status to ready for committer.

Regards
MauMau

On 3rd February 2014, MauMau Wrote:

> Thanks, your patch looks good. I confirmed the following:
>
> * Applied cleanly to the latest source tree, and built on Linux and
> Windows (with MSVC) without any warnings.
>
> * Changed to D:\ and ran "pg_ctl register -N pg -D pgdata", and checked
> the registry value ImagePath with regedit. It contained -D "D:\pgdata".
>
> * Changed to D:\pgdata and ran "pg_ctl register -N pg -D ..\pgdata",
> and checked the registry value ImagePath with regedit. It contained -D
> "D:\pgdata".

Thanks for reviewing and testing the patch.

> Finally, please change the function description comment of
> get_absolute_path() so that the style follows other function in path.c.
> That is:
>
> /*
> * <function_name>
> *
> * Function description
> */

I have modified the function description comment as per your suggestion.

> Then update the CommitFest entry with the latest patch and let me know
> of that. Then, I'll change the patch status to ready for committer.

I will update commitFest with the latest patch immediately after sending the mail.

Thanks and Regards,
Kumar Rajeev Rastogi

From: "Rajeev rastogi" <rajeev(dot)rastogi(at)huawei(dot)com>
> I will update commitFest with the latest patch immediately after sending
> the mail.

OK, done setting the status to ready for committer.

Regards
MauMau

On Mon, Feb 3, 2014 at 3:06 PM, MauMau <maumau307(at)gmail(dot)com> wrote:

> From: "Rajeev rastogi" <rajeev(dot)rastogi(at)huawei(dot)com>
>
> I will update commitFest with the latest patch immediately after sending
>> the mail.
>>
>
> OK, done setting the status to ready for committer.
>
>
We already have two different versions of make_absolute_path() in the tree
- one in src/backend/utils/init/miscinit.c and one in
src/test/regress/pg_regress.c.

I don't think we need a third one.

If we put it in port/ like this patch done, then we should make the other
callers use it. We might need one for frontend and one for backend (I
haven't looked into that much detail), but definitely the one between
pg_regress and pg_ctl should be shared.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

From: "Magnus Hagander" <magnus(at)hagander(dot)net>
> We already have two different versions of make_absolute_path() in the tree
> - one in src/backend/utils/init/miscinit.c and one in
> src/test/regress/pg_regress.c.
>
> I don't think we need a third one.
>
> If we put it in port/ like this patch done, then we should make the other
> callers use it. We might need one for frontend and one for backend (I
> haven't looked into that much detail), but definitely the one between
> pg_regress and pg_ctl should be shared.

Agreed. Sorry, I should have proposed the refactoring.

Rajeev, could you refactor the code so that there is only one
make_absolute_path()? I think we can do like this:

1. Delete get_absolute_path() in src/port/path.c.
2. Delete make_absolute_path() in src/test/regress/pg_regress.c.
3. Move make_absolute_path() from src/backend/utils/init/miscinit.c to
src/port/path.c, and delete the declaration in miscadmin.h.
4. Modify make_absolute_path() in path.c so that it can be used both in
backend and frontend. That is, surround the error message output with
#ifdef FRONTEND ... #else ... #endif. See some other source files in
src/port.

Regards
MauMau

On 18 February 2014 21:47, MauMau Wrote:

> > We already have two different versions of make_absolute_path() in the
> > tree
> > - one in src/backend/utils/init/miscinit.c and one in
> > src/test/regress/pg_regress.c.
> >
> > I don't think we need a third one.
> >
> > If we put it in port/ like this patch done, then we should make the
> > other callers use it. We might need one for frontend and one for
> > backend (I haven't looked into that much detail), but definitely the
> > one between pg_regress and pg_ctl should be shared.
>
> Agreed. Sorry, I should have proposed the refactoring.
>
> Rajeev, could you refactor the code so that there is only one
> make_absolute_path()? I think we can do like this:
>
> 1. Delete get_absolute_path() in src/port/path.c.
> 2. Delete make_absolute_path() in src/test/regress/pg_regress.c.
> 3. Move make_absolute_path() from src/backend/utils/init/miscinit.c to
> src/port/path.c, and delete the declaration in miscadmin.h.
> 4. Modify make_absolute_path() in path.c so that it can be used both in
> backend and frontend. That is, surround the error message output with
> #ifdef FRONTEND ... #else ... #endif. See some other source files in
> src/port.

Changed the patch as per your suggestion.
Now only one version of make_absolute_path there defined in src/port/path.c

Found one small memory leak in the existing function make_absolute_path(miscinit.c),
fixed the same.

Please find the revised patch.

Thanks and Regards,
Kumar Rajeev Rastogi

Hi Rajeev,

From: "Rajeev rastogi" <rajeev(dot)rastogi(at)huawei(dot)com>
Changed the patch as per your suggestion.
Now only one version of make_absolute_path there defined in src/port/path.c

Found one small memory leak in the existing function
make_absolute_path(miscinit.c),
fixed the same.

Thanks for refactoring. I confirmed that the revised patch applies to HEAD
cleanly, the source files built without extra warnings, and the original
intended problem was solved.

Please make small cosmetic changes so that make_absolute_path() follows the
style of other parts. Then I'll make this ready for committer.

(1)
Add the function name in the comment as in:

/*
* make_absolute_path
*
* ...existing function descripton
*/

(2)
Add errno description as in:

fprintf(stderr, _("could not get current working directory: %s\n",
strerror(errno)));

Regards
MauMau

On 22 February 2014 06:16, MauMau Wrote:

Thanks for reviewing again.

> Please make small cosmetic changes so that make_absolute_path() follows
> the
> style of other parts. Then I'll make this ready for committer.
>
> (1)
> Add the function name in the comment as in:
>
> /*
> * make_absolute_path
> *
> * ...existing function descripton
> */

Added.

> (2)
> Add errno description as in:
>
> fprintf(stderr, _("could not get current working directory: %s\n",
> strerror(errno)));

Modified.

Please find the attached modified patch.

Thanks and Regards,
Kumar Rajeev Rastogi

From: "Rajeev rastogi" <rajeev(dot)rastogi(at)huawei(dot)com>
Please find the attached modified patch.

Thanks, reviewed and made this patch ready for committer.

Regards
MauMau

While registering the service for PostgreSQL on windows, we cannot expect user to give absolute path always.
So it is required to support relative path as data directory. So this patch will be very useful to handle the same.

This patch has been in "Ready for committer" stage for long time.
Please check if this can be committed now or any other changes required.

Thanks and Regards,
Kumar Rajeev Rastogi

> -----Original Message-----
> From: MauMau [mailto:maumau307(at)gmail(dot)com]
> Sent: 24 February 2014 15:31
> To: Rajeev rastogi
> Cc: pgsql-hackers(at)postgresql(dot)org; Magnus Hagander
> Subject: Re: [HACKERS] [review] PostgreSQL Service on Windows does not
> start if data directory given is relative path
>
> From: "Rajeev rastogi" <rajeev(dot)rastogi(at)huawei(dot)com> Please find the
> attached modified patch.
>
> Thanks, reviewed and made this patch ready for committer.
>
> Regards
> MauMau

Rajeev rastogi <rajeev(dot)rastogi(at)huawei(dot)com> writes:
> [ pgctl_win32service_rel_dbpath_v6.patch ]

Committed with minor corrections, mostly but not all cosmetic.

regards, tom lane

On 05 April 2014 04:14, Tom Lane

> > [ pgctl_win32service_rel_dbpath_v6.patch ]
>
> Committed with minor corrections, mostly but not all cosmetic.

Thanks a lot...