On Tue, Sep 27, 2011 at 8:06 PM, Florian Pflug <f...@phlo.org> wrote:
> On Sep27, 2011, at 07:59 , Heikki Linnakangas wrote:
>> On 27.09.2011 00:28, Florian Pflug wrote:
>>> On Sep26, 2011, at 22:39 , Tom Lane wrote:
>>>> It might be worthwhile to invoke XLogCheckInvalidPages() as soon as
>>>> we (think we) have reached consistency, rather than leaving it to be
>>>> done only when we exit recovery mode.
>>>
>>> I believe we also need to prevent the creation of restart points before
>>> we've reached consistency.
>>
>> Seems reasonable. We could still allow restartpoints when the hash table is 
>> empty, though. And once we've reached consistency, we can throw an error 
>> immediately in log_invalid_page(), instead of adding the entry in the hash 
>> table.
>
> That mimics the way the rm_safe_restartpoint callbacks work, which is good.
>
> Actually, why don't we use that machinery to implement this? There's 
> currently no rm_safe_restartpoint callback for RM_XLOG_ID, so we'd just need 
> to create one that checks whether invalid_page_tab is empty.

Okay, the attached patch prevents the creation of restartpoints by using
rm_safe_restartpoint callback if we've not reached a consistent state yet
and the invalid-page table is not empty. But the invalid-page table is not
tied to the specific resource manager, so using rm_safe_restartpoint for
that seems to slightly odd. Is this OK?

Also, according to other suggestions, the patch changes XLogCheckInvalidPages()
so that it's called as soon as we've reached a consistent state, and changes
log_invalid_page() so that it emits PANIC immediately if consistency is already
reached. These are very good changes, I think. Because they enable us to
notice serious problem which causes PANIC error immediately. Without these
changes, you unfortunately might notice that the standby database is corrupted
when failover happens. Though such a problem might rarely happen, I think it's
worth doing those changes.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
*** a/src/backend/access/transam/rmgr.c
--- b/src/backend/access/transam/rmgr.c
***************
*** 16,21 ****
--- 16,22 ----
  #include "access/nbtree.h"
  #include "access/xact.h"
  #include "access/xlog_internal.h"
+ #include "access/xlogutils.h"
  #include "catalog/storage.h"
  #include "commands/dbcommands.h"
  #include "commands/sequence.h"
