Subject: somewhat wrong archive_command example

From: Peter Eisentraut <[email-masked]>
Date: Tue, 20 Sep 2011 16:17:51 +0300

At
http://www.postgresql.org/docs/current/static/continuous-archiving.html
we say

"""
Many people choose to use scripts to define their archive_command, so
that their postgresql.conf entry looks very simple:

archive_command = 'local_backup_script.sh'
"""

It seems to me, however, that even a simple archive_command like that
ought to contain at least %p, right?

---

From: Magnus Hagander <[email-masked]>
Date: Tue, 20 Sep 2011 15:34:06 +0200

> ought to contain at least %p, right?

Should always need both %p and %f, no?

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

---

From: Fujii Masao <[email-masked]>
Date: Wed, 21 Sep 2011 10:58:31 +0900

> Should always need both %p and %f, no?

Yes unless the script extracts the file name from the path given as %p.

Regards,

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

---

From: Greg Smith <[email-masked]>
Date: Tue, 20 Sep 2011 22:45:09 -0400

> ought to contain at least %p, right?

Yes, you have to pass %p, and even though you can derive it from there 
it's easier for most if they get %f too.

That whole section is really problematic.  It says "Any messages written 
to stderr from the script will appear in the database server log", but 
on some platforms things printed to stdout appear there too.  And 
without an example, it really doesn't make a good case for why a 
separate script can be useful in all cases.  I've lost count of how many 
people I've seen hang themselves with impossible to giant single line 
archive_command setups, following the examples that are given; those 
blow up quite badly when things go wrong.

Attached is a working local_backup_script.sh that does the same basic 
thing as the "Standalone Hot Backups" example.  It includes lots of 
error checking, useful messages when it doesn't work like this:

Archive directory does not exist
LOG:  archive command failed with exit code 1
DETAIL:  The failed archive command was: ./local_backup_script.sh 
pg_xlog/00000001000000010000007D 00000001000000010000007D

And it takes advantage of the fact that scripts will be executed in 
$PGDATA, by putting all the paths it uses relative to it.

If I could get some general agreement this is a good direction to wander 
in, I'd be happy to wrap this into a full doc patch and submit it.  I've 
been meaning to do this for a while and just never got around to it.

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

---

From: Magnus Hagander <[email-masked]>
Date: Wed, 21 Sep 2011 07:02:43 +0200

> Yes unless the script extracts the file name from the path given as %p.

Do we actually guarantee that this will wok?

I know our current implementation does, but does the contract in the
API actually guarantee that we will not change this implementation?

---

From: Fujii Masao <[email-masked]>
Date: Wed, 21 Sep 2011 15:30:33 +0900

> API actually guarantee that we will not change this implementation?

There is no such a guarantee. I agree with that %f should also be given
at the same time.

Regards,

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 21 Sep 2011 16:58:56 +0300

> been meaning to do this for a while and just never got around to it. 

I can see a few problems with your script, as far as making it into a
generally reusable example:

- Uses bash.  Portable scripts shouldn't do this.

- Overwrites PATH variable for unrelated purpose.

- Path ($PATH) should be quoted everywhere.  (By contrast, you don't
need the quotes in PATH="$1".  Just saying ...)

- cp shouldn't be called with an absolute path.

I guess this only supports the point that even a small script can
contain lots of traps, so providing a robust example would be helpful.

---

From: Greg Smith <[email-masked]>
Date: Wed, 21 Sep 2011 19:02:37 -0400

> - cp shouldn't be called with an absolute path.

Most of these problems stemmed from the unnoticed fact that I'd made 
PATH the name of my variable.  The absolute path for cp for example?  
Added that because it couldn't seem to find cp in the PATH, which was 
driving me crazy for a while there...sigh.

Attached version fixes the late night brain fade errors.  I think the 
right thing to do next is to package this up into a doc update that 
corrects the errors in that section too; just clean the whole thing up 
while I'm poking at it.  I'll submit that over to the hackers list so 
that everyone can take a shot at correcting my shell code.

---

From: Euler Taveira de Oliveira <[email-masked]>
Date: Wed, 21 Sep 2011 21:59:43 -0300

On 21-09-2011 20:02, Greg Smith wrote:
> Attached version fixes the late night brain fade errors. I think the right
> thing to do next is to package this up into a doc update that corrects the
> errors in that section too; just clean the whole thing up while I'm poking at
> it. I'll submit that over to the hackers list so that everyone can take a shot
> at correcting my shell code.
>
Just a few comments:

