Re: pg_standby -l might destory the archived file

Hi,

pg_standby can use ln command to restore an archived file,
which might destroy the archived file as follows.

1) pg_standby creates the symlink to the archived file '102'
2) '102' is applied
3) the next file '103' doesn't exist and the trigger file is created
4) '102' is re-fetched
5) at the end of recovery, the symlink to '102' is rename to '202',
but it still points '102'
6) after recovery, '202' is recycled (rename to '208', which still
points '102')
7) '208' is written new xlog records over
--> the archived file '102' comes down!

One simple solution to fix this problem is copying the content
of the symlink (ie. the archived file itself) and deleting it instead
of renaming it at the end of recovery.

Thought?

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

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> pg_standby can use ln command to restore an archived file,
> which might destroy the archived file as follows.

Does it matter? pg_standby's source area wouldn't normally be an
"archive" in the real sense of the word, it's just a temporary staging
area between master and slave. (If it were being used as a real
archive, keeping it on the same disk as the live slave seems pretty
foolish anyway, so the case wouldn't arise.)

regards, tom lane

Tom Lane wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>> pg_standby can use ln command to restore an archived file,
>> which might destroy the archived file as follows.
>
> Does it matter? pg_standby's source area wouldn't normally be an
> "archive" in the real sense of the word, it's just a temporary staging
> area between master and slave. (If it were being used as a real
> archive, keeping it on the same disk as the live slave seems pretty
> foolish anyway, so the case wouldn't arise.)

It seems perfectly sane to source pg_standby directly from the archive
to me. And we're talking about symbolic linking, so the archive
directory might well be on an NFS mount.

(I haven't looked at the issue yet)

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

* Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> [090601 10:56]:
> Tom Lane wrote:
>> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>>> pg_standby can use ln command to restore an archived file,
>>> which might destroy the archived file as follows.
>>
>> Does it matter? pg_standby's source area wouldn't normally be an
>> "archive" in the real sense of the word, it's just a temporary staging
>> area between master and slave. (If it were being used as a real
>> archive, keeping it on the same disk as the live slave seems pretty
>> foolish anyway, so the case wouldn't arise.)
>
> It seems perfectly sane to source pg_standby directly from the archive
> to me. And we're talking about symbolic linking, so the archive
> directory might well be on an NFS mount.

I would expect that any archive directly available would at least be RO
to the postgres slave... But....

Something like this would stop the "symlink" being renamed... Not portable, but probably portable
across platforms that have symlinks...
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1b575e2..cba3f7a 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3042,13 +3042,23 @@ RemoveOldXlogFiles(uint32 log, uint32 seg, XLogRecPtr endptr)
{
if (XLogArchiveCheckDone(xlde->d_name))
{
+ struct stat stat_buf;
snprintf(path, MAXPGPATH, XLOGDIR "/%s", xlde->d_name);

/*
* Before deleting the file, see if it can be recycled as a
* future log segment.
+ * If it's a symlink, we don't recycle it
*/
- if (InstallXLogFileSegment(&endlogId, &endlogSeg, path,
+ if ( (stat(path, &stat_buf) == 0) && S_ISLNK(stat_buf.st_mode))
+ {
+ ereport(DEBUG2,
+ (errmsg("removing transaction log symlink \"%s\"",
+ xlde->d_name)));
+ unlink(path);
+ CheckpointStats.ckpt_segs_removed++;
+ }
+ else if (InstallXLogFileSegment(&endlogId, &endlogSeg, path,
true, &max_advance,
true))
{

You can make a smaller patch if your not interested in the DEBUG2 message
saying that it deleted a symlink...

--
Aidan Van Dyk Create like a god,
aidan(at)highrise(dot)ca command like a king,
http://www.highrise.ca/ work like a slave.

Aidan Van Dyk wrote:
> * Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> [090601 10:56]:
>> Tom Lane wrote:
>>> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>>>> pg_standby can use ln command to restore an archived file,
>>>> which might destroy the archived file as follows.
>>> Does it matter? pg_standby's source area wouldn't normally be an
>>> "archive" in the real sense of the word, it's just a temporary staging
>>> area between master and slave. (If it were being used as a real
>>> archive, keeping it on the same disk as the live slave seems pretty
>>> foolish anyway, so the case wouldn't arise.)
>> It seems perfectly sane to source pg_standby directly from the archive
>> to me. And we're talking about symbolic linking, so the archive
>> directory might well be on an NFS mount.
>
> I would expect that any archive directly available would at least be RO
> to the postgres slave... But....

Me too.

I wonder if we should just remove the symlink option from pg_standby.
Does anyone use it? Is there a meaningful performance difference?

> Something like this would stop the "symlink" being renamed... Not portable, but probably portable
> across platforms that have symlinks...
> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c

That seems reasonable as well.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> I wonder if we should just remove the symlink option from pg_standby.

I was considering suggesting that too...

regards, tom lane

Hi,

On Mon, Jun 1, 2009 at 11:41 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>> pg_standby can use ln command to restore an archived file,
>> which might destroy the archived file as follows.
>
> Does it matter?  pg_standby's source area wouldn't normally be an
> "archive" in the real sense of the word, it's just a temporary staging
> area between master and slave.

If so, it might be deleted after triggering the warm-standby. But, we cannot
remove it because the latest xlog file which is required for normal recovery
might exist in it. This is another undesirable scenario. Is this problem?

Regards,

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

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> If so, it might be deleted after triggering the warm-standby. But, we cannot
> remove it because the latest xlog file which is required for normal recovery
> might exist in it. This is another undesirable scenario. Is this problem?

What recovery? In the problem case you're positing, the slave server
has executed a checkpoint and come up live. It's never going to be
interested in the old xlog again.

regards, tom lane

Hi,

On Tue, Jun 2, 2009 at 10:00 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>> If so, it might be deleted after triggering the warm-standby. But, we cannot
>> remove it because the latest xlog file which is required for normal recovery
>> might exist in it. This is another undesirable scenario. Is this problem?
>
> What recovery?  In the problem case you're positing, the slave server
> has executed a checkpoint and come up live.  It's never going to be
> interested in the old xlog again.

Yes, the old xlog itself is not used again. But, the *old file* might
be recycled
and used later. The case that I'm looking at is that the symlink to a temporary
area is recycled. Am I missing something?

Regards,

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

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> Yes, the old xlog itself is not used again. But, the *old file* might
> be recycled and used later. The case that I'm looking at is that the
> symlink to a temporary area is recycled. Am I missing something?

Actually, I think the right fix for that would be to add defenses to
xlog.c to not try to "recycle" a file that is a symlink. No good could
possibly come of that.

regards, tom lane

Hi,

On Tue, Jun 2, 2009 at 10:21 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>> Yes, the old xlog itself is not used again. But, the *old file* might
>> be recycled and used later. The case that I'm looking at is that the
>> symlink to a temporary area is recycled. Am I missing something?
>
> Actually, I think the right fix for that would be to add defenses to
> xlog.c to not try to "recycle" a file that is a symlink.

OK, I tweaked Aidan's patch. Thanks Aidan!
http://archives.postgresql.org/message-id/20090601152736.GL15213@yugib.highrise.ca

Changes are:
- use lstat instead of stat
- add #if HAVE_WORKING_LINK and #endif code

Regards,

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

Fujii Masao wrote:
> On Tue, Jun 2, 2009 at 10:21 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>>> Yes, the old xlog itself is not used again. But, the *old file* might
>>> be recycled and used later. The case that I'm looking at is that the
>>> symlink to a temporary area is recycled. Am I missing something?
>> Actually, I think the right fix for that would be to add defenses to
>> xlog.c to not try to "recycle" a file that is a symlink.
>
> OK, I tweaked Aidan's patch. Thanks Aidan!
> http://archives.postgresql.org/message-id/20090601152736.GL15213@yugib.highrise.ca
>
> Changes are:
> - use lstat instead of stat
> - add #if HAVE_WORKING_LINK and #endif code

Committed. I left out the "#ifdef HAVE_WORKING_LINK" and used S_ISREG()
instead of S_ISLNK. We use lstat + S_ISREG elsewhere too, so there
should be no portability issues.

I backpatched to 8.3, since that's when pg_standby was added. Arguably
earlier versions should've been changed too, as pg_standby works with
earlier versions, but I decided to not rock the boat as this only
affects the pg_standby -l mode.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Hi,

On Tue, Jun 2, 2009 at 3:40 PM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> Fujii Masao wrote:
>>
>> On Tue, Jun 2, 2009 at 10:21 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>
>>> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>>>>
>>>> Yes, the old xlog itself is not used again. But, the *old file* might
>>>> be recycled and used later. The case that I'm looking at is that the
>>>> symlink to a temporary area is recycled. Am I missing something?
>>>
>>> Actually, I think the right fix for that would be to add defenses to
>>> xlog.c to not try to "recycle" a file that is a symlink.
>>
>> OK, I tweaked Aidan's patch. Thanks Aidan!
>>
>> http://archives.postgresql.org/message-id/20090601152736.GL15213@yugib.highrise.ca
>>
>> Changes are:
>> - use lstat instead of stat
>> - add #if HAVE_WORKING_LINK and #endif code
>
> Committed. I left out the "#ifdef HAVE_WORKING_LINK" and used S_ISREG()
> instead of S_ISLNK. We use lstat + S_ISREG elsewhere too, so there should be
> no portability issues.

Thanks a lot!

Regards,

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

On Mon, 2009-06-01 at 14:47 +0900, Fujii Masao wrote:

> pg_standby can use ln command to restore an archived file,
> which might destroy the archived file as follows.
>
> 1) pg_standby creates the symlink to the archived file '102'
> 2) '102' is applied
> 3) the next file '103' doesn't exist and the trigger file is created
> 4) '102' is re-fetched
> 5) at the end of recovery, the symlink to '102' is rename to '202',
> but it still points '102'
> 6) after recovery, '202' is recycled (rename to '208', which still
> points '102')
> 7) '208' is written new xlog records over
> --> the archived file '102' comes down!
>
> One simple solution to fix this problem...

err...I don't see *any* problem at all, since pg_standby does not do
step (1) in the way you say and therefore never does step (5). Any links
created are explicitly deleted in all cases at the end of recovery.

General comment on thread: What's going on with all these "fixes"?
Anybody reading the commit log and/or weekly news is going to get fairly
worried for no reason at all. For that reason I ask for longer
consideration and wider discussion before committing something - it
would certainly avoid lengthy post commit discussion as has occurred
twice recently. I see no reason for such haste on these "fixes". If
there's a need for haste, ship them to your customers directly, please
don't scare other people's.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> err...I don't see *any* problem at all, since pg_standby does not do
> step (1) in the way you say and therefore never does step (5). Any links
> created are explicitly deleted in all cases at the end of recovery.

That's a good point; don't we recover files under names like
RECOVERYXLOG, not under names that could possibly conflict with regular
WAL files?

regards, tom lane

Simon Riggs wrote:
> On Mon, 2009-06-01 at 14:47 +0900, Fujii Masao wrote:
>
>> pg_standby can use ln command to restore an archived file,
>> which might destroy the archived file as follows.
>>
>> 1) pg_standby creates the symlink to the archived file '102'
>> 2) '102' is applied
>> 3) the next file '103' doesn't exist and the trigger file is created
>> 4) '102' is re-fetched
>> 5) at the end of recovery, the symlink to '102' is rename to '202',
>> but it still points '102'
>> 6) after recovery, '202' is recycled (rename to '208', which still
>> points '102')
>> 7) '208' is written new xlog records over
>> --> the archived file '102' comes down!
>>
>> One simple solution to fix this problem...
>
> err...I don't see *any* problem at all, since pg_standby does not do
> step (1) in the way you say and therefore never does step (5). Any links
> created are explicitly deleted in all cases at the end of recovery.

