Thread: Bug in WAL backup documentation

Bug in WAL backup documentation

From
Bernd Helmle
Date:
Our WAL backup documentation says in some parts of it:

..."%p is replaced by the absolute path of the file to archive..." [1]

I think this is (at least for 8.1 and upcoming 8.2 releases) wrong, since
the archiver
replaces this with pg_xlog/<LOGFILENAME> only, so that the archive command
is invoked with the relative path to the database data directory and its
xlog files. This
applies to the restore_command as well. Attached is a small patch against
HEAD, which
will adjust things in the documentation.

[1]
http://www.postgresql.org/docs/8.1/interactive/backup-online.html#BACKUP-ARCHIVING-WAL

--
  Thanks

                    Bernd

Attachment

Re: Bug in WAL backup documentation

From
Tom Lane
Date:
Bernd Helmle <mailings@oopsware.de> writes:
> Our WAL backup documentation says in some parts of it:

> ..."%p is replaced by the absolute path of the file to archive..." [1]

> I think this is (at least for 8.1 and upcoming 8.2 releases) wrong, since
> the archiver replaces this with pg_xlog/<LOGFILENAME> only,

Good point.  Do we want to consider that this is a code bug rather than
a doc bug?  The relative path is more efficient as long as the archiver
script doesn't do a "cd", but if it does then there'd be a problem.
You could argue that the code should be tweaked to continue supplying
an absolute path.

Since 8.1 has done this all along and no one's actually complained about
it, I guess no one is using scripts that do "cd".  I'm inclined to go
with Bernd's suggestion to change the docs to match the code, but does
anyone have a contrary opinion?

            regards, tom lane

Re: [HACKERS] Bug in WAL backup documentation

From
Martijn van Oosterhout
Date:
On Fri, Nov 03, 2006 at 11:25:09AM -0500, Tom Lane wrote:
> Since 8.1 has done this all along and no one's actually complained about
> it, I guess no one is using scripts that do "cd".  I'm inclined to go
> with Bernd's suggestion to change the docs to match the code, but does
> anyone have a contrary opinion?

Arguably you could give people a choice, say %P for the absolute path
and %p for the relative one. In Unix you can easily prepend $PWD to the
string, but I don't know how easy that is in Windows.

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Attachment

Re: [HACKERS] Bug in WAL backup documentation

From
"Florian G. Pflug"
Date:
Tom Lane wrote:
> Bernd Helmle <mailings@oopsware.de> writes:
> Since 8.1 has done this all along and no one's actually complained about
> it, I guess no one is using scripts that do "cd".  I'm inclined to go
> with Bernd's suggestion to change the docs to match the code, but does
> anyone have a contrary opinion?

I think supplying the absolute path makes archiving scripts less
error-prone, which is a good time. So I'd vote for absolute paths.

greetings, Florian Pflug

Re: [HACKERS] Bug in WAL backup documentation

From
"Simon Riggs"
Date:
On Fri, 2006-11-03 at 17:34 +0100, Martijn van Oosterhout wrote:
> On Fri, Nov 03, 2006 at 11:25:09AM -0500, Tom Lane wrote:
> > Since 8.1 has done this all along and no one's actually complained about
> > it, I guess no one is using scripts that do "cd".  I'm inclined to go
> > with Bernd's suggestion to change the docs to match the code, but does
> > anyone have a contrary opinion?

> In Unix you can easily prepend $PWD to the
> string, but I don't know how easy that is in Windows.

Windows input anyone?


Given the lack of a comprehensive test suite at this stage, I'd vote on
the side of least change right now. We know the existing mechanism
works, and as Martijn point out there is a workaround, plus as Tom
discusses this would only happen if people "cd" which in my book would
be bad programming form anyway.

+1 Doc bug for 8.2, feature request for 8.3, unless Windows bites.

--
  Simon Riggs
  EnterpriseDB   http://www.enterprisedb.com



Re: [HACKERS] Bug in WAL backup documentation

From
"Andrew Dunstan"
Date:
Simon Riggs wrote:
> On Fri, 2006-11-03 at 17:34 +0100, Martijn van Oosterhout wrote:
>> On Fri, Nov 03, 2006 at 11:25:09AM -0500, Tom Lane wrote:
>> > Since 8.1 has done this all along and no one's actually complained
>> about
>> > it, I guess no one is using scripts that do "cd".  I'm inclined to go
>> > with Bernd's suggestion to change the docs to match the code, but does
>> > anyone have a contrary opinion?
>
>> In Unix you can easily prepend $PWD to the
>> string, but I don't know how easy that is in Windows.
>
> Windows input anyone?
>


Of course you can get the current directory on Windows, if that's what the
question is.

