pending patch: Re: Streaming replication and pg_xlogfile_name()

Started by Fujii Masaoalmost 16 years ago9 messages
#1Fujii Masao
masao.fujii@gmail.com
1 attachment(s)

On Wed, Mar 3, 2010 at 11:03 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Mar 2, 2010 at 10:52 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

It's not clear what it should return, a TLI corresponding the filename
of the WAL segment the record was replayed from, so that you can use
pg_xlogfile_name() to find out the filename of the WAL segment being
replayed, or the accurate TLI of the record being replayed. I'm leaning
towards the latter, it feels more correct and accurate, but you could
argue for the former too. In any case, it needs to be well-defined.

I agree with you that the latter is more correct and accurate. The simple
fix is updating the lastPageTLI with the CheckPoint->ThisTimeLineID when
replaying the shutdown checkpoint record. Though we might need to use new
variable to keep the last applied timeline instead of the lastPageTLI.

Here is the revised patch. I used new local variable instead of lastPageTLI
to track the tli of last applied record. It is updated with the tli of the
log page header when reading the page, and with the tli of the checkpoint
record when replaying the checkpoint shutdown record that changes the tli.
So pg_last_xlog_replay_location() can return the accurate tli of the last
applied record.

I rebased the patch to HEAD. Should I think that the patch has been rejected
because it has remained pending for about one month?

Regards,

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

Attachments:

extend_format_of_recovery_info_funcs_v5.patchapplication/octet-stream; name=extend_format_of_recovery_info_funcs_v5.patchDownload
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***************
*** 13206,13211 **** postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
--- 13206,13215 ----
      This is usually the desired behavior for managing transaction log archiving
      behavior, since the preceding file is the last one that currently
      needs to be archived.
+     These functions also accept as a parameter the string that consists of timeline and
+     location, separated by a slash. In this case a transaction log file name is computed
+     by using the given timeline. On the other hand, if timeline is not supplied, the
+     current timeline is used for the computation.
     </para>
  
     <para>
***************
*** 13252,13264 **** postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
          <literal><function>pg_last_xlog_receive_location</function>()</literal>
          </entry>
         <entry><type>text</type></entry>
!        <entry>Get last transaction log location received and synced to disk during
!         streaming recovery. If streaming recovery is still in progress
          this will increase monotonically. If streaming recovery has completed
          then this value will remain static at the value of the last WAL record
          received and synced to disk during that recovery. When the server has
          been started without a streaming recovery then the return value will be
!         InvalidXLogRecPtr (0/0).
         </entry>
        </row>
        <row>
--- 13256,13270 ----
          <literal><function>pg_last_xlog_receive_location</function>()</literal>
          </entry>
         <entry><type>text</type></entry>
!        <entry>Get timeline and location of last transaction log received and synced
!         to disk during streaming recovery. The return string is separated by a slash,
!         the first value indicates the timeline and the other the location.
!         If streaming recovery is still in progress
          this will increase monotonically. If streaming recovery has completed
          then this value will remain static at the value of the last WAL record
          received and synced to disk during that recovery. When the server has
          been started without a streaming recovery then the return value will be
!         <literal>0/0/0</>.
         </entry>
        </row>
        <row>
***************
*** 13266,13277 **** postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
          <literal><function>pg_last_xlog_replay_location</function>()</literal>
          </entry>
         <entry><type>text</type></entry>
!        <entry>Get last transaction log location replayed during recovery.
          If recovery is still in progress this will increase monotonically.
          If recovery has completed then this value will remain static at
          the value of the last WAL record applied during that recovery.
          When the server has been started normally without a recovery
!         then the return value will be InvalidXLogRecPtr (0/0).
         </entry>
        </row>
       </tbody>
--- 13272,13285 ----
          <literal><function>pg_last_xlog_replay_location</function>()</literal>
          </entry>
         <entry><type>text</type></entry>
!        <entry>Get timeline and location of last transaction log replayed during
!         recovery. The return string is separated by a slash, the first value
!         indicates the timeline and the other the location.
          If recovery is still in progress this will increase monotonically.
          If recovery has completed then this value will remain static at
          the value of the last WAL record applied during that recovery.
          When the server has been started normally without a recovery
