Prevent restored WAL files from being archived again Re: Unnecessary WAL archiving after failover

On Tue, Jun 5, 2012 at 3:37 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Fri, Mar 23, 2012 at 11:03:27PM +0900, Fujii Masao wrote:
>> (2) WAL files which were restored from the archive
>>
>> In 9.1 or before, the restored WAL files don't remain after failover
>> because they are always restored onto the temporary filename
>> "RECOVERYXLOG". So the issue which I explain from now doesn't exist
>> in 9.1 or before.
>>
>> In 9.2dev, as the result of supporting cascade replication,
>> an archived WAL file is restored onto correct file name so that
>> cascading walsender can send it to another standby. This restored
>
> The documentation still says this:
>
> WAL segments that cannot be found in the archive will be sought in pg_xlog/;
> this allows use of recent un-archived segments. However, segments that are
> available from the archive will be used in preference to files in
> pg_xlog/. The system will not overwrite the existing contents of pg_xlog/
> when retrieving archived files.
>
> I gather the last sentence is now false?

Yes. Attached patch removes that sentence.

>> WAL file has neither .ready nor .done archive status file. After
>> failover, checkpoint checks the archive status file of the restored
>> WAL file to attempt to recycle it, finds that it has neither .ready
>> nor ,done, and creates .ready. Because of existence of .ready,
>> it will be archived again even though it obviously already exists in
>> the archival storage :(
>>
>> To prevent a restored WAL file from being archived again, I think
>> that .done should be created whenever WAL file is successfully
>> restored (of course this should happen only when archive_mode is
>> enabled). Thought?
>
> Your proposed fix makes sense, and I cannot think of any disadvantage.
> Concerning only doing it when archive_mode=on, would there ever be a case
> where a segment is restored under archive_mode=off, then the server restarted
> with archive_mode=on and an archival attempted on that segment?

Yes, .done file should be created even if archive mode is not enabled.

Attached patch changes the startup process so that it creates .done file
whenever WAL file is successfully restored, whether archive mode is
enabled or not. The restored WAL files will not be archived again because
of .done file.

Regards,

--
Fujii Masao

On Mon, Jul 30, 2012 at 12:01 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Jun 5, 2012 at 3:37 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> On Fri, Mar 23, 2012 at 11:03:27PM +0900, Fujii Masao wrote:
>>> (2) WAL files which were restored from the archive
>>>
>>> In 9.1 or before, the restored WAL files don't remain after failover
>>> because they are always restored onto the temporary filename
>>> "RECOVERYXLOG". So the issue which I explain from now doesn't exist
>>> in 9.1 or before.
>>>
>>> In 9.2dev, as the result of supporting cascade replication,
>>> an archived WAL file is restored onto correct file name so that
>>> cascading walsender can send it to another standby. This restored
>>
>> The documentation still says this:
>>
>> WAL segments that cannot be found in the archive will be sought in pg_xlog/;
>> this allows use of recent un-archived segments. However, segments that are
>> available from the archive will be used in preference to files in
>> pg_xlog/. The system will not overwrite the existing contents of pg_xlog/
>> when retrieving archived files.
>>
>> I gather the last sentence is now false?
>
> Yes. Attached patch removes that sentence.
>
>>> WAL file has neither .ready nor .done archive status file. After
>>> failover, checkpoint checks the archive status file of the restored
>>> WAL file to attempt to recycle it, finds that it has neither .ready
>>> nor ,done, and creates .ready. Because of existence of .ready,
>>> it will be archived again even though it obviously already exists in
>>> the archival storage :(
>>>
>>> To prevent a restored WAL file from being archived again, I think
>>> that .done should be created whenever WAL file is successfully
>>> restored (of course this should happen only when archive_mode is
>>> enabled). Thought?
>>
>> Your proposed fix makes sense, and I cannot think of any disadvantage.
>> Concerning only doing it when archive_mode=on, would there ever be a case
>> where a segment is restored under archive_mode=off, then the server restarted
>> with archive_mode=on and an archival attempted on that segment?
>
> Yes, .done file should be created even if archive mode is not enabled.
>
> Attached patch changes the startup process so that it creates .done file
> whenever WAL file is successfully restored, whether archive mode is
> enabled or not. The restored WAL files will not be archived again because
> of .done file.

This patch can be applied cleanly for HEAD, but not in REL9_2_STABLE.
So here is the patch for REL9_2_STABLE.

Regards,

--
Fujii Masao

On 29 July 2012 16:01, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> Attached patch changes the startup process so that it creates .done file
> whenever WAL file is successfully restored, whether archive mode is
> enabled or not. The restored WAL files will not be archived again because
> of .done file.

The proposed patch works, for archiving only, but I don't like the
code. It's a partial refactoring of existing code.

I prefer to go for a full re-factoring version for HEAD, and a zero
refactoring version for 9.2 since we're deep into beta.

I've committed the simplified version for 9.2, as well as adding
support for streaming which you seem to have missed out.

Will look at the refactored version tomorrow.

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

On 9 August 2012 00:08, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> Will look at the refactored version tomorrow.

Rather than implement the minimal patch onto HEAD, I've written a
refactored patch. [attached]
This fixes the bugs discussed here, though allows for multiple archive
statuses, rather than just two, as well as removing similar code.

We've talked about allowing file based replication to cascade, i.e.
having the archiver run on standby and to push files onwards from the
standby.

The way I'd like to do this is

If archiving active
* Allow archiver to run on standby
* When xlog file written on standby it sets archive_status to ".standby"
* When archiver runs it will execute archive_command when it sees a
status of ".ready" or it will execute standby_archive_command when it
sees a status of ".standby". In both cases it will set a status of
".done" when complete. It doesn't need to know whether promotion has
happened, or even when it happened, because the status of the xlog
files is clearly marked.

We can't use ".ready" to mean two things because then we'd be confused
how to handle files generated during standby but archived after
promotion has taken place. ".done" always means you can clean up the
xlog files now, in all cases.

You're welcome to complete the implementation using this patch, or
I'll do that when I get back from leave.

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

On Thu, Aug 9, 2012 at 8:08 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 29 July 2012 16:01, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
>> Attached patch changes the startup process so that it creates .done file
>> whenever WAL file is successfully restored, whether archive mode is
>> enabled or not. The restored WAL files will not be archived again because
>> of .done file.
>
> The proposed patch works, for archiving only, but I don't like the
> code. It's a partial refactoring of existing code.
>
> I prefer to go for a full re-factoring version for HEAD, and a zero
> refactoring version for 9.2 since we're deep into beta.

Sounds reasonable.

> I've committed the simplified version for 9.2, as well as adding
> support for streaming which you seem to have missed out.

If the streaming case really must be covered, the fix should be backported to
9.1 and 9.0? Because there is the same "problem" in those versions.

But I'm not sure if this fix is right thing to do. I didn't add any code to the
streaming case because the current behavior exists since 9.0 and AFAIR
we've not received the complaints about the behavior. So ISTM changing
that behavior is debatable and looks like 9.3 item rather than 9.2.

Regards,

--
Fujii Masao

On Thu, Aug 9, 2012 at 11:24 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 9 August 2012 00:08, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
>> Will look at the refactored version tomorrow.
>
>
> Rather than implement the minimal patch onto HEAD, I've written a
> refactored patch. [attached]
> This fixes the bugs discussed here, though allows for multiple archive
> statuses, rather than just two, as well as removing similar code.
>
> We've talked about allowing file based replication to cascade, i.e.
> having the archiver run on standby and to push files onwards from the
> standby.
>
> The way I'd like to do this is
>
> If archiving active
> * Allow archiver to run on standby
> * When xlog file written on standby it sets archive_status to ".standby"
> * When archiver runs it will execute archive_command when it sees a
> status of ".ready" or it will execute standby_archive_command when it
> sees a status of ".standby". In both cases it will set a status of
> ".done" when complete. It doesn't need to know whether promotion has
> happened, or even when it happened, because the status of the xlog
> files is clearly marked.
>
> We can't use ".ready" to mean two things because then we'd be confused
> how to handle files generated during standby but archived after
> promotion has taken place. ".done" always means you can clean up the
> xlog files now, in all cases.

Sounds good to me.

Regards,

--
Fujii Masao

Dear sender,
I'm away from the office till 19th of August. In the meantime, please contact my colleague Alfred Vater at alfred(dot)vater(at)aegaeon(dot)de

On 10 August 2012 18:19, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

>> I've committed the simplified version for 9.2, as well as adding
>> support for streaming which you seem to have missed out.
>
> If the streaming case really must be covered, the fix should be backported to
> 9.1 and 9.0? Because there is the same "problem" in those versions.

True

> But I'm not sure if this fix is right thing to do. I didn't add any code to the
> streaming case because the current behavior exists since 9.0 and AFAIR
> we've not received the complaints about the behavior. So ISTM changing
> that behavior is debatable and looks like 9.3 item rather than 9.2.

If its a bug with archiving, then why is it not a bug with streaming?

What if you are using archiving and streaming, what then? There isn't
any way of telling the difference between them.

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

On Sat, Aug 11, 2012 at 2:19 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Thu, Aug 9, 2012 at 8:08 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On 29 July 2012 16:01, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>
>>> Attached patch changes the startup process so that it creates .done file
>>> whenever WAL file is successfully restored, whether archive mode is
>>> enabled or not. The restored WAL files will not be archived again because
>>> of .done file.
>>
>> The proposed patch works, for archiving only, but I don't like the
>> code. It's a partial refactoring of existing code.
>>
>> I prefer to go for a full re-factoring version for HEAD, and a zero
>> refactoring version for 9.2 since we're deep into beta.

Isn't it time to push the full re-factoring version to HEAD? If there is no
such version yet, what about pushing the zero refactoring version for now?

Regards,

--
Fujii Masao

On 2 October 2012 19:06, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Sat, Aug 11, 2012 at 2:19 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Thu, Aug 9, 2012 at 8:08 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> On 29 July 2012 16:01, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>
>>>> Attached patch changes the startup process so that it creates .done file
>>>> whenever WAL file is successfully restored, whether archive mode is
>>>> enabled or not. The restored WAL files will not be archived again because
>>>> of .done file.
>>>
>>> The proposed patch works, for archiving only, but I don't like the
>>> code. It's a partial refactoring of existing code.
>>>
>>> I prefer to go for a full re-factoring version for HEAD, and a zero
>>> refactoring version for 9.2 since we're deep into beta.
>
> Isn't it time to push the full re-factoring version to HEAD? If there is no
> such version yet, what about pushing the zero refactoring version for now?

If you send a rebased patch, I'll review, but its not high on my radar
right now unless you can explain why it should be higher.

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

On Wed, Oct 3, 2012 at 3:11 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 2 October 2012 19:06, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Sat, Aug 11, 2012 at 2:19 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Thu, Aug 9, 2012 at 8:08 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>>> On 29 July 2012 16:01, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>>
>>>>> Attached patch changes the startup process so that it creates .done file
>>>>> whenever WAL file is successfully restored, whether archive mode is
>>>>> enabled or not. The restored WAL files will not be archived again because
>>>>> of .done file.
>>>>
>>>> The proposed patch works, for archiving only, but I don't like the
>>>> code. It's a partial refactoring of existing code.
>>>>
>>>> I prefer to go for a full re-factoring version for HEAD, and a zero
>>>> refactoring version for 9.2 since we're deep into beta.
>>
>> Isn't it time to push the full re-factoring version to HEAD? If there is no
>> such version yet, what about pushing the zero refactoring version for now?
>
> If you send a rebased patch, I'll review,

Okay. Will do. The patch needs to be revised to correspond with the recent
split of xlog.c.

> but its not high on my radar
> right now unless you can explain why it should be higher.

It may not be high, but I'm just worried that we are likely to forget to
apply that change into HEAD if we postpone it furthermore.

Regards,

--
Fujii Masao

On 02.10.2012 21:20, Fujii Masao wrote:
> On Wed, Oct 3, 2012 at 3:11 AM, Simon Riggs<simon(at)2ndquadrant(dot)com> wrote:
>> but its not high on my radar
>> right now unless you can explain why it should be higher.
>
> It may not be high, but I'm just worried that we are likely to forget to
> apply that change into HEAD if we postpone it furthermore.

Ping? I haven't been paying much attention to this, but please commit
the 9.2 fix to HEAD. This just caused a small merge conflict when I
tried to backport (or rather, forward-port) a patch. We do more changes
to HEAD later.

- Heikki

On Sun, Dec 30, 2012 at 6:07 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> On 02.10.2012 21:20, Fujii Masao wrote:
>>
>> On Wed, Oct 3, 2012 at 3:11 AM, Simon Riggs<simon(at)2ndquadrant(dot)com> wrote:
>>>
>>> but its not high on my radar
>>>
>>> right now unless you can explain why it should be higher.
>>
>>
>> It may not be high, but I'm just worried that we are likely to forget to
>> apply that change into HEAD if we postpone it furthermore.
>
>
> Ping? I haven't been paying much attention to this, but please commit the
> 9.2 fix to HEAD. This just caused a small merge conflict when I tried to
> backport (or rather, forward-port) a patch. We do more changes to HEAD
> later.

Sorry for the late. I attached the patch for HEAD. Since I've not understood
completely the recent change related to handling of the timeline yet, the patch
might treat with the timeline wrongly. I'm not sure if this patch has something
to do with the recent change, though. I appreciate it if you review carefully
the patch.

Regards,

--
Fujii Masao