cheers

andrew


Re: [HACKERS] Bug in WAL backup documentation

From
"Magnus Hagander"
Date:
> > > Since 8.1 has done this all along and no one's actually
> complained
> > > about it, I guess no one is using scripts that do "cd".  I'm
> > > inclined to go with Bernd's suggestion to change the docs
> to match
> > > the code, but does anyone have a contrary opinion?
>
> > In Unix you can easily prepend $PWD to the string, but I don't know
> > how easy that is in Windows.
>
> Windows input anyone?

%CD% gives the same as $PWD in a command shell:

C:\Program Files\Microsoft Visual Studio 8\VC>echo %CD%
C:\Program Files\Microsoft Visual Studio 8\VC


//Magnus

Re: [HACKERS] Bug in WAL backup documentation

From
Tom Lane
Date:
"Simon Riggs" <simon@2ndquadrant.com> writes:
>> On Fri, Nov 03, 2006 at 11:25:09AM -0500, Tom Lane wrote:
>>> Since 8.1 has done this all along and no one's actually complained about
>>> it, I guess no one is using scripts that do "cd".  I'm inclined to go
>>> with Bernd's suggestion to change the docs to match the code, but does
>>> anyone have a contrary opinion?

> +1 Doc bug for 8.2, feature request for 8.3, unless Windows bites.

Looking back in the archives, I note that one of the arguments for
making the server use relative paths everywhere was so that it'd be
robust against things like DBAs moving directories that contain live
postmasters.  If we provide a %P option, or otherwise encourage people
to write scripts that depend on the absolute path of $PGDATA, we'd lose
some of this robustness.  So that might be an argument for leaving the
code as-is indefinitely ... not a very strong argument maybe, but it's
more than just we're-too-lazy-to-add-%P.

Anyway, I've corrected the documentation in HEAD and 8.1.

            regards, tom lane

Re: [HACKERS] Bug in WAL backup documentation

From
"Simon Riggs"
Date:
On Sat, 2006-11-04 at 13:29 -0500, Tom Lane wrote:
> "Simon Riggs" <simon@2ndquadrant.com> writes:
> >> On Fri, Nov 03, 2006 at 11:25:09AM -0500, Tom Lane wrote:
> >>> Since 8.1 has done this all along and no one's actually complained about
> >>> it, I guess no one is using scripts that do "cd".  I'm inclined to go
> >>> with Bernd's suggestion to change the docs to match the code, but does
> >>> anyone have a contrary opinion?
>
> > +1 Doc bug for 8.2, feature request for 8.3, unless Windows bites.
>
> Looking back in the archives, I note that one of the arguments for
> making the server use relative paths everywhere was so that it'd be
> robust against things like DBAs moving directories that contain live
> postmasters.  If we provide a %P option, or otherwise encourage people
> to write scripts that depend on the absolute path of $PGDATA, we'd lose
> some of this robustness.  So that might be an argument for leaving the
> code as-is indefinitely ... not a very strong argument maybe, but it's
> more than just we're-too-lazy-to-add-%P.
>
> Anyway, I've corrected the documentation in HEAD and 8.1.

I think I can fulfil Bernd, Florian and Martijn's wishes by supplying an
additional substitutable parameter %d which is replaced by the DataDir.
This allows people to use an absolute directory if they wish, allows us
to continue with the functionality of %p as-is and all without a
possible confusion between %p and %P. It also allows %d to be used as an
identifier which might be used to locate the appropriate archive for
those with multiple servers without editing the archive_command for each
of those servers.

So using %d/%p will give you the absolute path for forward-slashers.
Works for archive and recovery.

Patch included, code and docs.
Code comments now discuss relative paths also.

Comments?

--
  Simon Riggs
  EnterpriseDB   http://www.enterprisedb.com


Attachment

Re: [HACKERS] Bug in WAL backup documentation

From
Tom Lane
Date:
"Simon Riggs" <simon@2ndquadrant.com> writes:
> On Sat, 2006-11-04 at 13:29 -0500, Tom Lane wrote:
>> Looking back in the archives, I note that one of the arguments for
>> making the server use relative paths everywhere was so that it'd be
>> robust against things like DBAs moving directories that contain live
>> postmasters.  If we provide a %P option, or otherwise encourage people
>> to write scripts that depend on the absolute path of $PGDATA, we'd lose
>> some of this robustness.

> I think I can fulfil Bernd, Florian and Martijn's wishes by supplying an
> additional substitutable parameter %d which is replaced by the DataDir.

This fails to respond to the concern that DataDir might be out-of-date.

            regards, tom lane

Re: [HACKERS] Bug in WAL backup documentation