!         then the return value will be <literal>0/0/0</>.
         </entry>
        </row>
       </tbody>
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***************
*** 397,402 **** typedef struct XLogCtlData
--- 397,404 ----
  	TimestampTz recoveryLastXTime;
  	/* end+1 of the last record replayed */
  	XLogRecPtr	recoveryLastRecPtr;
+ 	/* tli of last record replayed */
+ 	TimeLineID	recoveryLastTLI;
  
  	slock_t		info_lck;		/* locks shared variables shown above */
  } XLogCtlData;
***************
*** 476,481 **** static uint32 readRecordBufSize = 0;
--- 478,484 ----
  static XLogRecPtr ReadRecPtr;	/* start of last record read */
  static XLogRecPtr EndRecPtr;	/* end+1 of last record read */
  static TimeLineID lastPageTLI = 0;
+ static TimeLineID lastRecTLI = 0;	/* tli of last record read */
  
  static XLogRecPtr minRecoveryPoint;		/* local copy of
  										 * ControlFile->minRecoveryPoint */
***************
*** 3945,3951 **** ValidXLOGHeader(XLogPageHeader hdr, int emode)
  						readId, readSeg, readOff)));
  		return false;
  	}
! 	lastPageTLI = hdr->xlp_tli;
  	return true;
  }
  
--- 3948,3954 ----
  						readId, readSeg, readOff)));
  		return false;
  	}
! 	lastRecTLI = lastPageTLI = hdr->xlp_tli;
  	return true;
  }
  
***************
*** 5810,5819 **** StartupXLOG(void)
  			/* use volatile pointer to prevent code rearrangement */
  			volatile XLogCtlData *xlogctl = XLogCtl;
  
! 			/* initialize shared replayEndRecPtr and recoveryLastRecPtr */
  			SpinLockAcquire(&xlogctl->info_lck);
  			xlogctl->replayEndRecPtr = ReadRecPtr;
  			xlogctl->recoveryLastRecPtr = ReadRecPtr;
  			SpinLockRelease(&xlogctl->info_lck);
  
  			InRedo = true;
--- 5813,5829 ----
  			/* use volatile pointer to prevent code rearrangement */
  			volatile XLogCtlData *xlogctl = XLogCtl;
  
! 			/*
! 			 * initialize shared replayEndRecPtr, recoveryLastRecPtr and
! 			 * recoveryLastTLI. Actually, the latter two variables don't need to
! 			 * be initialized here since they are expected to be updated at least
! 			 * once until read only connections will have read them. But just in
! 			 * case.
! 			 */
  			SpinLockAcquire(&xlogctl->info_lck);
  			xlogctl->replayEndRecPtr = ReadRecPtr;
  			xlogctl->recoveryLastRecPtr = ReadRecPtr;
+ 			xlogctl->recoveryLastTLI = lastRecTLI;
  			SpinLockRelease(&xlogctl->info_lck);
  
  			InRedo = true;
***************
*** 5941,5951 **** StartupXLOG(void)
  				error_context_stack = errcontext.previous;
  
  				/*
! 				 * Update shared recoveryLastRecPtr after this record has been
! 				 * replayed.
  				 */
  				SpinLockAcquire(&xlogctl->info_lck);
  				xlogctl->recoveryLastRecPtr = EndRecPtr;
  				SpinLockRelease(&xlogctl->info_lck);
  
  				LastRec = ReadRecPtr;
--- 5951,5962 ----
  				error_context_stack = errcontext.previous;
  
  				/*
! 				 * Update shared recoveryLastRecPtr and recoveryLastTLI
! 				 * after this record has been replayed.
  				 */
  				SpinLockAcquire(&xlogctl->info_lck);
  				xlogctl->recoveryLastRecPtr = EndRecPtr;
+ 				xlogctl->recoveryLastTLI = lastRecTLI;
  				SpinLockRelease(&xlogctl->info_lck);
  
  				LastRec = ReadRecPtr;
