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

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.

Modified Files:
--------------
pgsql/doc/src/sgml:
func.sgml (r1.512 -> r1.513)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/func.sgml?r1=1.512&r2=1.513)
pgsql/src/backend/access/transam:
xlog.c (r1.389 -> r1.390)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xlog.c?r1=1.389&r2=1.390)

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?

--
Simon Riggs www.2ndQuadrant.com

(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.

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.

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

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

On Wed, Apr 07, 2010 at 06:12:52AM +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.

Is this Fujii Masao's patch? If so, please make sure to mention this
in your commit messages :)

Cheers,
David.
>
> Modified Files:
> --------------
> pgsql/doc/src/sgml:
> func.sgml (r1.512 -> r1.513)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/func.sgml?r1=1.512&r2=1.513)
> pgsql/src/backend/access/transam:
> xlog.c (r1.389 -> r1.390)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xlog.c?r1=1.389&r2=1.390)
>
> --
> Sent via pgsql-committers mailing list (pgsql-committers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-committers

--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter wrote:
> On Wed, Apr 07, 2010 at 06:12:52AM +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.
>
> Is this Fujii Masao's patch? If so, please make sure to mention this
> in your commit messages :)

Yes. Sorry.

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

On Wed, Apr 7, 2010 at 7:23 PM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> 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.

Why? The tli of the last WAL record received is always the
recovery target tli currently. So using GetRecoveryTargetTLI()
for pg_last_xlog_receive_location() seems OK for me.
Am I missing something?

Regards,

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

Fujii Masao wrote:
> On Wed, Apr 7, 2010 at 7:23 PM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> 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.
>
> Why? The tli of the last WAL record received is always the
> recovery target tli currently.

True.

Hmm, currently pg_last_xlog_receive_location() returns the last location
streamed via streaming replication. Should that be changed so that it
also advances when a WAL segment is restored from archive? It seems
strange that pg_last_xlog_receive_location() can be smaller than
pg_last_xlog_replay_location().

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

On Thu, 2010-04-08 at 09:54 +0300, Heikki Linnakangas wrote:
> Fujii Masao wrote:
> > On Wed, Apr 7, 2010 at 7:23 PM, Heikki Linnakangas
> > <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> >> 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.
> >
> > Why? The tli of the last WAL record received is always the
> > recovery target tli currently.
>
> True.

Only in streaming mode. If you use the current TLI as I have suggested
then it will be correct in more cases.

> Hmm, currently pg_last_xlog_receive_location() returns the last location
> streamed via streaming replication. Should that be changed so that it
> also advances when a WAL segment is restored from archive? It seems
> strange that pg_last_xlog_receive_location() can be smaller than
> pg_last_xlog_replay_location().

Yes, it should be changed.

--
Simon Riggs www.2ndQuadrant.com

On Thu, Apr 8, 2010 at 4:06 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Thu, 2010-04-08 at 09:54 +0300, Heikki Linnakangas wrote:
>> Fujii Masao wrote:
>> > On Wed, Apr 7, 2010 at 7:23 PM, Heikki Linnakangas
>> > <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> >> 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.
>> >
>> > Why? The tli of the last WAL record received is always the
>> > recovery target tli currently.
>>
>> True.
>
> Only in streaming mode. If you use the current TLI as I have suggested
> then it will be correct in more cases.

pg_last_xlog_receive_location() might be executed also after archive
recovery ends. In this case, using the current tli seems not correct
because it's always different from the recovery target tli after recovery.

>> Hmm, currently pg_last_xlog_receive_location() returns the last location
>>  streamed via streaming replication. Should that be changed so that it
>> also advances when a WAL segment is restored from archive? It seems
>> strange that pg_last_xlog_receive_location() can be smaller than
>> pg_last_xlog_replay_location().
>
> Yes, it should be changed.

Should it advance when WAL file in pg_xlog is read? If not,
pg_last_xlog_receive_location() can be smaller than
pg_last_xlog_replay_location().

And, how far should it advance when WAL file is
partially-filled for some reasons?

Regards,

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

On Thu, 2010-04-08 at 16:41 +0900, Fujii Masao wrote:
> >> > Why? The tli of the last WAL record received is always the
> >> > recovery target tli currently.
> >>
> >> True.
> >
> > Only in streaming mode. If you use the current TLI as I have suggested
> > then it will be correct in more cases.
>
> pg_last_xlog_receive_location() might be executed also after archive
> recovery ends. In this case, using the current tli seems not correct
> because it's always different from the recovery target tli after recovery.

Which is why the code I write says "if (RecoveryInProgress())"

> >> Hmm, currently pg_last_xlog_receive_location() returns the last location
> >> streamed via streaming replication. Should that be changed so that it
> >> also advances when a WAL segment is restored from archive? It seems
> >> strange that pg_last_xlog_receive_location() can be smaller than
> >> pg_last_xlog_replay_location().
> >
> > Yes, it should be changed.
>
> Should it advance when WAL file in pg_xlog is read? If not,
> pg_last_xlog_receive_location() can be smaller than
> pg_last_xlog_replay_location().
>
> And, how far should it advance when WAL file is
> partially-filled for some reasons?

Just make pg_last_xlog_receive_location() do exactly the same thing as
pg_last_xlog_replay_location() when working with files. No need to try
to keep two things exactly in sync.

--
Simon Riggs www.2ndQuadrant.com