I don't know how you came to that conclusion, but Fujii-sans description
seems accurate to me, and I can reproduce the behavior on my laptop.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> err...I don't see *any* problem at all, since pg_standby does not do
>> step (1) in the way you say and therefore never does step (5). Any links
>> created are explicitly deleted in all cases at the end of recovery.
>
> That's a good point; don't we recover files under names like
> RECOVERYXLOG, not under names that could possibly conflict with regular
> WAL files?

Yes. But we rename RECOVERYXLOG to 000000010000000000000057 or similar
at the end of recovery, in exitArchiveRecovery().

Thinking about this some more, I think we should've changed
exitArchiveRecovery() rather than RemoveOldXlogFiles(): it would be more
robust if exitArchiveRecovery() always copied the last WAL file rather
than just renamed it. It doesn't seem safe to rely on the file the
symlink points to to be valid after recovery is finished, and we might
write to it before it's recycled, so the current fix isn't complete.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> Tom Lane wrote:
>> That's a good point; don't we recover files under names like
>> RECOVERYXLOG, not under names that could possibly conflict with regular
>> WAL files?

> Yes. But we rename RECOVERYXLOG to 000000010000000000000057 or similar
> at the end of recovery, in exitArchiveRecovery().

> Thinking about this some more, I think we should've changed
> exitArchiveRecovery() rather than RemoveOldXlogFiles(): it would be more
> robust if exitArchiveRecovery() always copied the last WAL file rather
> than just renamed it. It doesn't seem safe to rely on the file the
> symlink points to to be valid after recovery is finished, and we might
> write to it before it's recycled, so the current fix isn't complete.

