Re: [COMMITTERS] pgsql: Forbid using pg_xlogfile_name() and pg_xlogfile_name_offset()

On Wed, 2010-04-07 at 13:23 +0300, Heikki Linnakangas wrote:
> (moving to pgsql-hackers)
> 
> Simon Riggs wrote:
> > On Wed, 2010-04-07 at 06:12 +0000, Heikki Linnakangas wrote:
> >> Log Message:
> >> -----------
> >> Forbid using pg_xlogfile_name() and pg_xlogfile_name_offset() during
> >> recovery. We might want to relax this in the future, but ThisTimeLineID
> >> isn't currently correct in backends during recovery, so the filename
> >> returned was wrong.
> > 
> > Any reason why we couldn't just do this:
> > 
> > if (RecoveryInProgress())
> > {
> >     volatile XLogCtlData *xlogctl = XLogCtl;
> >     XLogFileName(xlogfilename, xlogctl->ThisTimeLineID, 
> >             xlogid, xlogseg);
> > }
> > else
> >     XLogFileName(xlogfilename, ThisTimeLineID, xlogid, xlogseg);
> > 
> > 
> > rather than preventing access to those functions completely?
> 
> Because if you do something like
> pg_xlogfile_name(pg_last_xlog_receive_location()),
> xloctl->ThisTimeLineId would not necessarily be the timeline
> corresponding the last received location. Even with
> pg_xlogfile_name(pg_last_xlog_replay_location()), there's a small race
> condition between those calls; if a checkpoint record is replayed in
> between that changes timeline, the returned filename doesn't correspond
> the name of the file where the replayed WAL record was read from, as you
> would expect.

If timelineId changed in normal operation, I'd be inclined to agree this
was a problem. It hardly ever changes, and can only change on standby
when that server is not yet streaming.

I'd rather have a function with a rare and documented weirdness, than no
function at all.

> This commit is a stop-gap solution until we figure out what exactly to
> do about that. Masao-san wrote a patch that included the TLI in the
> string returned by pg_last_xlog_receive/replay_location() (see
> http://archives.postgresql.org/message-id/3f0b79eb1003030603ibd0cbadjebb09fa4249304ba@mail.gmail.com
> and
> http://archives.postgresql.org/message-id/3f0b79eb1003300214r6cf98c46tc9be5d563ccf48db@mail.gmail.com),
> but it still wasn't clear it did the right thing in corner-cases where
> the TLI changes. 

> Using GetRecoveryTargetTLI() for the tli returned by
> pg_last_receive_location() seems bogus, at least.

Agree with that, using the current value makes most sense 
xlogctl->ThisTimeLineID

-- Simon Riggs           www.2ndQuadrant.com