Re: [PATCH] Bug fix in initdb output

Hi,

I found a behaviour mismatch in initdb's output between Windows and Unix in
PostgreSQL. When we run initdb, it displays a command at the end of the
output which is used to run the server. It displays properly in Unix where
we can directly use the command to run the server. But in the case of
Windows, the command shown in initdb's output cannot be directly used to
run the server since Windows uses forward slashes in the path but the
command contains backward slashes.

Please refer to the sample output of initdb given below. The command
contains 2 paths.
1. pg_ctl path - This path is handled properly in Unix whereas in Windows,
this path contains backward slashes which is a Unix complaint and it wont
work in Windows.
2. data path - This path is handled properly in both Unix and Windows.

C:\Users\Administrator\Desktop\Nitin_PostgreSQL\postgresql_test\src\tools\msvc>install\bin\initdb.exe
> -D database\data
> The files belonging to this database system will be owned by user
> "Administrator".
> This user must also own the server process.
> The database cluster will be initialized with locale "English_United
> States.1252".
> The default database encoding has accordingly been set to "WIN1252".
> The default text search configuration will be set to "english".
> Data page checksums are disabled.
> creating directory database/data ... ok
> creating subdirectories ... ok
> selecting dynamic shared memory implementation ... windows
> selecting default max_connections ... 100
> selecting default shared_buffers ... 128MB
> selecting default time zone ... US/Pacific
> creating configuration files ... ok
> running bootstrap script ... ok
> performing post-bootstrap initialization ... ok
> syncing data to disk ... ok
> initdb: warning: enabling "trust" authentication for local connections
> You can change this by editing pg_hba.conf or using the option -A, or
> --auth-local and --auth-host, the next time you run initdb.
> Success. You can now start the database server using:
> install/bin/pg_ctl -D ^"database^\data^" -l logfile start

The problem in the code is canonicalize_path() function simplifies the path
and it also converts the native path to Unix style path while simplifying
the path. That is why the path shown in windows contains backslashes.
Please refer below piece of code for more information.

> /* Get directory specification used to start initdb ... */

strlcpy(pg_ctl_path, argv[0], sizeof(pg_ctl_path));
> canonicalize_path(pg_ctl_path);
> get_parent_directory(pg_ctl_path);
> /* ... and tag on pg_ctl instead */
> join_path_components(pg_ctl_path, pg_ctl_path, "pg_ctl");

In case of a data directory path, it uses the native path. Hence there is
no problem.

> /* path to pg_ctl, properly quoted */
> appendShellString(start_db_cmd, pg_ctl_path);
> /* add -D switch, with properly quoted data directory */
> appendPQExpBufferStr(start_db_cmd, " -D ");
> appendShellString(start_db_cmd, pgdata_native);

The solution to this issue is we need to convert the ' pg_ctl_path' to
native path before displaying. The issue is fixed in the patch attached.

Following is the sample output after fixing the issue.

Success. You can now start the database server using:
> ^"install^\bin^\pg^_ctl^" -D ^"database^\data^" -l logfile start

Now this path can be directly used to run the server in windows.

Please share your thoughts on this. If we go ahead with this change, then
we need to back-patch. I would be happy to create those patches.

Thanks and Regards,
Nitin Jadhav

On Mon, Mar 1, 2021 at 5:52 AM Nitin Jadhav <nitinjadhavpostgres(at)gmail(dot)com>
wrote:

>
>> Please share your thoughts on this. If we go ahead with this change,
> then we need to back-patch. I would be happy to create those patches.
>

A full path works, even with the slashes. The commiter will take care of
back-patching, if needed. As for HEAD at least, this LGTM.

Regards,

Juan José Santamaría Flecha

On 2021-Mar-01, Juan José Santamaría Flecha wrote:

> On Mon, Mar 1, 2021 at 5:52 AM Nitin Jadhav <nitinjadhavpostgres(at)gmail(dot)com>
> wrote:
>
> >
> >> Please share your thoughts on this. If we go ahead with this change,
> > then we need to back-patch. I would be happy to create those patches.
>
> A full path works, even with the slashes. The commiter will take care of
> back-patching, if needed. As for HEAD at least, this LGTM.

