Re: Re: [COMMITTERS] pgsql: Don't use "cp -i" in the example WAL archive_command.

Don't use "cp -i" in the example WAL archive_command.

This is a dangerous example to provide because on machines with GNU cp,
it will silently do the wrong thing and risk archive corruption. Worse,
during the 9.0 cycle somebody "improved" the discussion by removing the
warning that used to be there about that, and instead leaving the
impression that the command would work as desired on most Unixen.
It doesn't. Try to rectify the damage by providing an example that is safe
most everywhere, and then noting that you can try cp -i if you want but
you'd better test that.

In back-patching this to all supported branches, I also added an example
command for Windows, which wasn't provided before 9.0.

Branch
------
REL8_3_STABLE

Details
-------
http://git.postgresql.org/pg/commitdiff/23843d242f00e6597af91d4f4d08b655b2b362ba

Modified Files
--------------
doc/src/sgml/backup.sgml | 25 ++++++++++++++-----------
1 files changed, 14 insertions(+), 11 deletions(-)

Wow, this is the first I am hearing GNU cp -i can return zero exit if it
doesn't do the copy. I tested this on Ubuntu 10.04 using cp 7.4 and
got:

$ touch x y
$ cp -i x y; echo $?
cp: overwrite `y'? n
0

I see the same on my anchent BSD/OS machine too:

$ touch x y
$ cp -i x y; echo $?
overwrite y? n
0

Were we expecting an error if the file already existed? Assuming that,
we should assume the file will always exist so basically archiving will
never progress. Is this what we want? I just wasn't aware we were
expecting an already-existing this to be an error --- I thought we just
didn't want to overwrite it.

---------------------------------------------------------------------------

Tom Lane wrote:
> Don't use "cp -i" in the example WAL archive_command.
>
> This is a dangerous example to provide because on machines with GNU cp,
> it will silently do the wrong thing and risk archive corruption. Worse,
> during the 9.0 cycle somebody "improved" the discussion by removing the
> warning that used to be there about that, and instead leaving the
> impression that the command would work as desired on most Unixen.
> It doesn't. Try to rectify the damage by providing an example that is safe
> most everywhere, and then noting that you can try cp -i if you want but
> you'd better test that.
>
> In back-patching this to all supported branches, I also added an example
> command for Windows, which wasn't provided before 9.0.
>
> Branch
> ------
> REL8_3_STABLE
>
> Details
> -------
> http://git.postgresql.org/pg/commitdiff/23843d242f00e6597af91d4f4d08b655b2b362ba
>
> Modified Files
> --------------
> doc/src/sgml/backup.sgml | 25 ++++++++++++++-----------
> 1 files changed, 14 insertions(+), 11 deletions(-)
>
>
> --
> Sent via pgsql-committers mailing list (pgsql-committers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-committers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
>
> Wow, this is the first I am hearing GNU cp -i can return zero exit if it
> doesn't do the copy. I tested this on Ubuntu 10.04 using cp 7.4 and
> got:
>
> $ touch x y
> $ cp -i x y; echo $?
> cp: overwrite `y'? n
> 0
>
> I see the same on my anchent BSD/OS machine too:
>
> $ touch x y
> $ cp -i x y; echo $?
> overwrite y? n
> 0
>
> Were we expecting an error if the file already existed? Assuming that,
> we should assume the file will always exist so basically archiving will
> never progress. Is this what we want? I just wasn't aware we were
> expecting an already-existing this to be an error --- I thought we just
> didn't want to overwrite it.

I tested on FreeBSD 7.4 and saw a 1 error return:

$ touch x y
$ cp -i x y; echo $?
overwrite y? (y/n [n]) n
not overwritten
1

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 18 June 2011 04:13, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Bruce Momjian wrote:
>>
>> Wow, this is the first I am hearing GNU cp -i can return zero exit if it
>> doesn't do the copy.  I tested this on Ubuntu 10.04 using cp 7.4 and
>> got:
>>
>>       $ touch x y
>>       $ cp -i x y; echo $?
>>       cp: overwrite `y'? n
>>       0
>>
>> I see the same on my anchent BSD/OS machine too:
>>
>>       $ touch x y
>>       $ cp -i x y; echo $?
>>       overwrite y? n
>>       0
>>
>> Were we expecting an error if the file already existed?  Assuming that,
>> we should assume the file will always exist so basically archiving will
>> never progress.  Is this what we want?  I just wasn't aware we were
>> expecting an already-existing this to be an error --- I thought we just
>> didn't want to overwrite it.
>
> I tested on FreeBSD 7.4 and saw a 1 error return:
>
>        $ touch x y
>        $ cp -i x y; echo $?
>        overwrite y? (y/n [n]) n
>        not overwritten
>        1

And on a Mac (so through Darwin 10.7.0 a BSD version too):

toucan:tmp thom$ touch x y
toucan:tmp thom$ cp -i x y; echo $?
overwrite y? (y/n [n]) n
not overwritten
1

Thom

2011/6/18 Thom Brown <thom(at)linux(dot)com>:
[.. cut ..]
> And on a Mac (so through Darwin 10.7.0 a BSD version too):
>
> toucan:tmp thom$ touch x y
> toucan:tmp thom$ cp -i x y; echo $?
> overwrite y? (y/n [n]) n
> not overwritten
> 1

On AIX 5.3

bash-3.00$ touch x y
bash-3.00$ cp -i x y; echo $?
overwrite y? n
0

--
Dickson S. Guedes
mail/xmpp: guedes(at)guedesoft(dot)net - skype: guediz
http://guedesoft.net - http://www.postgresql.org.br

Thom Brown <thom(at)linux(dot)com> writes:
> On 18 June 2011 04:13, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> I tested on FreeBSD 7.4 and saw a 1 error return:

> And on a Mac (so through Darwin 10.7.0 a BSD version too):

Yeah, see yesterday's discussion on pgsql-admin. I think the behavior
with the error return may be a BSD-ism. In any case, GNU cp does *not*
do what we want, and that accounts for a sufficiently large fraction of
machines in the field that I think it's just unsafe to suggest using
"cp -i" so prominently. There are too many people who'll just copy and
paste the first example provided, especially if the warning to test it
is buried several paragraphs later.

regards, tom lane

Tom Lane wrote:
> Thom Brown <thom(at)linux(dot)com> writes:
> > On 18 June 2011 04:13, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >> I tested on FreeBSD 7.4 and saw a 1 error return:
>
> > And on a Mac (so through Darwin 10.7.0 a BSD version too):
>
> Yeah, see yesterday's discussion on pgsql-admin. I think the behavior
> with the error return may be a BSD-ism. In any case, GNU cp does *not*
> do what we want, and that accounts for a sufficiently large fraction of
> machines in the field that I think it's just unsafe to suggest using
> "cp -i" so prominently. There are too many people who'll just copy and
> paste the first example provided, especially if the warning to test it
> is buried several paragraphs later.

Agreed. Even if we could decide whether we want an existing file to
cause cp to fail or succeed, the bigger problem is that 'test ! -f $FILE
&& cp' and 'cp -i' often don't do the same thing, to the point where it
doesn't even seem worth mentioning the idea of using 'cp -i' at all.

I frankly don't think most users are competent enough to be able to test
their cp -i command, end even if they are, that script might migrate to
a machine that handles cp -i differently.

I think we should just document the test ! -f version and be done with
it, and maybe mention cp -i as non-portable.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 06/18/2011 09:19 AM, Tom Lane wrote:
> Thom Brown<thom(at)linux(dot)com> writes:
>> On 18 June 2011 04:13, Bruce Momjian<bruce(at)momjian(dot)us> wrote:
>>> I tested on FreeBSD 7.4 and saw a 1 error return:
>> And on a Mac (so through Darwin 10.7.0 a BSD version too):
> Yeah, see yesterday's discussion on pgsql-admin. I think the behavior
> with the error return may be a BSD-ism. In any case, GNU cp does *not*
> do what we want, and that accounts for a sufficiently large fraction of
> machines in the field that I think it's just unsafe to suggest using
> "cp -i" so prominently. There are too many people who'll just copy and
> paste the first example provided, especially if the warning to test it
> is buried several paragraphs later.
>
>

Yeah, I'm glad to see this go anyway. I always thought the example was
too cute by half. Using an explicit test is much nicer and more obvious.

cheers

andrew

Andrew Dunstan wrote:
>
>
> On 06/18/2011 09:19 AM, Tom Lane wrote:
> > Thom Brown<thom(at)linux(dot)com> writes:
> >> On 18 June 2011 04:13, Bruce Momjian<bruce(at)momjian(dot)us> wrote:
> >>> I tested on FreeBSD 7.4 and saw a 1 error return:
> >> And on a Mac (so through Darwin 10.7.0 a BSD version too):
> > Yeah, see yesterday's discussion on pgsql-admin. I think the behavior
> > with the error return may be a BSD-ism. In any case, GNU cp does *not*
> > do what we want, and that accounts for a sufficiently large fraction of
> > machines in the field that I think it's just unsafe to suggest using
> > "cp -i" so prominently. There are too many people who'll just copy and
> > paste the first example provided, especially if the warning to test it
> > is buried several paragraphs later.
> >
> >
>
> Yeah, I'm glad to see this go anyway. I always thought the example was
> too cute by half. Using an explicit test is much nicer and more obvious.

I think the only real risk to the 'test' example is the possibility that
they will mistype the pathname in one of the two places it is required,
or forget to change both of them at the same time.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> Andrew Dunstan wrote:
> >
> >
> > On 06/18/2011 09:19 AM, Tom Lane wrote:
> > > Thom Brown<thom(at)linux(dot)com> writes:
> > >> On 18 June 2011 04:13, Bruce Momjian<bruce(at)momjian(dot)us> wrote:
> > >>> I tested on FreeBSD 7.4 and saw a 1 error return:
> > >> And on a Mac (so through Darwin 10.7.0 a BSD version too):
> > > Yeah, see yesterday's discussion on pgsql-admin. I think the behavior
> > > with the error return may be a BSD-ism. In any case, GNU cp does *not*
> > > do what we want, and that accounts for a sufficiently large fraction of
> > > machines in the field that I think it's just unsafe to suggest using
> > > "cp -i" so prominently. There are too many people who'll just copy and
> > > paste the first example provided, especially if the warning to test it
> > > is buried several paragraphs later.
> > >
> > >
> >
> > Yeah, I'm glad to see this go anyway. I always thought the example was
> > too cute by half. Using an explicit test is much nicer and more obvious.
>
> I think the only real risk to the 'test' example is the possibility that
> they will mistype the pathname in one of the two places it is required,
> or forget to change both of them at the same time.

Perhaps we should recommend:

cd /path && test ! -f %f && cp %p %f

That is shorter and removes the duplicate problem.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Perhaps we should recommend:
> cd /path && test ! -f %f && cp %p %f
> That is shorter and removes the duplicate problem.

Um ... %p is relative to the database directory.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Perhaps we should recommend:
> > cd /path && test ! -f %f && cp %p %f
> > That is shorter and removes the duplicate problem.
>
> Um ... %p is relative to the database directory.

Oh, I see now. I had thought it was an absolute path, but good thing it
isn't because of the possible need for quoting characters in the path
name.

The only other idea I have is:

NEW=/path && test ! -f $NEW/%f && cp %p $NEW/%f

but that is not going to work with csh-based shells, while I think the
original is fine.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 06/19/2011 08:00 AM, Bruce Momjian wrote:
> Tom Lane wrote:
>> Bruce Momjian<bruce(at)momjian(dot)us> writes:
>>> Perhaps we should recommend:
>>> cd /path&& test ! -f %f&& cp %p %f
>>> That is shorter and removes the duplicate problem.
>> Um ... %p is relative to the database directory.
> Oh, I see now. I had thought it was an absolute path, but good thing it
> isn't because of the possible need for quoting characters in the path
> name.
>
> The only other idea I have is:
>
> NEW=/path&& test ! -f $NEW/%f&& cp %p $NEW/%f
>
> but that is not going to work with csh-based shells, while I think the
> original is fine.

Isn't this invoked via system()? AFAIK that should always invoke a
Bourne shell or equivalent on Unix. But in any case, I think you're
trying to solve a problem that doesn't really exist.

cheers

andrew

On Sat, Jun 18, 2011 at 10:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thom Brown <thom(at)linux(dot)com> writes:
>> On 18 June 2011 04:13, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>>> I tested on FreeBSD 7.4 and saw a 1 error return:
>
>> And on a Mac (so through Darwin 10.7.0 a BSD version too):
>
> Yeah, see yesterday's discussion on pgsql-admin.  I think the behavior
> with the error return may be a BSD-ism.  In any case, GNU cp does *not*
> do what we want, and that accounts for a sufficiently large fraction of
> machines in the field that I think it's just unsafe to suggest using
> "cp -i" so prominently.  There are too many people who'll just copy and
> paste the first example provided, especially if the warning to test it
> is buried several paragraphs later.

Anyway, you seem to have forgotten to fix the example of archive_command
in "24.3.5.1. Standalone Hot Backups".

archive_command = 'test ! -f /var/lib/pgsql/backup_in_progress ||
cp -i %p /var/lib/pgsql/archive/%f < /dev/null'

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> Anyway, you seem to have forgotten to fix the example of archive_command
> in "24.3.5.1. Standalone Hot Backups".

> archive_command = 'test ! -f /var/lib/pgsql/backup_in_progress ||
> cp -i %p /var/lib/pgsql/archive/%f < /dev/null'

You're right, I didn't see that one. Fixed, thanks.

regards, tom lane