Re: [HACKERS] pg_standby -l might destory the archived file
On Tue, 2009-06-02 at 14:54 -0400, Tom Lane wrote: Heikki Linnakangas heikki.linnakan...@enterprisedb.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 00010057 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Hi, On Wed, Jun 3, 2009 at 3:54 AM, Tom Lane t...@sss.pgh.pa.us wrote: Heikki Linnakangas heikki.linnakan...@enterprisedb.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 00010057 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Fujii Masao wrote: On Tue, Jun 2, 2009 at 10:21 AM, Tom Lane t...@sss.pgh.pa.us wrote: Fujii Masao masao.fu...@gmail.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.gl15...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Hi, On Tue, Jun 2, 2009 at 3:40 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Fujii Masao wrote: On Tue, Jun 2, 2009 at 10:21 AM, Tom Lane t...@sss.pgh.pa.us wrote: Fujii Masao masao.fu...@gmail.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.gl15...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Simon Riggs si...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Tom Lane wrote: Simon Riggs si...@2ndquadrant.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 00010057 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Heikki Linnakangas heikki.linnakan...@enterprisedb.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 00010057 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Fujii Masao masao.fu...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Tom Lane wrote: Fujii Masao masao.fu...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
* Heikki Linnakangas heikki.linnakan...@enterprisedb.com [090601 10:56]:
Tom Lane wrote:
Fujii Masao masao.fu...@gmail.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,
ai...@highrise.ca command like a king,
http://www.highrise.ca/ work like a slave.
signature.asc
Description: Digital signature
Re: [HACKERS] pg_standby -l might destory the archived file
Aidan Van Dyk wrote: * Heikki Linnakangas heikki.linnakan...@enterprisedb.com [090601 10:56]: Tom Lane wrote: Fujii Masao masao.fu...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: I wonder if we should just remove the symlink option from pg_standby. I was considering suggesting that too... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Hi, On Mon, Jun 1, 2009 at 11:41 PM, Tom Lane t...@sss.pgh.pa.us wrote: Fujii Masao masao.fu...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Fujii Masao masao.fu...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Hi, On Tue, Jun 2, 2009 at 10:00 AM, Tom Lane t...@sss.pgh.pa.us wrote: Fujii Masao masao.fu...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Fujii Masao masao.fu...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_standby -l might destory the archived file
Hi,
On Tue, Jun 2, 2009 at 10:21 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Fujii Masao masao.fu...@gmail.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.gl15...@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
Index: src/backend/access/transam/xlog.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xlog.c,v
retrieving revision 1.341
diff -c -r1.341 xlog.c
*** src/backend/access/transam/xlog.c 28 May 2009 11:02:16 - 1.341
--- src/backend/access/transam/xlog.c 2 Jun 2009 02:31:34 -
***
*** 3006,3011
--- 3006,3012
struct dirent *xlde;
char lastoff[MAXFNAMELEN];
char path[MAXPGPATH];
+ struct stat stat_buf;
/*
* Initialize info about where to try to recycle to. We allow recycling
***
*** 3047,3054
/*
* Before deleting the file, see if it can be recycled as a
* future log segment.
*/
! if (InstallXLogFileSegment(endlogId, endlogSeg, path,
true, max_advance,
true))
{
--- 3048,3060
/*
* 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 (
! #if HAVE_WORKING_LINK
! lstat(path, stat_buf) == 0 !S_ISLNK(stat_buf.st_mode)
! #endif
! InstallXLogFileSegment(endlogId, endlogSeg, path,
true, max_advance,
true))
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