***************
*** 7515,7520 **** xlog_redo(XLogRecPtr lsn, XLogRecord *record)
--- 7526,7532 ----
  			/* Following WAL records should be run with new TLI */
  			ThisTimeLineID = checkPoint.ThisTimeLineID;
  		}
+ 		lastRecTLI = ThisTimeLineID;
  
  		RecoveryRestartPoint(&checkPoint);
  	}
***************
*** 8310,8316 **** pg_current_xlog_insert_location(PG_FUNCTION_ARGS)
  }
  
  /*
!  * Report the last WAL receive location (same format as pg_start_backup etc)
   *
   * This is useful for determining how much of WAL is guaranteed to be received
   * and synced to disk by walreceiver.
--- 8322,8328 ----
  }
  
  /*
!  * Report the last WAL receive tli and location
   *
   * This is useful for determining how much of WAL is guaranteed to be received
   * and synced to disk by walreceiver.
***************
*** 8323,8335 **** pg_last_xlog_receive_location(PG_FUNCTION_ARGS)
  
  	recptr = GetWalRcvWriteRecPtr();
  
! 	snprintf(location, sizeof(location), "%X/%X",
  			 recptr.xlogid, recptr.xrecoff);
  	PG_RETURN_TEXT_P(cstring_to_text(location));
  }
  
  /*
!  * Report the last WAL replay location (same format as pg_start_backup etc)
   *
   * This is useful for determining how much of WAL is visible to read-only
   * connections during recovery.
--- 8335,8348 ----
  
  	recptr = GetWalRcvWriteRecPtr();
  
! 	snprintf(location, sizeof(location), "%X/%X/%X",
! 			 XLogRecPtrIsInvalid(recptr) ? 0 : GetRecoveryTargetTLI(),
  			 recptr.xlogid, recptr.xrecoff);
  	PG_RETURN_TEXT_P(cstring_to_text(location));
  }
  
  /*
!  * Report the last WAL replay tli and location
   *
   * This is useful for determining how much of WAL is visible to read-only
   * connections during recovery.
***************
*** 8339,8353 **** pg_last_xlog_replay_location(PG_FUNCTION_ARGS)
  {
  	/* use volatile pointer to prevent code rearrangement */
  	volatile XLogCtlData *xlogctl = XLogCtl;
  	XLogRecPtr	recptr;
  	char		location[MAXFNAMELEN];
  
  	SpinLockAcquire(&xlogctl->info_lck);
  	recptr = xlogctl->recoveryLastRecPtr;
  	SpinLockRelease(&xlogctl->info_lck);
  
! 	snprintf(location, sizeof(location), "%X/%X",
! 			 recptr.xlogid, recptr.xrecoff);
  	PG_RETURN_TEXT_P(cstring_to_text(location));
  }
  
--- 8352,8368 ----
  {
  	/* use volatile pointer to prevent code rearrangement */
  	volatile XLogCtlData *xlogctl = XLogCtl;
+ 	TimeLineID	tli;
  	XLogRecPtr	recptr;
  	char		location[MAXFNAMELEN];
  
  	SpinLockAcquire(&xlogctl->info_lck);
+ 	tli = xlogctl->recoveryLastTLI;
  	recptr = xlogctl->recoveryLastRecPtr;
  	SpinLockRelease(&xlogctl->info_lck);
  
! 	snprintf(location, sizeof(location), "%X/%X/%X",
! 			 tli, recptr.xlogid, recptr.xrecoff);
  	PG_RETURN_TEXT_P(cstring_to_text(location));
  }
  
***************
*** 8355,8360 **** pg_last_xlog_replay_location(PG_FUNCTION_ARGS)
--- 8370,8379 ----
   * Compute an xlog file name and decimal byte offset given a WAL location,
   * such as is returned by pg_stop_backup() or pg_xlog_switch().
   *
+  * Also use the tli for the computation if it's given with a location,
+  * such as is returned by pg_last_xlog_receive_location() or
+  * pg_last_xlog_replay_location().
+  *
   * Note that a location exactly at a segment boundary is taken to be in
   * the previous segment.  This is usually the right thing, since the
   * expected usage is to determine which xlog file(s) are ready to archive.
