Обсуждение: pending patch: Re: Streaming replication and pg_xlogfile_name()

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

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

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

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

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

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

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

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

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