Subject: confusing archive_command example

From: Josh Kupershmidt <[email-masked]>
Date: Mon, 29 Mar 2010 12:34:46 -0400

Hi,
On the page:
http://www.postgresql.org/docs/current/interactive/continuous-archiving.html#BACKUP-ARCHIVING-WAL

an example archive_command of:
archive_command = 'cp -i %p /mnt/server/archivedir/%f </dev/null'

is given. Then, a few lines later, an example archive command of:
archive_command = 'test ! -f .../%f && cp %p .../%f'

is given. I think this second command would be more clear if it used
"/mnt/server/archivedir/" instead of "...", to tie in with the
previous archive_command. It took me a minute to figure out that the
three dots were supposed to be an ellipsis instead of a typo for the
parent directory "..".

Josh

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 31 Mar 2010 19:39:21 -0400 (EDT)

> three dots were supposed to be an ellipsis instead of a typo for the

I agree our use of cp -i </dev/null is a little too fancy not to be
explained in the docs, so I have done so with the attached, applied
patch.

As far as "...", those are used to show only the important changes in
the string, which I think is the right approach, but I did change the
line so the dots are not right up against slashes:

	archive_command = 'test ! -f ... %f &amp;&amp; cp %p ... %f'

-- 
  Bruce Momjian  <[email-masked]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
Index: doc/src/sgml/backup.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/backup.sgml,v
retrieving revision 2.144
diff -c -c -r2.144 backup.sgml
*** doc/src/sgml/backup.sgml	22 Feb 2010 17:15:10 -0000	2.144
--- doc/src/sgml/backup.sgml	31 Mar 2010 23:34:20 -0000
***************
*** 604,614 ****
      directory).
      It is advisable to test your proposed archive command to ensure that it
      indeed does not overwrite an existing file, <emphasis>and that it returns
!     nonzero status in this case</>.  We have found that <literal>cp -i</> does
!     this correctly on some platforms but not others.  If the chosen command
!     does not itself handle this case correctly, you should add a command
!     to test for existence of the archive file.  For example, something
!     like:
  <programlisting>
  archive_command = 'test ! -f .../%f &amp;&amp; cp %p .../%f'
  </programlisting>
--- 604,615 ----
      directory).
      It is advisable to test your proposed archive command to ensure that it
      indeed does not overwrite an existing file, <emphasis>and that it returns
!     nonzero status in this case</>.  On many Unix platforms, <command>cp
!     -i</> causes copy to prompt before overwriting a file, and
!     <literal>&lt; /dev/null</> causes the prompt (and overwriting) to
!     fail.  If your platform does not support this behavior, you should
!     add a command to test for the existence of the archive file.  For
!     example, something like:
  <programlisting>
  archive_command = 'test ! -f .../%f &amp;&amp; cp %p .../%f'
  </programlisting>

---

From: Tom Lane <[email-masked]>
Date: Wed, 31 Mar 2010 19:44:15 -0400

> 	archive_command = 'test ! -f ... %f &amp;&amp; cp %p ... %f'

This is *not* an improvement, because it makes it look like the %f is a
separate command argument.  Please revert if you can't think of
something better.

			regards, tom lane

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 31 Mar 2010 19:45:16 -0400 (EDT)

> separate command argument.  Please revert if you can't think of

What about:

 	archive_command = 'test ! -f ...%f &amp;&amp; cp %p ...%f'

---

From: Tom Lane <[email-masked]>
Date: Wed, 31 Mar 2010 19:46:49 -0400

>  	archive_command = 'test ! -f ...%f &amp;&amp; cp %p ...%f'

Perhaps, though I still think this isn't an improvement over the
original.

			regards, tom lane

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 31 Mar 2010 19:51:14 -0400 (EDT)

> Perhaps, though I still think this isn't an improvement over the

His complaint was that .../%f looks like ../%f;  is that a valid
concern?  I have reverted the change.  Also, should we be using test !
-e instead of -f?

---

From: Tom Lane <[email-masked]>
Date: Wed, 31 Mar 2010 20:03:03 -0400

Bruce Momjian <[email-masked]> writes:
> concern?

Well, it does look like it, I'm just not seeing an easy fix that makes
that better.  I think the original suggestion was to turn it into a
concrete example by writing something like /mnt/archive/%f.

> I have reverted the change.  Also, should we be using test !
> -e instead of -f?

No opinion.

			regards, tom lane

---

From: "Greg Sabino Mullane" <[email-masked]>
Date: Thu, 1 Apr 2010 00:09:04 -0000

> archive_command = 'test ! -f ... %f &amp;&amp; cp %p ... %f'

The triple dots need to be removed entirely, too confusing. Write 
everything out.