I don't get it. I thought the windows API accepted both forward slashes
and backslashes as path separators. Did you try the command and see it
fail?

--
Álvaro Herrera 39°49'30"S 73°17'W
"Entristecido, Wutra (canción de Las Barreras)
echa a Freyr a rodar
y a nosotros al mar"

El lun., 1 mar. 2021 19:16, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
escribió:

>
> I don't get it. I thought the windows API accepted both forward slashes
> and backslashes as path separators. Did you try the command and see it
> fail?
>

This is not a problem with the APi, but the shell. e.g. when using a CMD:

- This works:
c:\>c:\Windows\System32\notepad.exe
c:\>c:/Windows/System32/notepad.exe
c:\>/Windows/System32/notepad.exe

- This doesn't:
c:\>./Windows/System32/notepad.exe
'.' is not recognized as an internal or external command,
operable program or batch file.
c:\>Windows/System32/notepad.exe
'Windows' is not recognized as an internal or external command,
operable program or batch file.

Regards,

Juan José Santamaría Flecha

On 2021-Mar-01, Juan José Santamaría Flecha wrote:

> This is not a problem with the APi, but the shell. e.g. when using a CMD:
>
> - This works:
> c:\>c:\Windows\System32\notepad.exe
> c:\>c:/Windows/System32/notepad.exe
> c:\>/Windows/System32/notepad.exe
>
> - This doesn't:
> c:\>./Windows/System32/notepad.exe
> '.' is not recognized as an internal or external command,
> operable program or batch file.
> c:\>Windows/System32/notepad.exe
> 'Windows' is not recognized as an internal or external command,
> operable program or batch file.

Ah, so another way to fix it would be to make the path to pg_ctl be
absolute?

--
Álvaro Herrera 39°49'30"S 73°17'W

On Mon, Mar 1, 2021 at 7:50 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
wrote:

> On 2021-Mar-01, Juan José Santamaría Flecha wrote:
>
> > This is not a problem with the APi, but the shell. e.g. when using a CMD:
> >
> > - This works:
> > c:\>c:\Windows\System32\notepad.exe
> > c:\>c:/Windows/System32/notepad.exe
> > c:\>/Windows/System32/notepad.exe
> >
> > - This doesn't:
> > c:\>./Windows/System32/notepad.exe
> > '.' is not recognized as an internal or external command,
> > operable program or batch file.
> > c:\>Windows/System32/notepad.exe
> > 'Windows' is not recognized as an internal or external command,
> > operable program or batch file.
>
> Ah, so another way to fix it would be to make the path to pg_ctl be
> absolute?
>

Yes, that's right. If you call initdb with an absolute path you won't see a
problem.

Regards,

Juan José Santamaría Flecha

On 2021-Mar-01, Juan José Santamaría Flecha wrote:

> On Mon, Mar 1, 2021 at 7:50 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> wrote:

> > Ah, so another way to fix it would be to make the path to pg_ctl be
> > absolute?
>
> Yes, that's right. If you call initdb with an absolute path you won't see a
> problem.

So, is make_native_path a better fix than make_absolute_path? (I find
it pretty surprising that initdb shows a relative path, but maybe that's
just me.)

--
Álvaro Herrera 39°49'30"S 73°17'W
"How strange it is to find the words "Perl" and "saner" in such close
proximity, with no apparent sense of irony. I doubt that Larry himself
could have managed it." (ncm, http://lwn.net/Articles/174769/)

On Mon, Mar 1, 2021 at 9:09 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
wrote:

> On 2021-Mar-01, Juan José Santamaría Flecha wrote:
>
> > On Mon, Mar 1, 2021 at 7:50 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> > wrote:
>
> > > Ah, so another way to fix it would be to make the path to pg_ctl be
> > > absolute?
> >
> > Yes, that's right. If you call initdb with an absolute path you won't
> see a
> > problem.
>
> So, is make_native_path a better fix than make_absolute_path? (I find
> it pretty surprising that initdb shows a relative path, but maybe that's
> just me.)
>

