On 10/10/2016 05:25 PM, Michael Paquier wrote:
On Fri, Oct 7, 2016 at 2:59 AM, Pavan Deolasee <pavan.deola...@gmail.com> wrote:
I believe the fix is very simple. The FSM change during truncation is
critical and the buffer must be marked by MarkBufferDirty() i.e. those
changes must make to the disk. I think it's alright not to WAL log them
because XLOG_SMGR_TRUNCATE will redo() them if a crash occurs. But it must
not be lost across a checkpoint. Also, since it happens only during relation
truncation, I don't see any problem from performance perspective.
Agreed. I happen to notice that VM is similalry careful when it comes
to truncate it (visibilitymap_truncate).
visibilitymap_truncate is actually also wrong, in a different way. The
truncation WAL record is written only after the VM (and FSM) are
truncated. But visibilitymap_truncate() has already modified and dirtied
the page. If the VM page change is flushed to disk before the WAL
record, and you crash, you might have a torn VM page and a checksum failure.
Simply replacing the MarkBufferDirtyHint() call with MarkBufferDirty()
in FreeSpaceMapTruncateRel would have the same issue. If you call
MarkBufferDirty(), you must WAL-log the change, and also set the page's
LSN to make sure the WAL record is flushed first.
I think we need something like the attached.
- Heikki
>From 7b100e951236c21656eb7bd63b4631c0c32cc573 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Mon, 17 Oct 2016 14:02:52 +0300
Subject: [PATCH 1/1] WIP: Fix "FSM corruption leading to errors"
---
src/backend/access/heap/visibilitymap.c | 9 ++-
src/backend/catalog/storage.c | 110 ++++++++++++++++++------------
src/backend/storage/freespace/freespace.c | 14 +++-
src/include/access/visibilitymap.h | 2 +-
src/include/storage/freespace.h | 3 +-
src/test/recovery/t/008_fsm_check.pl | 90 ++++++++++++++++++++++++
6 files changed, 179 insertions(+), 49 deletions(-)
create mode 100644 src/test/recovery/t/008_fsm_check.pl
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index 3ad4a9f..377e511 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -457,9 +457,14 @@ visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_fro
* before they access the VM again.
*
* nheapblocks is the new size of the heap.
+ *
+ * The caller must have WAL-logged the truncation already (if the relation
+ * needs WAL-logging at all). 'lsn' is the LSN of the XLOG record. It is
+ * used to stamp the last remaining VM page, so that it doesn't get flushed
+ * to disk before the WAL record.
*/
void
-visibilitymap_truncate(Relation rel, BlockNumber nheapblocks)
+visibilitymap_truncate(Relation rel, BlockNumber nheapblocks, XLogRecPtr lsn)
{
BlockNumber newnblocks;
@@ -524,6 +529,8 @@ visibilitymap_truncate(Relation rel, BlockNumber nheapblocks)
map[truncByte] &= (1 << truncOffset) - 1;
MarkBufferDirty(mapBuffer);
+ if (lsn != InvalidXLogRecPtr)
+ PageSetLSN(page, lsn);
UnlockReleaseBuffer(mapBuffer);
}
else
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 0d8311c..5478953 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -28,6 +28,7 @@
#include "catalog/storage.h"
#include "catalog/storage_xlog.h"
#include "storage/freespace.h"
+#include "storage/proc.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -228,6 +229,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
{
bool fsm;
bool vm;
+ XLogRecPtr lsn = InvalidXLogRecPtr;
/* Open it at the smgr level if not already done */
RelationOpenSmgr(rel);
@@ -239,56 +241,76 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
rel->rd_smgr->smgr_fsm_nblocks = InvalidBlockNumber;
rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
- /* Truncate the FSM first if it exists */
- fsm = smgrexists(rel->rd_smgr, FSM_FORKNUM);
- if (fsm)
- FreeSpaceMapTruncateRel(rel, nblocks);
-
- /* Truncate the visibility map too if it exists. */
- vm = smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM);
- if (vm)
- visibilitymap_truncate(rel, nblocks);
-
- /*
- * We WAL-log the truncation before actually truncating, which means
- * trouble if the truncation fails. If we then crash, the WAL replay
- * likely isn't going to succeed in the truncation either, and cause a
- * PANIC. It's tempting to put a critical section here, but that cure
- * would be worse than the disease. It would turn a usually harmless
- * failure to truncate, that might spell trouble at WAL replay, into a
- * certain PANIC.
- */
- if (RelationNeedsWAL(rel))
+ PG_TRY();
{
/*
- * Make an XLOG entry reporting the file truncation.
+ * We WAL-log the truncation before actually truncating, which means
+ * trouble if the truncation fails. If we then crash, the WAL replay
+ * likely isn't going to succeed in the truncation either, and cause a
+ * PANIC. It's tempting to put a critical section here, but that cure
+ * would be worse than the disease. It would turn a usually harmless
+ * failure to truncate, that might spell trouble at WAL replay, into a
+ * certain PANIC.
*/
- XLogRecPtr lsn;
- xl_smgr_truncate xlrec;
-
- xlrec.blkno = nblocks;
- xlrec.rnode = rel->rd_node;
- xlrec.flags = SMGR_TRUNCATE_ALL;
+ if (RelationNeedsWAL(rel))
+ {
+ /*
+ * Make an XLOG entry reporting the file truncation.
+ */
+ xl_smgr_truncate xlrec;
+
+ /*
+ * Since we're not holding a buffer lock, use delayChkpt to
+ * prevent a checkpoint from happening between WAL-logging, and
+ * performing the truncation.
+ */
+ MyPgXact->delayChkpt = true;
+
+ xlrec.blkno = nblocks;
+ xlrec.rnode = rel->rd_node;
+ xlrec.flags = SMGR_TRUNCATE_ALL;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, sizeof(xlrec));
+
+ lsn = XLogInsert(RM_SMGR_ID,
+ XLOG_SMGR_TRUNCATE | XLR_SPECIAL_REL_UPDATE);
+
+ /*
+ * Flush, because otherwise the truncation of the main relation
+ * might hit the disk before the WAL record, and the truncation of
+ * the FSM or visibility map. If we crashed during that window,
+ * we'd be left with a truncated heap, but the FSM or visibility
+ * map would still contain entries for the non-existent heap
+ * pages.
+ */
+ if (fsm || vm)
+ XLogFlush(lsn);
+ }
- XLogBeginInsert();
- XLogRegisterData((char *) &xlrec, sizeof(xlrec));
+ /* Truncate the FSM first if it exists */
+ fsm = smgrexists(rel->rd_smgr, FSM_FORKNUM);
+ if (fsm)
+ FreeSpaceMapTruncateRel(rel, nblocks, lsn);
- lsn = XLogInsert(RM_SMGR_ID,
- XLOG_SMGR_TRUNCATE | XLR_SPECIAL_REL_UPDATE);
+ /* Truncate the visibility map too if it exists. */
+ vm = smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM);
+ if (vm)
+ visibilitymap_truncate(rel, nblocks, lsn);
- /*
- * Flush, because otherwise the truncation of the main relation might
- * hit the disk before the WAL record, and the truncation of the FSM
- * or visibility map. If we crashed during that window, we'd be left
- * with a truncated heap, but the FSM or visibility map would still
- * contain entries for the non-existent heap pages.
- */
- if (fsm || vm)
- XLogFlush(lsn);
+ /* Do the real work */
+ smgrtruncate(rel->rd_smgr, MAIN_FORKNUM, nblocks);
}
+ PG_CATCH();
+ {
+ if (RelationNeedsWAL(rel))
+ MyPgXact->delayChkpt = false;
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- /* Do the real work */
- smgrtruncate(rel->rd_smgr, MAIN_FORKNUM, nblocks);
+ if (RelationNeedsWAL(rel))
+ MyPgXact->delayChkpt = false;
}
/*
@@ -536,10 +558,10 @@ smgr_redo(XLogReaderState *record)
if ((xlrec->flags & SMGR_TRUNCATE_FSM) != 0 &&
smgrexists(reln, FSM_FORKNUM))
- FreeSpaceMapTruncateRel(rel, xlrec->blkno);
+ FreeSpaceMapTruncateRel(rel, xlrec->blkno, lsn);
if ((xlrec->flags & SMGR_TRUNCATE_VM) != 0 &&
smgrexists(reln, VISIBILITYMAP_FORKNUM))
- visibilitymap_truncate(rel, xlrec->blkno);
+ visibilitymap_truncate(rel, xlrec->blkno, lsn);
FreeFakeRelcacheEntry(rel);
}
diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index bbd90c9..4483984 100644
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -294,9 +294,14 @@ GetRecordedFreeSpace(Relation rel, BlockNumber heapBlk)
* before they access the FSM again.
*
* nblocks is the new size of the heap.
+ *
+ * The caller must have WAL-logged the truncation already (if the relation
+ * needs WAL-logging at all). 'lsn' is the LSN of the XLOG record. It is
+ * used to stamp the last remaining VM page, so that it doesn't get flushed
+ * to disk before the WAL record.
*/
void
-FreeSpaceMapTruncateRel(Relation rel, BlockNumber nblocks)
+FreeSpaceMapTruncateRel(Relation rel, BlockNumber nblocks, XLogRecPtr lsn)
{
BlockNumber new_nfsmblocks;
FSMAddress first_removed_address;
@@ -327,8 +332,13 @@ FreeSpaceMapTruncateRel(Relation rel, BlockNumber nblocks)
if (!BufferIsValid(buf))
return; /* nothing to do; the FSM was already smaller */
LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
+
fsm_truncate_avail(BufferGetPage(buf), first_removed_slot);
- MarkBufferDirtyHint(buf, false);
+
+ MarkBufferDirty(buf);
+ if (lsn != InvalidXLogRecPtr)
+ PageSetLSN(BufferGetPage(buf), lsn);
+
UnlockReleaseBuffer(buf);
new_nfsmblocks = fsm_logical_to_physical(first_removed_address) + 1;
diff --git a/src/include/access/visibilitymap.h b/src/include/access/visibilitymap.h
index 00bbd4c..8fe8906 100644
--- a/src/include/access/visibilitymap.h
+++ b/src/include/access/visibilitymap.h
@@ -44,6 +44,6 @@ extern void visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
uint8 flags);
extern uint8 visibilitymap_get_status(Relation rel, BlockNumber heapBlk, Buffer *vmbuf);
extern void visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_frozen);
-extern void visibilitymap_truncate(Relation rel, BlockNumber nheapblocks);
+extern void visibilitymap_truncate(Relation rel, BlockNumber nheapblocks, XLogRecPtr lsn);
#endif /* VISIBILITYMAP_H */
diff --git a/src/include/storage/freespace.h b/src/include/storage/freespace.h
index 77b3bc3..ac014a4 100644
--- a/src/include/storage/freespace.h
+++ b/src/include/storage/freespace.h
@@ -14,6 +14,7 @@
#ifndef FREESPACE_H_
#define FREESPACE_H_
+#include "access/xlogdefs.h"
#include "storage/block.h"
#include "storage/relfilenode.h"
#include "utils/relcache.h"
@@ -30,7 +31,7 @@ extern void RecordPageWithFreeSpace(Relation rel, BlockNumber heapBlk,
extern void XLogRecordPageWithFreeSpace(RelFileNode rnode, BlockNumber heapBlk,
Size spaceAvail);
-extern void FreeSpaceMapTruncateRel(Relation rel, BlockNumber nblocks);
+extern void FreeSpaceMapTruncateRel(Relation rel, BlockNumber nblocks, XLogRecPtr lsn);
extern void FreeSpaceMapVacuum(Relation rel);
extern void UpdateFreeSpaceMap(Relation rel,
BlockNumber startBlkNum,
diff --git a/src/test/recovery/t/008_fsm_check.pl b/src/test/recovery/t/008_fsm_check.pl
new file mode 100644
index 0000000..2c58e31
--- /dev/null
+++ b/src/test/recovery/t/008_fsm_check.pl
@@ -0,0 +1,90 @@
+# Set of tests to check file-space map consistency on standbys
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More tests => 1;
+
+my $node_master = get_new_node('master');
+$node_master->init(allows_streaming => 1);
+
+$node_master->append_conf('postgresql.conf', qq{
+fsync = on
+wal_level = replica
+wal_log_hints = on
+max_prepared_transactions = 5
+autovacuum = off
+});
+
+# Create a master node and its standby, initializing both with
+# some data at the same time.
+$node_master->start;
+
+$node_master->backup('master_backup');
+my $node_standby = get_new_node('standby');
+$node_standby->init_from_backup($node_master, 'master_backup',
+ has_streaming => 1);
+$node_standby->start;
+
+$node_master->psql('postgres', qq{
+create table testtab (a int, b char(100));
+insert into testtab select generate_series(1,1000), 'foo';
+insert into testtab select generate_series(1,1000), 'foo';
+delete from testtab where ctid > '(8,0)';
+});
+
+# Take a lock on the table to prevent following vacuum from truncating it
+$node_master->psql('postgres', qq{
+begin;
+lock table testtab in row share mode;
+prepare transaction 'p1';
+});
+
+# Vacuum, update FSM without truncation
+$node_master->psql('postgres', 'vacuum verbose testtab');
+
+# Force a checkpoint
+$node_master->psql('postgres', 'checkpoint');
+
+# Now do some more insert/deletes, another vacuum to ensure FPW writes are
+# done
+$node_master->psql('postgres', qq{
+insert into testtab select generate_series(1,1000), 'foo';
+delete from testtab where ctid > '(8,0)';
+vacuum verbose testtab;
+});
+
+# Ensure all buffers are now clean on the standby
+$node_standby->psql('postgres', 'checkpoint');
+
+# Release the lock, vacuum again which should lead to truncation
+$node_master->psql('postgres', qq{
+rollback prepared 'p1';
+vacuum verbose testtab;
+});
+
+$node_master->psql('postgres', 'checkpoint');
+my $until_lsn =
+ $node_master->safe_psql('postgres', "SELECT pg_current_xlog_location();");
+
+# Wait long enough for standby to receive and apply all WAL
+my $caughtup_query =
+ "SELECT '$until_lsn'::pg_lsn <= pg_last_xlog_replay_location()";
+$node_standby->poll_query_until('postgres', $caughtup_query)
+ or die "Timed out while waiting for standby to catch up";
+
+# now promote the standby
+$node_standby->promote;
+$node_standby->poll_query_until('postgres',
+ "SELECT NOT pg_is_in_recovery()")
+ or die "Timed out while waiting for promotion of standby";
+$node_standby->psql('postgres', 'checkpoint');
+
+# restart to discard in-memory copy of FSM
+$node_standby->restart;
+
+# Insert should work on standby
+is($node_standby->psql('postgres',
+ qq{insert into testtab select generate_series(1,1000), 'foo';}),
+ 0, 'INSERT succeeds with truncated relation FSM');
--
2.9.3
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
