Re: pg_resetxlog to clear backup start/end locations.

Hello, this is a patch that add the function to clear backup
location information to pg_resetxlog.

As per the discussion held before, this function cannot be back
patched to the older versions than 9.4. And it also slipped over
9.4 so proposed in this CF.

This simplly erases the backup location information in pg_control
written during recovery. This is seen as the output of
pg_controldata.

| $ pg_controldata
| pg_control version number: 942
| Catalog version number: 201406121
| ...
!| Backup start location: 0/0
!| Backup end location: 0/0
!| End-of-backup record required: no
| ...

Under some condition, this values sticks having valid values even
though the WAL record which indicates the end of backup won't
come. This option could be used to forcibly finish the blocked
recovery procedure.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Jun 13, 2014 at 5:08 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello, this is a patch that add the function to clear backup
> location information to pg_resetxlog.
>
> As per the discussion held before, this function cannot be back
> patched to the older versions than 9.4. And it also slipped over
> 9.4 so proposed in this CF.
>
> This simplly erases the backup location information in pg_control
> written during recovery. This is seen as the output of
> pg_controldata.
>
> | $ pg_controldata
> | pg_control version number: 942
> | Catalog version number: 201406121
> | ...
> !| Backup start location: 0/0
> !| Backup end location: 0/0
> !| End-of-backup record required: no
> | ...
>
> Under some condition, this values sticks having valid values even
> though the WAL record which indicates the end of backup won't
> come. This option could be used to forcibly finish the blocked
> recovery procedure.

The document about pg_resetxlog needs to be updated.

I think that pg_resetxlog should reset backup locations by default
since they are useless (rather harmful) after pg_resetxlog. Thought?

Regards,

--
Fujii Masao

On 13 June 2014 12:27, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> I think that pg_resetxlog should reset backup locations by default
> since they are useless (rather harmful) after pg_resetxlog. Thought?

+1

Do we regard that point as a bug that should be backpatched?

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Jun 22, 2014 at 8:54 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 13 June 2014 12:27, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
>> I think that pg_resetxlog should reset backup locations by default
>> since they are useless (rather harmful) after pg_resetxlog. Thought?
>
> +1
>
> Do we regard that point as a bug that should be backpatched?

Yep, I think so.

Regards,

--
Fujii Masao

Hello, thank you for the comments.

> On Sun, Jun 22, 2014 at 8:54 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On 13 June 2014 12:27, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> >
> >> I think that pg_resetxlog should reset backup locations by default
> >> since they are useless (rather harmful) after pg_resetxlog. Thought?
> >
> > +1
> >
> > Do we regard that point as a bug that should be backpatched?
>
> Yep, I think so.