Uhm, now that you point it out, an absolute path would make the message
more consistent and reusable.

Regards,

Juan José Santamaría Flecha

On 2021-Mar-01, Juan José Santamaría Flecha wrote:

> Uhm, now that you point it out, an absolute path would make the message
> more consistent and reusable.

Well. This code was introduced in a00c58314745, with discussion at
http://postgr.es/m/CAHeEsBeAe1FeBypT3E8R1ZVZU0e8xv3A-7BHg6bEOi=jZny2Uw@mail.gmail.com
which did not touch on the point of the pg_ctl path being relative or
absolute. The previous decision to use relative seems to have been made
here in commit ee814b4511ec, which was backed by this discussion
/message-id/flat/200411020134.52513.peter_e%40gmx.net

So I'm not sure that anybody would love me if I change it again to
absolute.

--
Álvaro Herrera Valdivia, Chile
Voy a acabar con todos los humanos / con los humanos yo acabaré
voy a acabar con todos (bis) / con todos los humanos acabaré ¡acabaré! (Bender)

On Mon, Mar 1, 2021 at 10:18 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
wrote:

> On 2021-Mar-01, Juan José Santamaría Flecha wrote:
>
> > Uhm, now that you point it out, an absolute path would make the message
> > more consistent and reusable.
>
> Well. This code was introduced in a00c58314745, with discussion at
>
> http://postgr.es/m/CAHeEsBeAe1FeBypT3E8R1ZVZU0e8xv3A-7BHg6bEOi=jZny2Uw@mail.gmail.com
> which did not touch on the point of the pg_ctl path being relative or
> absolute. The previous decision to use relative seems to have been made
> here in commit ee814b4511ec, which was backed by this discussion
>
> /message-id/flat/200411020134.52513.peter_e%40gmx.net
>
> So I'm not sure that anybody would love me if I change it again to
> absolute.
>

For me it is a +1 for the change to absolute. Let's see if more people want
to weigh in on the matter.

Regards,

Juan José Santamaría Flecha

On Tue, Mar 02, 2021 at 01:28:57AM +0100, Juan José Santamaría Flecha wrote:
> For me it is a +1 for the change to absolute. Let's see if more people want
> to weigh in on the matter.

FWIW, I don't think that it is a good idea to come back to this
decision for *nix platforms, so I would let it as-is, and use relative
paths if initdb is called using a relative path.

The number of people using a relative path for Windows initialization
sounds rather limited to me. However, if you can write something that
makes the path printed compatible for a WIN32 terminal while keeping
the behavior consistent with other platforms, people would have no
objections to that.
--
Michael

>
> FWIW, I don't think that it is a good idea to come back to this
> decision for *nix platforms, so I would let it as-is, and use relative
> paths if initdb is called using a relative path.

The command to be displayed either in absolute path or relative path
depends on the way the user is calling initdb. I agree with the above
comment to keep this behaviour as-is, that is if the initdb is called using
relative path, then it displays relative path otherwise absolute path.

> However, if you can write something that
> makes the path printed compatible for a WIN32 terminal while keeping
> the behavior consistent with other platforms, people would have no
> objections to that.

I feel the patch attached above handles this scenario.

Thanks and Regards,
Nitin Jadhav

On Tue, Mar 2, 2021 at 6:53 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Tue, Mar 02, 2021 at 01:28:57AM +0100, Juan José Santamaría Flecha
> wrote:
> > For me it is a +1 for the change to absolute. Let's see if more people
> want
> > to weigh in on the matter.
>
> FWIW, I don't think that it is a good idea to come back to this
> decision for *nix platforms, so I would let it as-is, and use relative
> paths if initdb is called using a relative path.
>
> The number of people using a relative path for Windows initialization
> sounds rather limited to me. However, if you can write something that
> makes the path printed compatible for a WIN32 terminal while keeping
> the behavior consistent with other platforms, people would have no
> objections to that.
> --
> Michael
>

On 2021-Mar-02, Nitin Jadhav wrote:

