Re: [PATCH] Bug fix in initdb output

[Andrew Dunstan]

On 3/22/21 4:36 AM, Juan José Santamaría Flecha wrote:
>
> On Sun, Mar 21, 2021 at 10:28 PM Andrew Dunstan  > 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

Re: [PATCH] Bug fix in initdb output

[Juan José Santamaría Flecha]

On Sun, Mar 21, 2021 at 10:28 PM Andrew Dunstan  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

Re: [PATCH] Bug fix in initdb output

[Andrew Dunstan]

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

Re: [PATCH] Bug fix in initdb output

[Alvaro Herrera]

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 Herrera39°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)

Re: [PATCH] Bug fix in initdb output

[Nitin Jadhav]

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

Re: [PATCH] Bug fix in initdb output

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: [PATCH] Bug fix in initdb output

[Juan José Santamaría Flecha]

On Mon, Mar 1, 2021 at 10:18 PM Alvaro Herrera 
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=jzny...@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
>
> https://www.postgresql.org/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

Re: [PATCH] Bug fix in initdb output

[Alvaro Herrera]

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=jzny...@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
https://www.postgresql.org/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)

Re: [PATCH] Bug fix in initdb output

[Juan José Santamaría Flecha]

On Mon, Mar 1, 2021 at 9:09 PM Alvaro Herrera 
wrote:

> On 2021-Mar-01, Juan José Santamaría Flecha wrote:
>
> > On Mon, Mar 1, 2021 at 7:50 PM Alvaro Herrera 
> > 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

Re: [PATCH] Bug fix in initdb output

[Alvaro Herrera]

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

> On Mon, Mar 1, 2021 at 7:50 PM Alvaro Herrera 
> 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 Herrera39°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/)

Re: [PATCH] Bug fix in initdb output

[Juan José Santamaría Flecha]

On Mon, Mar 1, 2021 at 7:50 PM Alvaro Herrera 
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

Re: [PATCH] Bug fix in initdb output

[Alvaro Herrera]

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 Herrera39°49'30"S 73°17'W

Re: [PATCH] Bug fix in initdb output

[Juan José Santamaría Flecha]

El lun., 1 mar. 2021 19:16, Alvaro Herrera 
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

Re: [PATCH] Bug fix in initdb output

[Alvaro Herrera]

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

> On Mon, Mar 1, 2021 at 5:52 AM Nitin Jadhav 
> 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 Herrera39°49'30"S 73°17'W
"Entristecido, Wutra (canción de Las Barreras)
echa a Freyr a rodar
y a nosotros al mar"

Re: [PATCH] Bug fix in initdb output

[Juan José Santamaría Flecha]

On Mon, Mar 1, 2021 at 5:52 AM Nitin Jadhav 
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