***************
*** 25,31 ****
  
  
  const RmgrData RmgrTable[RM_MAX_ID + 1] = {
! 	{"XLOG", xlog_redo, xlog_desc, NULL, NULL, NULL},
  	{"Transaction", xact_redo, xact_desc, NULL, NULL, NULL},
  	{"Storage", smgr_redo, smgr_desc, NULL, NULL, NULL},
  	{"CLOG", clog_redo, clog_desc, NULL, NULL, NULL},
--- 26,32 ----
  
  
  const RmgrData RmgrTable[RM_MAX_ID + 1] = {
! 	{"XLOG", xlog_redo, xlog_desc, NULL, NULL, xlog_safe_restartpoint},
  	{"Transaction", xact_redo, xact_desc, NULL, NULL, NULL},
  	{"Storage", smgr_redo, smgr_desc, NULL, NULL, NULL},
  	{"CLOG", clog_redo, clog_desc, NULL, NULL, NULL},
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***************
*** 557,563 **** static TimeLineID lastPageTLI = 0;
  static XLogRecPtr minRecoveryPoint;		/* local copy of
  										 * ControlFile->minRecoveryPoint */
  static bool updateMinRecoveryPoint = true;
! static bool reachedMinRecoveryPoint = false;
  
  static bool InRedo = false;
  
--- 557,563 ----
  static XLogRecPtr minRecoveryPoint;		/* local copy of
  										 * ControlFile->minRecoveryPoint */
  static bool updateMinRecoveryPoint = true;
! bool reachedMinRecoveryPoint = false;
  
  static bool InRedo = false;
  
***************
*** 6840,6851 **** StartupXLOG(void)
  		LocalXLogInsertAllowed = -1;
  
  		/*
- 		 * Check to see if the XLOG sequence contained any unresolved
- 		 * references to uninitialized pages.
- 		 */
- 		XLogCheckInvalidPages();
- 
- 		/*
  		 * Perform a checkpoint to update all our recovery activity to disk.
  		 *
  		 * Note that we write a shutdown checkpoint rather than an on-line
--- 6840,6845 ----
***************
*** 6982,6987 **** CheckRecoveryConsistency(void)
--- 6976,6987 ----
  		XLByteLE(minRecoveryPoint, EndRecPtr) &&
  		XLogRecPtrIsInvalid(ControlFile->backupStartPoint))
  	{
+ 		/*
+ 		 * Check to see if the XLOG sequence contained any unresolved
+ 		 * references to uninitialized pages.
+ 		 */
+ 		XLogCheckInvalidPages();
+ 
  		reachedMinRecoveryPoint = true;
  		ereport(LOG,
  				(errmsg("consistent recovery state reached at %X/%X",
*** a/src/backend/access/transam/xlogutils.c
--- b/src/backend/access/transam/xlogutils.c
***************
*** 52,57 **** typedef struct xl_invalid_page
--- 52,73 ----
  static HTAB *invalid_page_tab = NULL;
  
  
+ /* Report a reference to an invalid page */
+ static void
+ report_invalid_page(int elevel, RelFileNode node, ForkNumber forkno,
+ 					BlockNumber blkno, bool present)
+ {
+ 	char	   *path = relpathperm(node, forkno);
+ 
+ 	if (present)
+ 		elog(elevel, "page %u of relation %s is uninitialized",
+ 			 blkno, path);
+ 	else
+ 		elog(elevel, "page %u of relation %s does not exist",
+ 			 blkno, path);
+ 	pfree(path);
+ }
+ 
  /* Log a reference to an invalid page */
  static void
  log_invalid_page(RelFileNode node, ForkNumber forkno, BlockNumber blkno,
***************
*** 62,83 **** log_invalid_page(RelFileNode node, ForkNumber forkno, BlockNumber blkno,
  	bool		found;
  
  	/*
  	 * Log references to invalid pages at DEBUG1 level.  This allows some
  	 * tracing of the cause (note the elog context mechanism will tell us
  	 * something about the XLOG record that generated the reference).
  	 */
  	if (log_min_messages <= DEBUG1 || client_min_messages <= DEBUG1)
! 	{
! 		char	   *path = relpathperm(node, forkno);
! 
! 		if (present)
! 			elog(DEBUG1, "page %u of relation %s is uninitialized",
! 				 blkno, path);
! 		else
! 			elog(DEBUG1, "page %u of relation %s does not exist",
! 				 blkno, path);
! 		pfree(path);
! 	}
  
  	if (invalid_page_tab == NULL)
  	{
--- 78,101 ----
  	bool		found;
  
  	/*
+ 	 * Once recovery has reached a consistent state, the invalid-page
+ 	 * table should be empty and remain so. If a reference to an invalid
+ 	 * page is found after consistency is reached, emit PANIC immediately
+ 	 * instead of adding the entry in the invalid-page table.
+ 	 */
+ 	if (reachedMinRecoveryPoint)
+ 	{
+ 		report_invalid_page(WARNING, node, forkno, blkno, present);
+ 		elog(PANIC, "WAL contains references to invalid pages");
+ 	}
+ 
+ 	/*
  	 * Log references to invalid pages at DEBUG1 level.  This allows some
  	 * tracing of the cause (note the elog context mechanism will tell us
  	 * something about the XLOG record that generated the reference).
  	 */
  	if (log_min_messages <= DEBUG1 || client_min_messages <= DEBUG1)
! 		report_invalid_page(DEBUG1, node, forkno, blkno, present);
  
  	if (invalid_page_tab == NULL)
  	{
***************
*** 200,214 **** XLogCheckInvalidPages(void)
  	 */
  	while ((hentry = (xl_invalid_page *) hash_seq_search(&status)) != NULL)
  	{
! 		char	   *path = relpathperm(hentry->key.node, hentry->key.forkno);
! 
! 		if (hentry->present)
! 			elog(WARNING, "page %u of relation %s was uninitialized",
! 				 hentry->key.blkno, path);
! 		else
! 			elog(WARNING, "page %u of relation %s did not exist",
! 				 hentry->key.blkno, path);
! 		pfree(path);
  		foundone = true;
  	}
  
--- 218,225 ----
  	 */
  	while ((hentry = (xl_invalid_page *) hash_seq_search(&status)) != NULL)
  	{
! 		report_invalid_page(WARNING, hentry->key.node, hentry->key.forkno,
! 							hentry->key.blkno, hentry->present);
  		foundone = true;
  	}
  
***************
*** 449,451 **** XLogTruncateRelation(RelFileNode rnode, ForkNumber forkNum,
--- 460,471 ----
  {
  	forget_invalid_pages(rnode, forkNum, nblocks);
  }
+ 
+ bool
+ xlog_safe_restartpoint(void)
+ {
+ 	if (invalid_page_tab != NULL &&
+ 		hash_get_num_entries(invalid_page_tab) > 0)
+ 		return false;
+ 	return true;
+ }
*** a/src/include/access/xlog.h
--- b/src/include/access/xlog.h
***************
*** 189,194 **** typedef enum
--- 189,196 ----
  
  extern XLogRecPtr XactLastRecEnd;
  
+ extern bool reachedMinRecoveryPoint;
+ 
  /* these variables are GUC parameters related to XLOG */
  extern int	CheckPointSegments;
  extern int	wal_keep_segments;
*** a/src/include/access/xlogutils.h
--- b/src/include/access/xlogutils.h
***************
*** 28,31 **** extern Buffer XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum,
--- 28,33 ----
  extern Relation CreateFakeRelcacheEntry(RelFileNode rnode);
  extern void FreeFakeRelcacheEntry(Relation fakerel);
  
+ extern bool xlog_safe_restartpoint(void);
+ 
  #endif
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to