... Ouch, I was too short-sighted :( That is pretty natural to do
so after hearing that. I should have pointed this at the previous
discusion.

I assume the primary usage of this patch to be, as described
before, Dissolving a recovery freezing caused by wrongly placed
backup label. Crash recovery has been already done at that time
so resetxlog's current behavior seems also fittin the situation,
I suppose.

Ok, I'm doing modify it to reset backup locations by default and
remove the new option '-b' to do that. Since this seems looking
to be a bug for the poeple, I'll provide backpatches back
to... 8.4? (Final release of 8.4 is scheduled at July 2014)

Concerning documentation, I see no need to edit it if no one
wants it more detailed after the visible option has been called
off.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Jun 23, 2014 at 3:49 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello, thank you for the comments.
>
>> On Sun, Jun 22, 2014 at 8:54 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> > On 13 June 2014 12:27, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> >
>> >> I think that pg_resetxlog should reset backup locations by default
>> >> since they are useless (rather harmful) after pg_resetxlog. Thought?
>> >
>> > +1
>> >
>> > Do we regard that point as a bug that should be backpatched?
>>
>> Yep, I think so.
>
> ... Ouch, I was too short-sighted :( That is pretty natural to do
> so after hearing that. I should have pointed this at the previous
> discusion.
>
> I assume the primary usage of this patch to be, as described
> before, Dissolving a recovery freezing caused by wrongly placed
> backup label. Crash recovery has been already done at that time
> so resetxlog's current behavior seems also fittin the situation,
> I suppose.

One question is; is there case where a user wants to reset only
backup locations? I'm not sure if there are such cases. If they exist,
probably we should implement new option which resets only backup
locations. Thought?

> Ok, I'm doing modify it to reset backup locations by default and
> remove the new option '-b' to do that. Since this seems looking
> to be a bug for the poeple, I'll provide backpatches back
> to... 8.4? (Final release of 8.4 is scheduled at July 2014)

I was thinking that we don't need to backpatch this to 8.4 because
8.4 doesn't have any backup locations. No?

Regards,

--
Fujii Masao

Hi,

At Mon, 23 Jun 2014 17:10:05 +0900, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote in <CAHGQGwFy_CDmfURiu6ZOaT2hTQo_eiJAJ7vWEWysOL15ocTAdg(at)mail(dot)gmail(dot)com>
> > I assume the primary usage of this patch to be, as described
> > before, Dissolving a recovery freezing caused by wrongly placed
> > backup label. Crash recovery has been already done at that time
> > so resetxlog's current behavior seems also fittin the situation,
> > I suppose.
>
> One question is; is there case where a user wants to reset only
> backup locations? I'm not sure if there are such cases. If they exist,
> probably we should implement new option which resets only backup
> locations. Thought?

As I described as above, I don't see obvious use case where ONLY
backup location is needed to be resetted. The option you proposed
not only clears backup locations but also inhibits resetting xlog
infos which would be done without the option. The behavior would
puzzle users.

> > Ok, I'm doing modify it to reset backup locations by default and
> > remove the new option '-b' to do that. Since this seems looking
> > to be a bug for the poeple, I'll provide backpatches back
> > to... 8.4? (Final release of 8.4 is scheduled at July 2014)
>
> I was thinking that we don't need to backpatch this to 8.4 because
> 8.4 doesn't have any backup locations. No?

I'm not so knowledgeable about ancint(?) specs:p Of course this
phrase means "back to 8.4 that this patch has any meaning".

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

> Ok, I'm doing modify it to reset backup locations by default and
> remove the new option '-b' to do that. Since this seems looking
> to be a bug for the poeple, I'll provide backpatches back
> to... 8.4? (Final release of 8.4 is scheduled at July 2014)

I looked closer to pg_resetxlog and found I'm feeling discomfort
of its behavior.

"pg_resetxlog -n", which is explained in its help that "no
update, just show what would be done (for testing) ", shows
current values then values to be changed, like this.

| Current pg_control values:
|
| pg_control version number: 942
| Catalog version number: 201406181
| Database system identifier: 6026883474640211951
| Latest checkpoint's TimeLineID: 1
| Latest checkpoint's full_page_writes: on
| Latest checkpoint's NextXID: 0/1831
| Latest checkpoint's NextOID: 32772
| Latest checkpoint's NextMultiXactId: 1
| Latest checkpoint's NextMultiOffset: 0
| Latest checkpoint's oldestXID: 1800
| Latest checkpoint's oldestXID's DB: 1
| Latest checkpoint's oldestActiveXID: 0
| Latest checkpoint's oldestMultiXid: 1
| Latest checkpoint's oldestMulti's DB: 1
| Maximum data alignment: 8
...
| Values to be changed:
|
| First log segment after reset: 00000001000000000000000C

But "pg_resetxlog -f" made the change as below,

| *** /tmp/1.txt 2014-06-25 10:49:42.269336739 +0900
| --- /tmp/2.txt 2014-06-25 10:49:49.780266229 +0900
| ***************
| *** 5,9 ****
| ! pg_control last modified: Wed 25 Jun 2014 10:45:20 AM JST
| ! Latest checkpoint location: 0/B8DE1A0
| ! Prior checkpoint location: 0/B8DE138
| ! Latest checkpoint's REDO location: 0/B8DE1A0
| ! Latest checkpoint's REDO WAL file: 00000001000000000000000B
| --- 5,9 ----
| ! pg_control last modified: Wed 25 Jun 2014 10:49:46 AM JST
| ! Latest checkpoint location: 0/C000028
| ! Prior checkpoint location: 0/0
| ! Latest checkpoint's REDO location: 0/C000028
| ! Latest checkpoint's REDO WAL file: 00000001000000000000000C
| ***************
| *** 22 ****
| ! Time of latest checkpoint: Wed 25 Jun 2014 10:45:20 AM JST
| --- 22 ----
| ! Time of latest checkpoint: Wed 25 Jun 2014 10:49:46 AM JST

There are some changes which haven't been shown by '-n' option,
even not displayed at all. I think these should be shown by
'-n'. I suppose this is a kind of bug but fixing it seems to be a
kind of 'feature change'..

Any suggestions?

reagrds,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Jun 25, 2014 at 11:13 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello,
>
>> Ok, I'm doing modify it to reset backup locations by default and
>> remove the new option '-b' to do that. Since this seems looking
>> to be a bug for the poeple, I'll provide backpatches back
>> to... 8.4? (Final release of 8.4 is scheduled at July 2014)
>
> I looked closer to pg_resetxlog and found I'm feeling discomfort
> of its behavior.
>
> "pg_resetxlog -n", which is explained in its help that "no
> update, just show what would be done (for testing) ", shows
> current values then values to be changed, like this.
>
> | Current pg_control values:
> |
> | pg_control version number: 942
> | Catalog version number: 201406181
> | Database system identifier: 6026883474640211951
> | Latest checkpoint's TimeLineID: 1
> | Latest checkpoint's full_page_writes: on
> | Latest checkpoint's NextXID: 0/1831
> | Latest checkpoint's NextOID: 32772
> | Latest checkpoint's NextMultiXactId: 1
> | Latest checkpoint's NextMultiOffset: 0
> | Latest checkpoint's oldestXID: 1800
> | Latest checkpoint's oldestXID's DB: 1
> | Latest checkpoint's oldestActiveXID: 0
> | Latest checkpoint's oldestMultiXid: 1
> | Latest checkpoint's oldestMulti's DB: 1
> | Maximum data alignment: 8
> ...
> | Values to be changed:
> |
> | First log segment after reset: 00000001000000000000000C
>
> But "pg_resetxlog -f" made the change as below,
>
> | *** /tmp/1.txt 2014-06-25 10:49:42.269336739 +0900
> | --- /tmp/2.txt 2014-06-25 10:49:49.780266229 +0900
> | ***************
> | *** 5,9 ****
> | ! pg_control last modified: Wed 25 Jun 2014 10:45:20 AM JST
> | ! Latest checkpoint location: 0/B8DE1A0
> | ! Prior checkpoint location: 0/B8DE138
> | ! Latest checkpoint's REDO location: 0/B8DE1A0
> | ! Latest checkpoint's REDO WAL file: 00000001000000000000000B
> | --- 5,9 ----
> | ! pg_control last modified: Wed 25 Jun 2014 10:49:46 AM JST
> | ! Latest checkpoint location: 0/C000028
> | ! Prior checkpoint location: 0/0
> | ! Latest checkpoint's REDO location: 0/C000028
> | ! Latest checkpoint's REDO WAL file: 00000001000000000000000C
> | ***************
> | *** 22 ****
> | ! Time of latest checkpoint: Wed 25 Jun 2014 10:45:20 AM JST
> | --- 22 ----
> | ! Time of latest checkpoint: Wed 25 Jun 2014 10:49:46 AM JST
>
> There are some changes which haven't been shown by '-n' option,
> even not displayed at all. I think these should be shown by
> '-n'. I suppose this is a kind of bug but fixing it seems to be a
> kind of 'feature change'..
>
> Any suggestions?

This seems the problem of the document and the help message of -n option.
According to the source code, -n option displays only the values that -e, -l,
-m -o, -O, and -x options change. The values -f option forcibly changes are
not be shown in -n option. I'm not sure if this is an oversight in 108e399...

Anyway, I think that making -n option display all the values that -f option
changes would be useful. But since that's not a bugfix, we should apply it
only in HEAD.

Regards,

--
Fujii Masao

Hello, thank you for the suggestion.

I dont' touch what '-n' option shows and rewrite documents for
the option a bit. And '-n' won't show the changes of backup
location.

=======
> > There are some changes which haven't been shown by '-n' option,
> > even not displayed at all. I think these should be shown by
> > '-n'. I suppose this is a kind of bug but fixing it seems to be a
> > kind of 'feature change'..
> >
> > Any suggestions?
>
> This seems the problem of the document and the help message of -n option.
> According to the source code, -n option displays only the values that -e, -l,
> -m -o, -O, and -x options change. The values -f option forcibly changes are
> not be shown in -n option. I'm not sure if this is an oversight in 108e399...

The html(sgml) document says that,

=== share/doc/html/app-pgresetxlog.html
| The -n (no operation) option instructs pg_resetxlog to print
| the values reconstructed from pg_control and values about to be
| changed, and then exit without modifying anything. This is
| mainly a debugging tool, but can be useful as a sanity check
| before allowing pg_resetxlog to proceed for real.

This seems to have same meaning to the help message. "For
debugging use" also supports your way of understanding the
option, I suppose.

> Anyway, I think that making -n option display all the values that -f option
> changes would be useful. But since that's not a bugfix, we should apply it
> only in HEAD.

Agreed.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello, I have finished the patches for all of 9.x.

> I dont' touch what '-n' option shows and rewrite documents for
> the option a bit. And '-n' won't show the changes of backup
> location.

-8.4: does not have backup locations in ControlFileData.

9.0-9.1: resetxlog_backuploc_9_0-9.1.patch: Add clearance of backupStartPoint.

9.2: resetxlog_backuploc_9_2.patch: Add clearance of
backupStart/EndPoint and backupEndRequired

9.3: resetxlog_backuploc_9_3.patch: Ditto. (format of XLogRecPtr changed)

9.4-master: resetxlog_backuploc_9_4-master.patch: Add clearance of
backupPoints. help message and html doc changed.

With these patches, pg_resetxlog saves the stuck after recovery
failure with wrongly placed backup label.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Jun 27, 2014 at 12:29 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello, I have finished the patches for all of 9.x.
>
>> I dont' touch what '-n' option shows and rewrite documents for
>> the option a bit. And '-n' won't show the changes of backup
>> location.
>
> -8.4: does not have backup locations in ControlFileData.
>
> 9.0-9.1: resetxlog_backuploc_9_0-9.1.patch: Add clearance of backupStartPoint.
>
> 9.2: resetxlog_backuploc_9_2.patch: Add clearance of
> backupStart/EndPoint and backupEndRequired
>
> 9.3: resetxlog_backuploc_9_3.patch: Ditto. (format of XLogRecPtr changed)
>
> 9.4-master: resetxlog_backuploc_9_4-master.patch: Add clearance of
> backupPoints. help message and html doc changed.

Thanks for the patch! But when I read the source code of pg_resetxlog,
I found that it has already reset the backup locations. Please see
RewriteControlFile() which does that. So I wonder if we need nothing
at least in HEAD for the purpose which you'd like to achieve. Thought?

Regards,

--
Fujii Masao

Mmm. This patch is found that useless, from the first.

> Thanks for the patch! But when I read the source code of pg_resetxlog,
> I found that it has already reset the backup locations. Please see
> RewriteControlFile() which does that. So I wonder if we need nothing
> at least in HEAD for the purpose which you'd like to achieve. Thought?

Thank you for noticing of that, I checked by myself and found
that what this patch intended is already done in all
origin/REL9_x_STABLE. What is more, that code has not changed for
over hundreds of commits on each branch, that is, maybe from the
first. I don't know how I caught in such a stupid
misunderstanding, but all these patches are totally useless.

Sorry for taking your time for such a useless thing and thank you
for your kindness.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Jun 30, 2014 at 12:49 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Mmm. This patch is found that useless, from the first.
>
>> Thanks for the patch! But when I read the source code of pg_resetxlog,
>> I found that it has already reset the backup locations. Please see
>> RewriteControlFile() which does that. So I wonder if we need nothing
>> at least in HEAD for the purpose which you'd like to achieve. Thought?
>
> Thank you for noticing of that, I checked by myself and found
> that what this patch intended is already done in all
> origin/REL9_x_STABLE. What is more, that code has not changed for
> over hundreds of commits on each branch, that is, maybe from the
> first. I don't know how I caught in such a stupid
> misunderstanding, but all these patches are totally useless.

So we should mark this patch as "Rejected Patches"?

> Sorry for taking your time for such a useless thing and thank you
> for your kindness.

No problem!

Regards,

--
Fujii Masao

Hello,

> > misunderstanding, but all these patches are totally useless.
>
> So we should mark this patch as "Rejected Patches"?

I think so.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

At 2014-06-30 13:55:59 +0900, horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp wrote:
>
> > So we should mark this patch as "Rejected Patches"?
>
> I think so.

Done.

-- Abhijit

On Wed, Jun 25, 2014 at 02:08:26PM +0900, Kyotaro HORIGUCHI wrote:
> Hello, thank you for the suggestion.
>
> I dont' touch what '-n' option shows and rewrite documents for
> the option a bit. And '-n' won't show the changes of backup
> location.
>
> =======
> > > There are some changes which haven't been shown by '-n' option,
> > > even not displayed at all. I think these should be shown by
> > > '-n'. I suppose this is a kind of bug but fixing it seems to be a
> > > kind of 'feature change'..
> > >
> > > Any suggestions?
> >
> > This seems the problem of the document and the help message of -n option.
> > According to the source code, -n option displays only the values that -e, -l,
> > -m -o, -O, and -x options change. The values -f option forcibly changes are
> > not be shown in -n option. I'm not sure if this is an oversight in 108e399...
>
> The html(sgml) document says that,
>
> === share/doc/html/app-pgresetxlog.html
> | The -n (no operation) option instructs pg_resetxlog to print
> | the values reconstructed from pg_control and values about to be
> | changed, and then exit without modifying anything. This is
> | mainly a debugging tool, but can be useful as a sanity check
> | before allowing pg_resetxlog to proceed for real.
>
> This seems to have same meaning to the help message. "For
> debugging use" also supports your way of understanding the
> option, I suppose.
>
> > Anyway, I think that making -n option display all the values that -f option
> > changes would be useful. But since that's not a bugfix, we should apply it
> > only in HEAD.
>
> Agreed.

Is this a TODO item?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Hello,

> > > Anyway, I think that making -n option display all the values that -f option
> > > changes would be useful. But since that's not a bugfix, we should apply it
> > > only in HEAD.
> >
> > Agreed.
>
> Is this a TODO item?

It's not that. The 'bug' was my wrong guess and I've found that
it is done by default from the first..

I'm sorry to have bothered you.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center