Обсуждение: pg_xlogfile_name_offset() et al and recovery

Currently in HEAD and 9.6, one can issue a non-exclusive backup on
standby, so this is OK:

select pg_is_in_recovery();pg_is_in_recovery
-------------------t
(1 row)

select pg_start_backup('sby-bkp-test', 'f', 'f');pg_start_backup
-----------------0/5000220
(1 row)

However the following happens:

select pg_xlogfile_name_offset(pg_start_backup('sby-bkp-test', 'f', 'f'));
ERROR:  recovery is in progress
HINT:  pg_xlogfile_name_offset() cannot be executed during recovery.

Should this restriction be relaxed or am I missing something?

Thanks,
Amit

On 2016/05/19 17:34, Amit Langote wrote:
> Currently in HEAD and 9.6, one can issue a non-exclusive backup on
> standby, so this is OK:
> 
> select pg_is_in_recovery();
>  pg_is_in_recovery
> -------------------
>  t
> (1 row)
> 
> select pg_start_backup('sby-bkp-test', 'f', 'f');
>  pg_start_backup
> -----------------
>  0/5000220
> (1 row)
> 
> However the following happens:
> 
> select pg_xlogfile_name_offset(pg_start_backup('sby-bkp-test', 'f', 'f'));
> ERROR:  recovery is in progress
> HINT:  pg_xlogfile_name_offset() cannot be executed during recovery.
> 
> Should this restriction be relaxed or am I missing something?

Answering my own question:

While reading the thread "BUG #14230: Wrong timeline returned by
pg_stop_backup on a standby", I came to know that ThisTimelineId is
invalid on standby.  And because pg_xlogfile_name_offset() uses the same
to compute its result, it makes sense to prevent it from being used on a
standby.

Thanks,
Amit

On Thu, Jul 7, 2016 at 3:59 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> While reading the thread "BUG #14230: Wrong timeline returned by
> pg_stop_backup on a standby", I came to know that ThisTimelineId is
> invalid on standby.  And because pg_xlogfile_name_offset() uses the same
> to compute its result, it makes sense to prevent it from being used on a
> standby.

To be honest, I have always found that this restriction was hard to
justify on a function that basically performs a static calculation. So
what about removing this error and use GetXLogReplayRecPtr() to fetch
the timeline ID? Per se the patch attached:
=# select * from pg_xlogfile_name_offset(pg_last_xlog_replay_location());
        file_name         | file_offset
--------------------------+-------------
 000000010000000000000003 |        2192
(1 row)

The same applies of course to pg_xlogfile_name():
=# select pg_xlogfile_name(pg_last_xlog_replay_location());
     pg_xlogfile_name
--------------------------
 000000010000000000000003
(1 row)
--
Michael

On 2016/07/07 16:26, Michael Paquier wrote:
> On Thu, Jul 7, 2016 at 3:59 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> While reading the thread "BUG #14230: Wrong timeline returned by
>> pg_stop_backup on a standby", I came to know that ThisTimelineId is
>> invalid on standby.  And because pg_xlogfile_name_offset() uses the same
>> to compute its result, it makes sense to prevent it from being used on a
>> standby.
> 
> To be honest, I have always found that this restriction was hard to
> justify on a function that basically performs a static calculation. So
> what about removing this error and use GetXLogReplayRecPtr() to fetch
> the timeline ID? Per se the patch attached:
> =# select * from pg_xlogfile_name_offset(pg_last_xlog_replay_location());
>         file_name         | file_offset
> --------------------------+-------------
>  000000010000000000000003 |        2192
> (1 row)
> 
> The same applies of course to pg_xlogfile_name():
> =# select pg_xlogfile_name(pg_last_xlog_replay_location());
>      pg_xlogfile_name
> --------------------------
>  000000010000000000000003
> (1 row)

+1 to enabling these functions' usage on standby and the patch.

Thanks,
Amit

At Thu, 7 Jul 2016 16:48:57 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<3649f1e4-ac74-841e-6ae5-cfe0022aa50b@lab.ntt.co.jp>
> On 2016/07/07 16:26, Michael Paquier wrote:
> > On Thu, Jul 7, 2016 at 3:59 PM, Amit Langote
> > <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> >> While reading the thread "BUG #14230: Wrong timeline returned by
> >> pg_stop_backup on a standby", I came to know that ThisTimelineId is
> >> invalid on standby.  And because pg_xlogfile_name_offset() uses the same
> >> to compute its result, it makes sense to prevent it from being used on a
> >> standby.
> > 
> > To be honest, I have always found that this restriction was hard to
> > justify on a function that basically performs a static calculation. So
> > what about removing this error and use GetXLogReplayRecPtr() to fetch
> > the timeline ID? Per se the patch attached:
> > =# select * from pg_xlogfile_name_offset(pg_last_xlog_replay_location());
> >         file_name         | file_offset
> > --------------------------+-------------
> >  000000010000000000000003 |        2192
> > (1 row)
> > 
> > The same applies of course to pg_xlogfile_name():
> > =# select pg_xlogfile_name(pg_last_xlog_replay_location());
> >      pg_xlogfile_name
> > --------------------------
> >  000000010000000000000003
> > (1 row)
> 
> +1 to enabling these functions' usage on standby and the patch.