(i) full path for ARCHIVE (say /path/to/archive) and backup_in_progress. 
Absolute paths are easier to check than relative ones;
(ii) is echo with unquoted string portable? I don't know but it confuses 
readability.

-- 
    Euler Taveira de Oliveira - Timbira       http://www.timbira.com.br/
    PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

---

From: Josh Kupershmidt <[email-masked]>
Date: Thu, 22 Sep 2011 14:15:30 -0400

>> at correcting my shell code.

First, a +1 on the impetus for this script.

Few more suggestions/nitpicks:
 1.) IMO it's more logical to put the test for whether the $ARCHIVE
directory exists before the test whether ${ARCHIVE}/${FILE} exists.
 2.) I think the error code reporting here is not sound:

cp ${FULLPATH} ${ARCHIVE}/${FILE}
if [ $? -ne 0 ] ; then
  echo $0 Archive copy of ${FILE} failed with error $? >&2

at least on my OS X machine, that echo produces a message like
"./local_backup_script.sh Archive copy of failed with error 0", I
guess since $? gets reset to 0 after that if-statement. You can use a
temporary variable like $ERRCODE=$? to get around this.

Josh

---

From: Euler Taveira de Oliveira <[email-masked]>
Date: Thu, 22 Sep 2011 15:41:31 -0300

> directory exists before the test whether ${ARCHIVE}/${FILE} exists.
No. If you do so, it will end up wasting a lot of cpu cycles testing something 
that is *always* true (if the directory exists). AFAICS this test is to handle 
a cp failure case nicely.

>    echo $0 Archive copy of ${FILE} failed with error $?>&2
>
Right.

---

From: Josh Kupershmidt <[email-masked]>
Date: Thu, 22 Sep 2011 15:29:25 -0400

On Thu, Sep 22, 2011 at 2:41 PM, Euler Taveira de Oliveira
<[email-masked]> wrote:
> No. If you do so, it will end up wasting a lot of cpu cycles testing
> something that is *always* true (if the directory exists). AFAICS this test
> is to handle a cp failure case nicely.

Maybe I misunderstand you.. I was talking about this test, which was
in Greg's script already:

if [ ! -d ${ARCHIVE} ] ; then
  echo Archive directory does not exist >&2
  exit 1
fi

I don't see how it would make any difference performance-wise whether
this block is moved up to right before the "if [ -f ${ARCHIVE}/${FILE}
] ; then" line: we expect both of these if-statements to evaluate
false if they are reached.

Josh

---

From: Euler Taveira de Oliveira <[email-masked]>
Date: Thu, 22 Sep 2011 16:58:25 -0300

On 22-09-2011 16:29, Josh Kupershmidt wrote:
>    echo Archive directory does not exist>&2
>    exit 1
> fi
>
No, I don't.

> false if they are reached.
If the archive directory exists, it will test at least 2 times per check 
otherwise just one time per check (unless there is a new wal file -- in this 
case 2 times per check). It is not necessary to move up the test. As I said, 
the directory check is only for predict a cp failure.

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 15 Aug 2012 21:13:47 -0400

> temporary variable like $ERRCODE=$? to get around this.

I have made all the suggestions posted and would like to add the
attached script to our documentation as a simple example.

-- 
  Bruce Momjian  <[email-masked]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

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

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 17 Aug 2012 15:51:44 -0400

> attached script to our documentation as a simple example.

Btw., is anyone else concerned about using plain cp for this?  If the cp 
fails half-way, it leaves a partial file around, but subsequent file 
existence checks will find the file OK and skip it.

I have occasionally used some combination of mktemp + cp + mv, which 
seems to work around this problem.

---

From: Euler Taveira <[email-masked]>
Date: Fri, 17 Aug 2012 17:29:54 -0300

On 17-08-2012 16:51, Peter Eisentraut wrote:
> Btw., is anyone else concerned about using plain cp for this?  If the cp fails
> half-way, it leaves a partial file around, but subsequent file existence
> checks will find the file OK and skip it.
> 
I've already faced this problem (with cp and scp).

> I have occasionally used some combination of mktemp + cp + mv, which seems to
> work around this problem.
> 
That's what rsync uses, no? Your suggestion or rsync seem safe options to me.
But if you insist on an academic example using cp, at least comment that it
can fail in some cases.