Hmm. I think really the reason it's coded that way is that we assumed
the recovery command would be physically copying the file from someplace
else. pg_standby is violating the backend's expectations by using a
symlink. And I really doubt that the technique is saving anything, since
the data has to be read in from the archive location anyway.

I'm leaning back to the position that pg_standby's -l option is simply a
bad idea and should be removed.

regards, tom lane

Hi,

On Wed, Jun 3, 2009 at 3:54 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
>> Tom Lane wrote:
>>> That's a good point; don't we recover files under names like
>>> RECOVERYXLOG, not under names that could possibly conflict with regular
>>> WAL files?
>
>> Yes. But we rename RECOVERYXLOG to 000000010000000000000057 or similar
>> at the end of recovery, in exitArchiveRecovery().
>
>> Thinking about this some more, I think we should've changed
>> exitArchiveRecovery() rather than RemoveOldXlogFiles(): it would be more
>> robust if exitArchiveRecovery() always copied the last WAL file rather
>> than just renamed it. It doesn't seem safe to rely on the file the
>> symlink points to to be valid after recovery is finished, and we might
>> write to it before it's recycled, so the current fix isn't complete.
>
> Hmm.  I think really the reason it's coded that way is that we assumed
> the recovery command would be physically copying the file from someplace
> else.  pg_standby is violating the backend's expectations by using a
> symlink.  And I really doubt that the technique is saving anything, since
> the data has to be read in from the archive location anyway.
>
> I'm leaning back to the position that pg_standby's -l option is simply a
> bad idea and should be removed.