Strongly agreed. At first I thought that using replay-loc TLI is
bogus but ThisTimeLineID for non-recovery time is also bogus at
almost the same degree.

> =# select * from pg_xlogfile_name_offset('0/100'::pg_lsn);
>         file_name         | file_offset 
> --------------------------+-------------
>  000000030000000000000000 |         256

The rest of the "almost" is master's timeline evolution. A master
won't get timeline evolution during runtime but a standby can do.

As the result, this relaxation adds more good than bad and I
agree to this.

Addition to that, I'd like to propose to add a description (or a
notice) about this restriction in documentation that the timeline
part of the file name may be wrong. Will post soon.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

I wrote a bit wrong thing.

At Thu, 07 Jul 2016 17:20:52 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20160707.172052.213690356.horiguchi.kyotaro@lab.ntt.co.jp>
> > > To be honest, I have always found that this restriction was hard to
> > > justify on a function that basically performs a static calculation. So
> > > what about removing this error and use GetXLogReplayRecPtr() to fetch
> > > the timeline ID? Per se the patch attached:
> > > =# select * from pg_xlogfile_name_offset(pg_last_xlog_replay_location());
> > >         file_name         | file_offset
> > > --------------------------+-------------
> > >  000000010000000000000003 |        2192
> > > (1 row)
> > > 
> > > The same applies of course to pg_xlogfile_name():
> > > =# select pg_xlogfile_name(pg_last_xlog_replay_location());
> > >      pg_xlogfile_name
> > > --------------------------
> > >  000000010000000000000003
> > > (1 row)
> > 
> > +1 to enabling these functions' usage on standby and the patch.
> 
> Strongly agreed. At first I thought that using replay-loc TLI is
> bogus but ThisTimeLineID for non-recovery time is also bogus at
> almost the same degree.
> 
> > =# select * from pg_xlogfile_name_offset('0/100'::pg_lsn);
> >         file_name         | file_offset 
> > --------------------------+-------------
> >  000000030000000000000000 |         256
> 
> The rest of the "almost" is master's timeline evolution. A master

It makes non sense. should be the follwoing,

| The rest of the "almost" is the upstreams' timeline evolution
| and promotion of itself. A master


> won't get timeline evolution during runtime but a standby can do.
> 
> As the result, this relaxation adds more good than bad and I
> agree to this.
> 
> Addition to that, I'd like to propose to add a description (or a
> notice) about this restriction in documentation that the timeline
> part of the file name may be wrong. Will post soon.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

At Thu, 07 Jul 2016 17:26:15 +0900, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20160707.172615.232806225.horiguchi.kyotaro@lab.ntt.co.jp>
> > As the result, this relaxation adds more good than bad and I
> > agree to this.
> > 
> > Addition to that, I'd like to propose to add a description (or a
> > notice) about this restriction in documentation that the timeline
> > part of the file name may be wrong. Will post soon.

The following sentense will be added by this patch. Does it make sense?

| Note that these two functions use the timeline ID at the time
| to construct the file name. Therefore they give wrong names for
| transaction log locations on the other timelines. This can
| happen during runtime on replication standby when a promotion
| occurs on itself or upstreams.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index baef80d..451f0ec 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17796,7 +17796,11 @@ postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());    these functions return
thename of the preceding transaction log file.    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.
+    needs to be archived. Note that these two functions use the timeline ID at
+    the time to construct the file name. Therefore they give wrong names for
+    transaction log locations on the other timelines. This can happen during
+    runtime on replication standby when a promotion occurs on itself or
+    upstreams.   </para>   <para>

On Thu, Jul 7, 2016 at 6:26 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> The behaviour of that function is defined in backbranches, so I suggest we
> should not alter that now.

Hm..

> What we can do is have another function that makes it clearer that the
> answer is variable.
>    pg_xlogfile_name_offset(offset, timelineid)
> always succeeds on standby but needs to be told what timelineid to use

This has clearly value, I have wanted to override the timeline number
a couple of times now. I have always done so with a custom function
and saw many people doing it at application level, but it is a weird
design to have the 1-argument version fail for a node in recovery, and
the 2-argument version succeed. I'd rather have everything consistent
to be honest, with the same function name, and the behavior clearly
documented.

> then we have another function
>   pg_last_xact_replay_timeline() that allows you to find out the current
> timeline

It would be good to have an equivalent pg_current_xlog_timeline as
well that can run on nodes not in recovery. I agree that having a
separate function makes more sense as its equivalents for WAL
positions.

> The actual problem here is somewhat more convoluted, because we would like
> to know the timeline of the basebackup at start and stop. That information
> is not easily available from the backup label data returned by
> pg_stop_backup().
> We would like to do something like this...
>
> select pg_xlogfile_name_offset(l.lsn, l.tli) from pg_stop_backup(false) l;
>
> So I suggest we add another column to the output of pg_stop_backup() to
> return the tli value directly.

This would be useful as well, that's less custom-parsing logic for
applications based on the segment name in backup_label.

Note that I am not personally planning to write a patch for all that,
but any people are welcome to pick up this task!
-- 
Michael

On Thu, Jul 7, 2016 at 5:26 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> The behaviour of that function is defined in backbranches, so I suggest we
> should not alter that now.

+1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company