---

From: Bruce Momjian <[email-masked]>
Date: Sat, 25 Aug 2012 19:07:55 -0400

On Fri, Aug 17, 2012 at 03:51:44PM -0400, Peter Eisentraut wrote:
> Btw., is anyone else concerned about using plain cp for this?  If
> the cp fails half-way, it leaves a partial file around, but
> subsequent file existence checks will find the file OK and skip it.
> 

I am unclear why the script returns success if the file already exists
--- seems if the file exists, we should throw an error, like we have
always done with cp -i < /dev/null.

Updated version attached.

Another option in this case would be to re-issue the copy.

---

From: Bruce Momjian <[email-masked]>
Date: Sat, 25 Aug 2012 19:40:26 -0400

> Another option in this case would be to re-issue the copy.

Sorry, proper attached file this time.

---

From: Peter Eisentraut <[email-masked]>
Date: Sun, 26 Aug 2012 23:43:45 -0400

I'm a little bit worried about this whole discussion.  We have half a
dozen supposed experts working on a 10-line shell script to do WAL
archiving, and there is still

- disagreement
- uncertainty
- bugs

How is a normal user supposed to do this?

That aside, I disagree with the premise for this script.  It doesn't do
anything more than the typical inline examples we give.  The additional
error reporting could be considered useful, but cp is perfectly capable
of reporting these errors itself, and getting in the way of that just
loses information and introduces race conditions.  But if we are going
to provide a separate shell script, we should at least offer some
additional value, such as addressing the partial copy problem.  But I
think that's not very easy to do in portable shell.

I think what we should do is write a wrapper program in C that covers
all the bases, support all platforms, and supports a handful of the most
commonly used copy methods (cp, scp, etc. + compression).  Then everyone
can just use that and doesn't have to figure all these things out from
scratch every time.

---

From: Bruce Momjian <[email-masked]>
Date: Mon, 27 Aug 2012 09:46:41 -0400

> think that's not very easy to do in portable shell.

I thought the idea was just to show what a shell script might look like 
--- we could say it does the same as a single-line copy, but then people
could improve it, or we could add a comment to show how people could
improve it.

I am actually unclear how the partial copy problem would not be fixed by
just using cp without -i < /dev/null.

> can just use that and doesn't have to figure all these things out from

I doubt it would work because people like the flexibility of shell
scripts.

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 29 Aug 2012 21:57:28 -0400

> could improve it, or we could add a comment to show how people could

Why can't we provide the improved version to begin with, if we know it
needs improvement?  I think we don't really know what it should look
like, do we?

> just using cp without -i < /dev/null.

Sure, that would do it, but elsewhere we also say that the command
should guard against overwriting existing files.  That's a safety
measure against overwriting existing archives due to configuration
mistakes, which is fair enough.  So there is still something that needs
refining.  Either

- We don't care about the partial copy problem, or

- We don't care about the overwriting existing archives issue, or

- We make the copy quasi-atomic using mktemp + mv (could also use the
"install" program, I think), or

- We make the file names absolutely unique by adding the system ID or a
UUID of some kind.

> I doubt it would work because people like the flexibility of shell

Well, we thought that about the base backup, but now we have
pg_basebackup, which supports two output methods that should cover most
cases.  If we could provide an archive command that has two or three
methods, that could also cover most uses.  Of course, in either case you
are free to write your own script.

---

From: Magnus Hagander <[email-masked]>
Date: Thu, 30 Aug 2012 11:15:30 +0200

> needs improvement?  I think we don't really know what it should look

+1.

> are free to write your own script.

I would say the majority of people *don't* like the flexibility of
shellscripts. They want something simple to use. pg_receivexlog on 9.2
is going to help some of them (as a completement to pg_basebackup).
But also shipping something to use for archive_command that just
copies the files in a proper way would definitely help for usability.

As long as we keep the write-your-own-script-however-you-like method,
just like we did for pg_basebackup, doing this can only be an
improvement. And I don't see anybody suggesting we *remove* this
capability.

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 30 Aug 2012 09:11:01 -0400

> improvement. And I don't see anybody suggesting we *remove* this

The original idea for the shell script was just to show people an
example of what it might look like --- not to try to fix or change our
recommended behavior.  If there is no interest in showing an example, we
can just close this thread.

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 30 Aug 2012 23:52:54 -0400

> can just close this thread.

Patch withdrawn.