- -- 
Greg Sabino Mullane [email-masked]
End Point Corporation http://www.endpoint.com/
PGP Key: 0x14964AC8 201003312007
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 31 Mar 2010 20:31:32 -0400 (EDT)

> > I have reverted the change.  Also, should we be using test !

Well -e tests for any type of file, while -f is only for regular files. 
In practice, there should only be regular files in the archive
directory.  But because we are always super-cautious, I changed it to
-e.

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 01 Apr 2010 08:25:24 +0300

On ons, 2010-03-31 at 19:51 -0400, Bruce Momjian wrote:
> Also, should we be using test ! -e instead of -f? 

test -e is not portable.

---

From: Greg Smith <[email-masked]>
Date: Thu, 01 Apr 2010 08:17:37 -0400

>> Also, should we be using test ! -e instead of -f? 

Right; there are plenty of shells where "test -e" isn't implemented, I 
believe some Solaris and/or HP-UX systems have that issue.  The original 
"test -f" was already the best example to give here.

As for the update Josh suggested, there are two issues here.  The PITR 
documentation at 
http://developer.postgresql.org/pgdocs/postgres/continuous-archiving.html 
starts with this example:

And then it used to show this one (now it has 'test -e'):

I agree this is confusing.  That second line should just read like this 
to match the other examples:

archive_command = 'test ! -f /mnt/server/archivedir/%f && cp %p 
/mnt/server/archivedir/%f'

And to revert the "test -e" change.

The third issue here, not mentioned yet, is that there's also an example 
of setting up archive_command in its GUC documentation:

http://developer.postgresql.org/pgdocs/postgres/runtime-config-wal.html

This has drifted out of sync with the section being updated here this 
week--its archive_command example isn't even using "-i".  I think the 
right thing to do here is to migrate the Windows example out of 18.5.3 
from 24.3.1, then replace the existing examples in 18.5.3 to instead 
suggest 24.3.1 has them.  Given that all of this documentation giving 
examples for how to correctly configure that parameter, like this cp 
trivia, only appears in the PITR docs section, I don't see any reason to 
duplicate that in the GUC area.  Having the second, minimally documented 
version of those in 18.5.3 just encourages people to use what we know is 
a bad practice by providing them as examples.

-- 
Greg Smith  2ndQuadrant US  Baltimore, MD
PostgreSQL Training, Services and Support
[email-masked]   www.2ndQuadrant.us

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 1 Apr 2010 09:52:57 -0400 (EDT)

> > Also, should we be using test ! -e instead of -f? 

Thanks, reverted

---

From: Bruce Momjian <[email-masked]>
Date: Mon, 12 Apr 2010 18:05:56 -0400 (EDT)

> a bad practice by providing them as examples.

Agreed.  The attached applied patch removes the example from the
configure section.

-- 
  Bruce Momjian  <[email-masked]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
Index: doc/src/sgml/config.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/config.sgml,v
retrieving revision 1.262
diff -c -c -r1.262 config.sgml
*** doc/src/sgml/config.sgml	3 Apr 2010 07:22:53 -0000	1.262
--- doc/src/sgml/config.sgml	12 Apr 2010 21:59:26 -0000
***************
*** 1721,1728 ****
          (The path name is relative to the working directory of the server,
          i.e., the cluster's data directory.)
          Use <literal>%%</> to embed an actual <literal>%</> character in the
!         command. For more information see <xref
!         linkend="backup-archiving-wal">.
          This parameter can only be set in the <filename>postgresql.conf</>
          file or on the server command line.  It is ignored unless
          <varname>archive_mode</> was enabled at server start.
--- 1721,1731 ----
          (The path name is relative to the working directory of the server,
          i.e., the cluster's data directory.)
          Use <literal>%%</> to embed an actual <literal>%</> character in the
!         command.  It is important for the command to return a zero
!         exit status only if it succeeds. For more information see
!         <xref linkend="backup-archiving-wal">.
!        </para>
!        <para>
          This parameter can only be set in the <filename>postgresql.conf</>
          file or on the server command line.  It is ignored unless
          <varname>archive_mode</> was enabled at server start.
***************
*** 1735,1748 ****
          archiving, but also breaks the chain of WAL files needed for
          archive recovery, so it should only be used in unusual circumstances.
         </para>
-        <para>
-         It is important for the command to return a zero exit status if
-         and only if it succeeds.  Examples:
- <programlisting>
- archive_command = 'cp "%p" /mnt/server/archivedir/"%f"'
- archive_command = 'copy "%p" "C:\\server\\archivedir\\%f"'  # Windows
- </programlisting>
-        </para>
        </listitem>
       </varlistentry>
       
--- 1738,1743 ----