***************
*** 8364,8374 **** pg_xlogfile_name_offset(PG_FUNCTION_ARGS)
--- 8383,8395 ----
  {
  	text	   *location = PG_GETARG_TEXT_P(0);
  	char	   *locationstr;
+ 	unsigned int utli;
  	unsigned int uxlogid;
  	unsigned int uxrecoff;
  	uint32		xlogid;
  	uint32		xlogseg;
  	uint32		xrecoff;
+ 	TimeLineID	tli = ThisTimeLineID;
  	XLogRecPtr	locationpoint;
  	char		xlogfilename[MAXFNAMELEN];
  	Datum		values[2];
***************
*** 8382,8388 **** pg_xlogfile_name_offset(PG_FUNCTION_ARGS)
  	 */
  	locationstr = text_to_cstring(location);
  
! 	if (sscanf(locationstr, "%X/%X", &uxlogid, &uxrecoff) != 2)
  		ereport(ERROR,
  				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
  				 errmsg("could not parse transaction log location \"%s\"",
--- 8403,8411 ----
  	 */
  	locationstr = text_to_cstring(location);
  
! 	if (sscanf(locationstr, "%X/%X/%X", &utli, &uxlogid, &uxrecoff) == 3)
! 		tli = (TimeLineID) utli;
! 	else if (sscanf(locationstr, "%X/%X", &uxlogid, &uxrecoff) != 2)
  		ereport(ERROR,
  				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
  				 errmsg("could not parse transaction log location \"%s\"",
***************
*** 8407,8413 **** pg_xlogfile_name_offset(PG_FUNCTION_ARGS)
  	 * xlogfilename
  	 */
  	XLByteToPrevSeg(locationpoint, xlogid, xlogseg);
! 	XLogFileName(xlogfilename, ThisTimeLineID, xlogid, xlogseg);
  
  	values[0] = CStringGetTextDatum(xlogfilename);
  	isnull[0] = false;
--- 8430,8436 ----
  	 * xlogfilename
  	 */
  	XLByteToPrevSeg(locationpoint, xlogid, xlogseg);
! 	XLogFileName(xlogfilename, tli, xlogid, xlogseg);
  
  	values[0] = CStringGetTextDatum(xlogfilename);
  	isnull[0] = false;
***************
*** 8433,8454 **** pg_xlogfile_name_offset(PG_FUNCTION_ARGS)
  /*
   * Compute an xlog file name given a WAL location,
   * such as is returned by pg_stop_backup() or pg_xlog_switch().
   */
  Datum
  pg_xlogfile_name(PG_FUNCTION_ARGS)
  {
  	text	   *location = PG_GETARG_TEXT_P(0);
  	char	   *locationstr;
  	unsigned int uxlogid;
  	unsigned int uxrecoff;
  	uint32		xlogid;
  	uint32		xlogseg;
  	XLogRecPtr	locationpoint;
  	char		xlogfilename[MAXFNAMELEN];
  
  	locationstr = text_to_cstring(location);
  
! 	if (sscanf(locationstr, "%X/%X", &uxlogid, &uxrecoff) != 2)
  		ereport(ERROR,
  				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
  				 errmsg("could not parse transaction log location \"%s\"",
--- 8456,8485 ----
  /*
   * Compute an xlog file name given a WAL location,
   * such as is returned by pg_stop_backup() or pg_xlog_switch().
+  *
+  * Also use the tli for the computation if it's given with a location,
+  * such as is returned by pg_last_xlog_receive_location() or
+  * pg_last_xlog_replay_location().
   */
  Datum
  pg_xlogfile_name(PG_FUNCTION_ARGS)
  {
  	text	   *location = PG_GETARG_TEXT_P(0);
  	char	   *locationstr;
+ 	unsigned int utli;
  	unsigned int uxlogid;
  	unsigned int uxrecoff;
  	uint32		xlogid;
  	uint32		xlogseg;
+ 	TimeLineID	tli = ThisTimeLineID;
  	XLogRecPtr	locationpoint;
  	char		xlogfilename[MAXFNAMELEN];
  
  	locationstr = text_to_cstring(location);
  
! 	if (sscanf(locationstr, "%X/%X/%X", &utli, &uxlogid, &uxrecoff) == 3)
! 		tli = (TimeLineID) utli;
! 	else if (sscanf(locationstr, "%X/%X", &uxlogid, &uxrecoff) != 2)
  		ereport(ERROR,
  				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
  				 errmsg("could not parse transaction log location \"%s\"",
***************
*** 8458,8464 **** pg_xlogfile_name(PG_FUNCTION_ARGS)
  	locationpoint.xrecoff = uxrecoff;
  
  	XLByteToPrevSeg(locationpoint, xlogid, xlogseg);
! 	XLogFileName(xlogfilename, ThisTimeLineID, xlogid, xlogseg);
  
  	PG_RETURN_TEXT_P(cstring_to_text(xlogfilename));
  }
--- 8489,8495 ----
  	locationpoint.xrecoff = uxrecoff;
  
  	XLByteToPrevSeg(locationpoint, xlogid, xlogseg);
! 	XLogFileName(xlogfilename, tli, xlogid, xlogseg);
  
  	PG_RETURN_TEXT_P(cstring_to_text(xlogfilename));
  }
#2Robert Haas
robertmhaas@gmail.com
In reply to: Fujii Masao (#1)
Re: pending patch: Re: Streaming replication and pg_xlogfile_name()

On Tue, Mar 30, 2010 at 5:14 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Wed, Mar 3, 2010 at 11:03 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Mar 2, 2010 at 10:52 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

It's not clear what it should return, a TLI corresponding the filename
of the WAL segment the record was replayed from, so that you can use
pg_xlogfile_name() to find out the filename of the WAL segment being
replayed, or the accurate TLI of the record being replayed. I'm leaning
towards the latter, it feels more correct and accurate, but you could
argue for the former too. In any case, it needs to be well-defined.

I agree with you that the latter is more correct and accurate. The simple
fix is updating the lastPageTLI with the CheckPoint->ThisTimeLineID when
replaying the shutdown checkpoint record. Though we might need to use new
variable to keep the last applied timeline instead of the lastPageTLI.

Here is the revised patch. I used new local variable instead of lastPageTLI
to track the tli of last applied record. It is updated with the tli of the
log page header when reading the page, and with the tli of the checkpoint
record when replaying the checkpoint shutdown record that changes the tli.
So pg_last_xlog_replay_location() can return the accurate tli of the last
applied record.

I rebased the patch to HEAD. Should I think that the patch has been rejected
because it has remained pending for about one month?

Can someone explain to me in plain language what problem this is
trying to fix? I'm having trouble figuring it out.

...Robert

#3Fujii Masao
masao.fujii@gmail.com
In reply to: Robert Haas (#2)
Re: pending patch: Re: Streaming replication and pg_xlogfile_name()

On Fri, Apr 2, 2010 at 2:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:

Can someone explain to me in plain language what problem this is
trying to fix?  I'm having trouble figuring it out.

The problem is that pg_xlogfile_name(pg_last_xlog_receive_location()) and
pg_xlogfile_name(pg_last_xlog_replay_location()) might report an inaccurate
WAL file name because currently pg_xlogfile_name() always uses the current
timeline to calculate the WAL file name. For example, even though the last
applied WAL file is 000000010000000000000002, the standby wrongly reports
that 000000000000000000000002 has been applied last.

postgres=# SELECT l lsn, pg_xlogfile_name(l) filename FROM
pg_last_xlog_replay_location() AS l;
lsn | filename
-----------+--------------------------
0/200FF70 | 000000000000000000000002
(1 row)

$ ls sby/pg_xlog/
000000010000000000000002 000000010000000000000003 archive_status

Regards,

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

#4Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Fujii Masao (#3)
Re: pending patch: Re: Streaming replication and pg_xlogfile_name()

Fujii Masao wrote:

On Fri, Apr 2, 2010 at 2:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:

Can someone explain to me in plain language what problem this is
trying to fix? I'm having trouble figuring it out.

The problem is that pg_xlogfile_name(pg_last_xlog_receive_location()) and
pg_xlogfile_name(pg_last_xlog_replay_location()) might report an inaccurate
WAL file name because currently pg_xlogfile_name() always uses the current
timeline to calculate the WAL file name. For example, even though the last
applied WAL file is 000000010000000000000002, the standby wrongly reports
that 000000000000000000000002 has been applied last.

Should we throw an error in pg_xlogfile_name() if called during
recovery? It's not doing anything useful as it is.

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

#5Fujii Masao
masao.fujii@gmail.com
In reply to: Heikki Linnakangas (#4)
Re: pending patch: Re: Streaming replication and pg_xlogfile_name()

On Tue, Apr 6, 2010 at 7:25 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Fujii Masao wrote:

On Fri, Apr 2, 2010 at 2:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:

Can someone explain to me in plain language what problem this is
trying to fix?  I'm having trouble figuring it out.

The problem is that pg_xlogfile_name(pg_last_xlog_receive_location()) and
pg_xlogfile_name(pg_last_xlog_replay_location()) might report an inaccurate
WAL file name because currently pg_xlogfile_name() always uses the current
timeline to calculate the WAL file name. For example, even though the last
applied WAL file is 000000010000000000000002, the standby wrongly reports
that 000000000000000000000002 has been applied last.

Should we throw an error in pg_xlogfile_name() if called during
recovery? It's not doing anything useful as it is.

I have no objection for now.

Regards,

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

#6Fujii Masao
masao.fujii@gmail.com
In reply to: Fujii Masao (#5)
1 attachment(s)
Re: pending patch: Re: Streaming replication and pg_xlogfile_name()

On Tue, Apr 6, 2010 at 8:02 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

Should we throw an error in pg_xlogfile_name() if called during
recovery? It's not doing anything useful as it is.

I have no objection for now.

Here is the patch.

Regards,

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

Attachments:

forbid_pg_xlogfile_name_during_recovery_v1.patchapplication/octet-stream; name=forbid_pg_xlogfile_name_during_recovery_v1.patchDownload
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***************
*** 13064,13071 **** SELECT set_config('log_statement_stats', 'off', false);
     <para>
      The functions shown in <xref
      linkend="functions-admin-backup-table"> assist in making on-line backups.
!     Use of the first three functions is restricted to superusers. The first
!     five functions cannot be executed during recovery.
     </para>
  
     <table id="functions-admin-backup-table">
--- 13064,13071 ----
     <para>
      The functions shown in <xref
      linkend="functions-admin-backup-table"> assist in making on-line backups.
!     These functions cannot be executed during recovery.
!     Use of the first three functions is restricted to superusers.
     </para>
  
     <table id="functions-admin-backup-table">
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***************
*** 8409,8414 **** pg_xlogfile_name_offset(PG_FUNCTION_ARGS)
--- 8409,8420 ----
  	HeapTuple	resultHeapTuple;
  	Datum		result;
  
+ 	if (RecoveryInProgress())
+ 		ereport(ERROR,
+ 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ 				 errmsg("recovery is in progress"),
+ 				 errhint("WAL control functions cannot be executed during recovery.")));
+ 
  	/*
  	 * Read input and parse
  	 */
***************
*** 8478,8483 **** pg_xlogfile_name(PG_FUNCTION_ARGS)
--- 8484,8495 ----
  	XLogRecPtr	locationpoint;
  	char		xlogfilename[MAXFNAMELEN];
  
+ 	if (RecoveryInProgress())
+ 		ereport(ERROR,
+ 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ 				 errmsg("recovery is in progress"),
+ 				 errhint("WAL control functions cannot be executed during recovery.")));
+ 
  	locationstr = text_to_cstring(location);
  
  	if (sscanf(locationstr, "%X/%X", &uxlogid, &uxrecoff) != 2)
#7Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Fujii Masao (#6)
Re: pending patch: Re: Streaming replication and pg_xlogfile_name()

Fujii Masao wrote:

On Tue, Apr 6, 2010 at 8:02 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

Should we throw an error in pg_xlogfile_name() if called during
recovery? It's not doing anything useful as it is.

I have no objection for now.

Here is the patch.
...
+ 	if (RecoveryInProgress())
+ 		ereport(ERROR,
+ 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ 				 errmsg("recovery is in progress"),
+ 				 errhint("WAL control functions cannot be executed during recovery.")));
+ 

The hint is a bit confusing for pg_xlogfile_name(). pg_xlogfile_name()
is hardly a WAL control function like pg_start/stop_backup() are. How
about "pg_xlogfile_name() cannot be executed during recovery."?

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

#8Fujii Masao
masao.fujii@gmail.com
In reply to: Heikki Linnakangas (#7)
1 attachment(s)
Re: pending patch: Re: Streaming replication and pg_xlogfile_name()

On Tue, Apr 6, 2010 at 11:48 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Fujii Masao wrote:

On Tue, Apr 6, 2010 at 8:02 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

Should we throw an error in pg_xlogfile_name() if called during
recovery? It's not doing anything useful as it is.

I have no objection for now.

Here is the patch.
...
+     if (RecoveryInProgress())
+             ereport(ERROR,
+                             (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                              errmsg("recovery is in progress"),
+                              errhint("WAL control functions cannot be executed during recovery.")));
+

The hint is a bit confusing for pg_xlogfile_name(). pg_xlogfile_name()
is hardly a WAL control function like pg_start/stop_backup() are. How
about "pg_xlogfile_name() cannot be executed during recovery."?

OK. Here is the revised patch.

Regards,

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

Attachments:

forbid_pg_xlogfile_name_during_recovery_v2.patchapplication/octet-stream; name=forbid_pg_xlogfile_name_during_recovery_v2.patchDownload
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***************
*** 13064,13071 **** SELECT set_config('log_statement_stats', 'off', false);
     <para>
      The functions shown in <xref
      linkend="functions-admin-backup-table"> assist in making on-line backups.
!     Use of the first three functions is restricted to superusers. The first
!     five functions cannot be executed during recovery.
     </para>
  
     <table id="functions-admin-backup-table">
--- 13064,13071 ----
     <para>
      The functions shown in <xref
      linkend="functions-admin-backup-table"> assist in making on-line backups.
!     These functions cannot be executed during recovery.
!     Use of the first three functions is restricted to superusers.
     </para>
  
     <table id="functions-admin-backup-table">
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***************
*** 8410,8415 **** pg_xlogfile_name_offset(PG_FUNCTION_ARGS)
--- 8410,8421 ----
  	HeapTuple	resultHeapTuple;
  	Datum		result;
  
+ 	if (RecoveryInProgress())
+ 		ereport(ERROR,
+ 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ 				 errmsg("recovery is in progress"),
+ 				 errhint("pg_xlogfile_name_offset() cannot be executed during recovery.")));
+ 
  	/*
  	 * Read input and parse
  	 */
***************
*** 8479,8484 **** pg_xlogfile_name(PG_FUNCTION_ARGS)
--- 8485,8496 ----
  	XLogRecPtr	locationpoint;
  	char		xlogfilename[MAXFNAMELEN];
  
+ 	if (RecoveryInProgress())
+ 		ereport(ERROR,
+ 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ 				 errmsg("recovery is in progress"),
+ 				 errhint("pg_xlogfile_name() cannot be executed during recovery.")));
+ 
  	locationstr = text_to_cstring(location);
  
  	if (sscanf(locationstr, "%X/%X", &uxlogid, &uxrecoff) != 2)
#9Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Fujii Masao (#8)
Re: pending patch: Re: Streaming replication and pg_xlogfile_name()

Fujii Masao wrote:

On Tue, Apr 6, 2010 at 11:48 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Fujii Masao wrote:

On Tue, Apr 6, 2010 at 8:02 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

Should we throw an error in pg_xlogfile_name() if called during
recovery? It's not doing anything useful as it is.

I have no objection for now.

Here is the patch.
...
+     if (RecoveryInProgress())
+             ereport(ERROR,
+                             (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                              errmsg("recovery is in progress"),
+                              errhint("WAL control functions cannot be executed during recovery.")));
+

The hint is a bit confusing for pg_xlogfile_name(). pg_xlogfile_name()
is hardly a WAL control function like pg_start/stop_backup() are. How
about "pg_xlogfile_name() cannot be executed during recovery."?

OK. Here is the revised patch.

Thanks, committed.

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