From
"Simon Riggs"
Date:
On Sun, 2006-11-05 at 11:10 -0500, Tom Lane wrote:
> "Simon Riggs" <simon@2ndquadrant.com> writes:
> > On Sat, 2006-11-04 at 13:29 -0500, Tom Lane wrote:
> >> Looking back in the archives, I note that one of the arguments for
> >> making the server use relative paths everywhere was so that it'd be
> >> robust against things like DBAs moving directories that contain live
> >> postmasters.  If we provide a %P option, or otherwise encourage people
> >> to write scripts that depend on the absolute path of $PGDATA, we'd lose
> >> some of this robustness.
>
> > I think I can fulfil Bernd, Florian and Martijn's wishes by supplying an
> > additional substitutable parameter %d which is replaced by the DataDir.
>
> This fails to respond to the concern that DataDir might be out-of-date.

I'm not suggesting that the option is necessary, but I am suggesting
offering it to those who consider it useful.

Let's allow it, but document the concern about its use in certain
circumstances.

I'm pretty sure most people don't move live postmasters very frequently,
plus it isn't clear to me why we should support the people that want
that to do that, yet not the people who want the absolute-path option.

--
  Simon Riggs
  EnterpriseDB   http://www.enterprisedb.com



Re: [HACKERS] Bug in WAL backup documentation

From
Tom Lane
Date:
"Simon Riggs" <simon@2ndquadrant.com> writes:
> I'm pretty sure most people don't move live postmasters very frequently,
> plus it isn't clear to me why we should support the people that want
> that to do that, yet not the people who want the absolute-path option.

As already discussed upthread, anyone who wants the path can get it from
`pwd` or local equivalent --- and that mechanism is robust (as long as
the directory move doesn't happen while any particular instance of the
script is running).  I don't see why we should go out of our way to
provide a bad substitute for pwd.

BTW, I note that some post-startup uses of DataDir have crept back in,
in places like utils/adt/dbsize.c.  I'll be sure to clean those up
before release...

            regards, tom lane

Re: [HACKERS] Bug in WAL backup documentation

From
"Simon Riggs"
Date:
On Sun, 2006-11-05 at 11:49 -0500, Tom Lane wrote:

> I don't see why we should go out of our way to
> provide a bad substitute for pwd.

That argument is conclusive. Agreed.

--
  Simon Riggs
  EnterpriseDB   http://www.enterprisedb.com



Re: [HACKERS] Bug in WAL backup documentation

From
"Simon Riggs"
Date:
On Sun, 2006-11-05 at 15:02 +0000, Simon Riggs wrote:

> Code comments now discuss relative paths also.

Patch containing just the minor cleanup of docs and code comments.

--
  Simon Riggs
  EnterpriseDB   http://www.enterprisedb.com


Attachment

Re: Bug in WAL backup documentation

From
Bruno Wolff III
Date:
On Sun, Nov 05, 2006 at 11:49:36 -0500,
  Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> As already discussed upthread, anyone who wants the path can get it from
> `pwd` or local equivalent --- and that mechanism is robust (as long as
> the directory move doesn't happen while any particular instance of the
> script is running).  I don't see why we should go out of our way to
> provide a bad substitute for pwd.

I think you also still need read access to the intervening directories.
If the command works by walking up and matching inode numbers with names,
then it will break if it can't read the names. (For example /bin/pwd
breaks when it can't read a parent directories filenames.)

Re: [HACKERS] Bug in WAL backup documentation

From
Martijn van Oosterhout
Date:
On Tue, Nov 07, 2006 at 07:11:35PM -0600, Bruno Wolff III wrote:
> On Sun, Nov 05, 2006 at 11:49:36 -0500,
>   Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > As already discussed upthread, anyone who wants the path can get it from
> > `pwd` or local equivalent --- and that mechanism is robust (as long as
> > the directory move doesn't happen while any particular instance of the
> > script is running).  I don't see why we should go out of our way to
> > provide a bad substitute for pwd.
>
> I think you also still need read access to the intervening directories.
> If the command works by walking up and matching inode numbers with names,
> then it will break if it can't read the names. (For example /bin/pwd
> breaks when it can't read a parent directories filenames.)

That's system dependant though, Linux getcwd doesn't have that problem
for example. Should probably dig up some documention on which systems
would be affected by this.

Have a ncie day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Attachment

Re: [HACKERS] Bug in WAL backup documentation

From
Tom Lane
Date:
"Simon Riggs" <simon@2ndquadrant.com> writes:
>> Code comments now discuss relative paths also.

> Patch containing just the minor cleanup of docs and code comments.

Applied, thanks.

            regards, tom lane