I'm OK with this. And, we should document the assumption for
restore_command? Otherwise, some users might wrongly use
'ln' command to restore archived files.

Regards,

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

On Tue, 2009-06-02 at 14:54 -0400, Tom Lane wrote:
> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> > Tom Lane wrote:
> >> That's a good point; don't we recover files under names like
> >> RECOVERYXLOG, not under names that could possibly conflict with regular
> >> WAL files?
>
> > Yes. But we rename RECOVERYXLOG to 000000010000000000000057 or similar
> > at the end of recovery, in exitArchiveRecovery().
>
> > Thinking about this some more, I think we should've changed
> > exitArchiveRecovery() rather than RemoveOldXlogFiles(): it would be more
> > robust if exitArchiveRecovery() always copied the last WAL file rather
> > than just renamed it. It doesn't seem safe to rely on the file the
> > symlink points to to be valid after recovery is finished, and we might
> > write to it before it's recycled, so the current fix isn't complete.
>
> Hmm. I think really the reason it's coded that way is that we assumed
> the recovery command would be physically copying the file from someplace
> else. pg_standby is violating the backend's expectations by using a
> symlink. And I really doubt that the technique is saving anything, since
> the data has to be read in from the archive location anyway.
>
> I'm leaning back to the position that pg_standby's -l option is simply a
> bad idea and should be removed.

ISTM we didn't clearly state what the recovery_command should do either
way. Even if you remove the pg_standby option that will not fix the
problem for people who have written their own script, or existing users
of pg_standby. The safe way is to do as Heikki suggests and copy the
final file into place and I would add that we must then fsync it also.
That should be back-patched possibly as far as 8.0. Documenting a change
is not nearly enough.

Removing -l is a separate discussion. If there was a consensus against
it, I would suggest that we deprecate the option, so that it does
nothing.

As an aside, I would be also much more comfortable if there was an
option to not recycle the WAL files at all, as a safe mode for error
checking at least. The question is: why do we need to zero fill the file
first anyway? We could save the 8 bytes for the prev pointer on every
record if we added enough zeroes onto every WAL write to zap any
pre-existing header data, causing recovery to fail.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Tue, 2009-06-02 at 19:43 +0300, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > On Mon, 2009-06-01 at 14:47 +0900, Fujii Masao wrote:
> >
> >> pg_standby can use ln command to restore an archived file,
> >> which might destroy the archived file as follows.
> >>
> >> 1) pg_standby creates the symlink to the archived file '102'
> >> 2) '102' is applied
> >> 3) the next file '103' doesn't exist and the trigger file is created
> >> 4) '102' is re-fetched
> >> 5) at the end of recovery, the symlink to '102' is rename to '202',
> >> but it still points '102'
> >> 6) after recovery, '202' is recycled (rename to '208', which still
> >> points '102')
> >> 7) '208' is written new xlog records over
> >> --> the archived file '102' comes down!
> >>
> >> One simple solution to fix this problem...
> >
> > err...I don't see *any* problem at all, since pg_standby does not do
> > step (1) in the way you say and therefore never does step (5). Any links
> > created are explicitly deleted in all cases at the end of recovery.
>
> I don't know how you came to that conclusion, but Fujii-sans description
> seems accurate to me, and I can reproduce the behavior on my laptop.

I accept there is a bug, though the initial description did not appear
to reflect the way the code works.

Thank you for not making further changes while we wait to discuss.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support