> > FWIW, I don't think that it is a good idea to come back to this
> > decision for *nix platforms, so I would let it as-is, and use
> > relative paths if initdb is called using a relative path.
>
> The command to be displayed either in absolute path or relative path
> depends on the way the user is calling initdb. I agree with the above
> comment to keep this behaviour as-is, that is if the initdb is called
> using relative path, then it displays relative path otherwise absolute
> path.

Yeah.

> > However, if you can write something that makes the path printed
> > compatible for a WIN32 terminal while keeping the behavior
> > consistent with other platforms, people would have no objections to
> > that.
>
> I feel the patch attached above handles this scenario.

Agreed. I'll push the original patch then. Thanks everybody for the
discussion.

--
Álvaro Herrera 39°49'30"S 73°17'W
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... ¿Quién es el machito que tendría carnet?" (Mafalda)

On 3/2/21 9:32 AM, Alvaro Herrera wrote:
> On 2021-Mar-02, Nitin Jadhav wrote:
>
>>> FWIW, I don't think that it is a good idea to come back to this
>>> decision for *nix platforms, so I would let it as-is, and use
>>> relative paths if initdb is called using a relative path.
>> The command to be displayed either in absolute path or relative path
>> depends on the way the user is calling initdb. I agree with the above
>> comment to keep this behaviour as-is, that is if the initdb is called
>> using relative path, then it displays relative path otherwise absolute
>> path.
> Yeah.
>
>>> However, if you can write something that makes the path printed
>>> compatible for a WIN32 terminal while keeping the behavior
>>> consistent with other platforms, people would have no objections to
>>> that.
>> I feel the patch attached above handles this scenario.
> Agreed. I'll push the original patch then. Thanks everybody for the
> discussion.
>

I'm late to the party on this which is my fault, I had my head down on
other stuff. But I just noticed this commit. Unfortunately the commit
message and this thread contain suggestions that aren't true. How do I
know? Because the buildfarm has for years called pg_ctl via cmd.exe with
a relative path with forward slashes. here's the relevant perl code that
runs on all platforms:

        chdir($installdir);
        my $cmd =
          qq{"bin/pg_ctl" -D data-$locale -l logfile -w start >startlog
2>&1};
        system($cmd);

Note that the pg_ctl path is quoted, and those quotes are passed through
to cmd.exe. That's what makes it work. It's possibly not worth changing
it now, but if anything that's the change that should have been made here.

Just wanted that on the record in case people got this wrong idea that
you can't use forward slashes when calling a program in cmd.exe.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sun, Mar 21, 2021 at 10:28 PM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

>
> Note that the pg_ctl path is quoted, and those quotes are passed through
> to cmd.exe. That's what makes it work. It's possibly not worth changing
> it now, but if anything that's the change that should have been made here.
>
> The OP claimed that the printed command was not working 'as-is', which is
a valid complaint.

Quoting the command seems like a complete answer for this, as it will solve
problems with spaces and such for both Windows and Unix-like systems.

Regards,

Juan José Santamaría Flecha

On 3/22/21 4:36 AM, Juan José Santamaría Flecha wrote:
>
> On Sun, Mar 21, 2021 at 10:28 PM Andrew Dunstan <andrew(at)dunslane(dot)net
> <mailto:andrew(at)dunslane(dot)net>> wrote:
>
>
> Note that the pg_ctl path is quoted, and those quotes are passed
> through
> to cmd.exe. That's what makes it work. It's possibly not worth
> changing
> it now, but if anything that's the change that should have been
> made here.
>
> The OP claimed that the printed command was not working 'as-is', which
> is a valid complaint.
>
> Quoting the command seems like a complete answer for this, as it will
> solve problems with spaces and such for both Windows and Unix-like
> systems.
>
>

Looking into this more closely, we are calling appendShellString() which
is designed to ensure that we call commands via system() cleanly and
securely. But we're not calling system() here. All we're doing is to
print a message. The caret-escaped message is horribly ugly IMNSHO. Can
we see if we can get something less ugly than this?:

Success. You can now start the database server using:

^"bin^\\pg^_ctl^" -D data-C -l logfile start

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com