Horton

Thread

Commits

  1. Ensure cleanup of orphan archive status files

  1. Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-09-28T03:28:27Z

    Hi all,

While reviewing the archiving code, I have bumped into the fact that
XLogArchiveCleanup() thinks that it is safe to do only a plain unlink()
for .ready and .done files when removing a past segment.  I don't think
that it is a smart move, as on a subsequent crash we may still see
those, but the related segment would have gone away.  This is not really
a problem for .done files, but it could confuse the archiver to see some
.ready files about things that have already gone away.

Attached is a patch.  Thoughts?
--
Michael

  2. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Andres Freund <andres@anarazel.de> — 2018-09-28T03:40:26Z

    Hi,

On 2018-09-28 12:28:27 +0900, Michael Paquier wrote:
> While reviewing the archiving code, I have bumped into the fact that
> XLogArchiveCleanup() thinks that it is safe to do only a plain unlink()
> for .ready and .done files when removing a past segment.  I don't think
> that it is a smart move, as on a subsequent crash we may still see
> those, but the related segment would have gone away.  This is not really
> a problem for .done files, but it could confuse the archiver to see some
> .ready files about things that have already gone away.

Isn't that window fundamentally there anyway?

- Andres

  3. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-09-28T05:23:31Z

    On Thu, Sep 27, 2018 at 08:40:26PM -0700, Andres Freund wrote:
> On 2018-09-28 12:28:27 +0900, Michael Paquier wrote:
>> While reviewing the archiving code, I have bumped into the fact that
>> XLogArchiveCleanup() thinks that it is safe to do only a plain unlink()
>> for .ready and .done files when removing a past segment.  I don't think
>> that it is a smart move, as on a subsequent crash we may still see
>> those, but the related segment would have gone away.  This is not really
>> a problem for .done files, but it could confuse the archiver to see some
>> .ready files about things that have already gone away.
> 
> Isn't that window fundamentally there anyway?

Sure.  However the point I would like to make is that if we have the
possibility to reduce this window, then we should.
--
Michael

  4. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Andres Freund <andres@anarazel.de> — 2018-09-28T05:24:47Z

    
On September 27, 2018 10:23:31 PM PDT, Michael Paquier <michael@paquier.xyz> wrote:
>On Thu, Sep 27, 2018 at 08:40:26PM -0700, Andres Freund wrote:
>> On 2018-09-28 12:28:27 +0900, Michael Paquier wrote:
>>> While reviewing the archiving code, I have bumped into the fact that
>>> XLogArchiveCleanup() thinks that it is safe to do only a plain
>unlink()
>>> for .ready and .done files when removing a past segment.  I don't
>think
>>> that it is a smart move, as on a subsequent crash we may still see
>>> those, but the related segment would have gone away.  This is not
>really
>>> a problem for .done files, but it could confuse the archiver to see
>some
>>> .ready files about things that have already gone away.
>> 
>> Isn't that window fundamentally there anyway?
>
>Sure.  However the point I would like to make is that if we have the
>possibility to reduce this window, then we should.

It's not free though. I don't think this is as clear cut as you make it sound.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  5. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Stephen Frost <sfrost@snowman.net> — 2018-09-28T18:36:19Z

    Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> While reviewing the archiving code, I have bumped into the fact that
> XLogArchiveCleanup() thinks that it is safe to do only a plain unlink()
> for .ready and .done files when removing a past segment.  I don't think
> that it is a smart move, as on a subsequent crash we may still see
> those, but the related segment would have gone away.  This is not really
> a problem for .done files, but it could confuse the archiver to see some
> .ready files about things that have already gone away.

Is there an issue with making the archiver able to understand that
situation instead of being confused by it..?  Seems like that'd probably
be a good thing to do regardless of this, but that would then remove the
need for this kind of change..

Thanks!

Stephen

  6. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-09-28T23:11:43Z

    On Fri, Sep 28, 2018 at 02:36:19PM -0400, Stephen Frost wrote:
> Is there an issue with making the archiver able to understand that
> situation instead of being confused by it..?  Seems like that'd probably
> be a good thing to do regardless of this, but that would then remove the
> need for this kind of change..

I thought about that a bit, and there is as well a lot which can be done
within the archive_command itself regarding that, so I am not sure that
there is the argument to make pgarch.c more complicated than it should.
Now it is true that for most users having a .ready file but no segment
would most likely lead in a failure.  I suspect that a large user base
is still just using plain cp in archive_command, which would cause the
archiver to be stuck.  So we could actually just tweak pgarch_readyXlog
to check if the segment fetched actually exists (see bottom of the
so-said function).  If it doesn't, then the archiver removes the .ready
file and retries fetching a new segment.
--
Michael

  7. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Stephen Frost <sfrost@snowman.net> — 2018-09-28T23:16:25Z

    Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Fri, Sep 28, 2018 at 02:36:19PM -0400, Stephen Frost wrote:
> > Is there an issue with making the archiver able to understand that
> > situation instead of being confused by it..?  Seems like that'd probably
> > be a good thing to do regardless of this, but that would then remove the
> > need for this kind of change..
> 
> I thought about that a bit, and there is as well a lot which can be done
> within the archive_command itself regarding that, so I am not sure that
> there is the argument to make pgarch.c more complicated than it should.
> Now it is true that for most users having a .ready file but no segment
> would most likely lead in a failure.  I suspect that a large user base
> is still just using plain cp in archive_command, which would cause the
> archiver to be stuck.  So we could actually just tweak pgarch_readyXlog
> to check if the segment fetched actually exists (see bottom of the
> so-said function).  If it doesn't, then the archiver removes the .ready
> file and retries fetching a new segment.

Yes, checking if the WAL file exists before calling archive_command on
it is what I was thinking we'd do here, and if it doesn't, then just
remove the .ready file.

An alternative would be to go through the .ready files on crash-recovery
and remove any .ready files that don't have corresponding WAL files, or
if we felt that it was necessary, we could do that on every restart but
do we really think we'd need to do that..?

Thanks!

Stephen

  8. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-09-29T07:58:57Z

    On Fri, Sep 28, 2018 at 07:16:25PM -0400, Stephen Frost wrote:
> An alternative would be to go through the .ready files on crash-recovery
> and remove any .ready files that don't have corresponding WAL files, or
> if we felt that it was necessary, we could do that on every restart but
> do we really think we'd need to do that..?

Actually, what you are proposing here sounds much better to me.  That's
in the area of what has been done recently with RemoveTempXlogFiles() in
5fc1008e.  Any objections to doing something like that? 
--
Michael

  9. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-09-30T12:40:31Z

    On Sat, Sep 29, 2018 at 04:58:57PM +0900, Michael Paquier wrote:
> Actually, what you are proposing here sounds much better to me.  That's
> in the area of what has been done recently with RemoveTempXlogFiles() in
> 5fc1008e.  Any objections to doing something like that? 

Okay.  I have hacked a patch based on Stephen's idea as attached.  Any
opinions?
--
Michael

  10. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Nathan Bossart <bossartn@amazon.com> — 2018-11-02T14:47:08Z

    One argument for instead checking WAL file existence before calling
archive_command might be to avoid the increased startup time.
Granted, any added delay from this patch is unlikely to be noticeable
unless your archiver is way behind and archive_status has a huge
number of files.  However, I have seen cases where startup is stuck on
other tasks like SyncDataDirectory() and RemovePgTempFiles() for a
very long time, so perhaps it is worth considering.

Nathan

  11. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-11-15T10:39:27Z

    At Fri, 02 Nov 2018 14:47:08 +0000, Nathan Bossart <bossartn@amazon.com> wrote in <154117002849.5569.14588306221618961668.pgcf@coridan.postgresql.org>
> One argument for instead checking WAL file existence before calling
> archive_command might be to avoid the increased startup time.
> Granted, any added delay from this patch is unlikely to be noticeable
> unless your archiver is way behind and archive_status has a huge
> number of files.  However, I have seen cases where startup is stuck on
> other tasks like SyncDataDirectory() and RemovePgTempFiles() for a
> very long time, so perhaps it is worth considering.

While archive_mode is tuned on, .ready files are created for all
exising wal files if not exists. Thus archiver may wait for the
ealiest segment to have .ready file. As the result
pgarch_readyXLog can be modified to loops over wal files, not
status files.  This prevents the confusion comes from .ready
files for non-existent segment files.

RemoveXlogFile as is doesn't get confused by .done files for
nonexistent segments.

We may leave useless .done/.ready files. We no longer scan over
the files so no matter how many files are there in the directory.

The remaining issue is removal of the files. Even if we blew away
the directory altogether, status files would be cleanly recreated
having already-archived wal segments are archived again. However,
redundant copy won't happen with our recommending configuration:p

# Yeah, I see almost all sites uses simple 'cp' or 'scp' for that..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  12. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-11-22T04:16:09Z

    On Thu, Nov 15, 2018 at 07:39:27PM +0900, Kyotaro HORIGUCHI wrote:
> At Fri, 02 Nov 2018 14:47:08 +0000, Nathan Bossart
> <bossartn@amazon.com> wrote in
> <154117002849.5569.14588306221618961668.pgcf@coridan.postgresql.org>:
>> One argument for instead checking WAL file existence before calling
>> archive_command might be to avoid the increased startup time.

I guess that you mean the startup of the archive command itself here.
Yes that can be an issue with a high WAL output depending on the
interpreter of the archive command :(

>> Granted, any added delay from this patch is unlikely to be noticeable
>> unless your archiver is way behind and archive_status has a huge
>> number of files.  However, I have seen cases where startup is stuck on
>> other tasks like SyncDataDirectory() and RemovePgTempFiles() for a
>> very long time, so perhaps it is worth considering.

What's the scale of the pg_wal partition and the amount of time things
were stuck?  I would imagine that the sync phase hurts the most, and a
fast startup time for crash recovery is always important.

> While archive_mode is tuned on, .ready files are created for all
> existing wal files if not exists. Thus archiver may wait for the
> earliest segment to have .ready file.

Yes, RemoveOldXlogFiles() does that via XLogArchiveCheckDone().

> As the result
> pgarch_readyXLog can be modified to loops over WAL files, not
> status files.  This prevents the confusion comes from .ready
> files for non-existent segment files.

No, pgarch_readyXLog() should still look after .ready files as those are
here for this purpose, but we could have an additional check to see if
the segment linked with it actually exists and can be archived.  This
check could happen in pgarch.c code before calling the archive command
gets called (just before pgarch_ArchiverCopyLoop and after
XLogArchiveCommandSet feels rather right, and that it should be cheap
enough to call stat()).
--
Michael

  13. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-11-27T12:34:40Z

    On Thu, Nov 22, 2018 at 01:16:09PM +0900, Michael Paquier wrote:
> No, pgarch_readyXLog() should still look after .ready files as those are
> here for this purpose, but we could have an additional check to see if
> the segment linked with it actually exists and can be archived.  This
> check could happen in pgarch.c code before calling the archive command
> gets called (just before pgarch_ArchiverCopyLoop and after
> XLogArchiveCommandSet feels rather right, and that it should be cheap
> enough to call stat()).

s/pgarch_ArchiverCopyLoop/pgarch_archiveXlog/.

Attached is a patch showing shaped based on the idea of upthread.
Thoughts?
--
Michael

  14. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Nathan Bossart <bossartn@amazon.com> — 2018-11-27T20:43:06Z

    On 11/21/18, 10:16 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
>> At Fri, 02 Nov 2018 14:47:08 +0000, Nathan Bossart
>> <bossartn@amazon.com> wrote in
>> <154117002849.5569.14588306221618961668.pgcf@coridan.postgresql.org>:
>>> Granted, any added delay from this patch is unlikely to be noticeable
>>> unless your archiver is way behind and archive_status has a huge
>>> number of files.  However, I have seen cases where startup is stuck on
>>> other tasks like SyncDataDirectory() and RemovePgTempFiles() for a
>>> very long time, so perhaps it is worth considering.
>
> What's the scale of the pg_wal partition and the amount of time things
> were stuck?  I would imagine that the sync phase hurts the most, and a
> fast startup time for crash recovery is always important.

I don't have exact figures to share, but yes, a huge number of calls
to sync_file_range() and fsync() can use up a lot of time.  Presumably
Postgres processes files individually instead of using sync() because
sync() may return before writing is done.  Also, sync() would affect
non-Postgres files.  However, it looks like Linux actually does wait
for writing to complete before returning from sync() [0].

For RemovePgTempFiles(), the documentation above the function
indicates that skipping temp file cleanup during startup would
actually be okay because collisions with existing temp file names
should be handled by OpenTemporaryFile().  I assume this cleanup is
done during startup because there isn't a great alternative besides
offloading the work to a new background worker or something.

On 11/27/18, 6:35 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Attached is a patch showing shaped based on the idea of upthread.
> Thoughts?

I took a look at this patch.

+                       /*
+                        * In the event of a system crash, archive status files may be
+                        * left behind as their removals are not durable, cleaning up
+                        * orphan entries here is the cheapest method.  So check that
+                        * the segment trying to be archived still exists.
+                        */
+                       snprintf(pathname, MAXPGPATH, XLOGDIR "/%s", xlog);
+                       if (stat(pathname, &stat_buf) != 0)
+                       {

Don't we also need to check that errno is ENOENT here?

+                               StatusFilePath(xlogready, xlog, ".ready");
+                               if (durable_unlink(xlogready, WARNING) == 0)
+                                       ereport(WARNING,
+                                                       (errmsg("removed orphan archive status file %s",
+                                                                       xlogready)));
+                               return;

IIUC any time that the file does not exist, we will attempt to unlink
it.  Regardless of whether unlinking fails or succeeds, we then
proceed to give up archiving for now, but it's not clear why.  Perhaps
we should retry unlinking a number of times (like we do for
pgarch_archiveXlog()) when durable_unlink() fails and simply "break"
to move on to the next .ready file if durable_unlink() succeeds.

Nathan

[0] http://man7.org/linux/man-pages/man2/sync.2.html

  15. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Andres Freund <andres@anarazel.de> — 2018-11-27T20:45:57Z

    Hi,

On 2018-11-27 20:43:06 +0000, Bossart, Nathan wrote:
> I don't have exact figures to share, but yes, a huge number of calls
> to sync_file_range() and fsync() can use up a lot of time.  Presumably
> Postgres processes files individually instead of using sync() because
> sync() may return before writing is done.  Also, sync() would affect
> non-Postgres files.  However, it looks like Linux actually does wait
> for writing to complete before returning from sync() [0].

sync() has absolutely no way to report errors. So, we're never going to
be able to use it.  Besides, even postgres' temp files would be a good
reason to not use it.

Greetings,

Andres Freund

  16. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Nathan Bossart <bossartn@amazon.com> — 2018-11-27T20:50:05Z

    On 11/27/18, 2:46 PM, "Andres Freund" <andres@anarazel.de> wrote:
> On 2018-11-27 20:43:06 +0000, Bossart, Nathan wrote:
>> I don't have exact figures to share, but yes, a huge number of calls
>> to sync_file_range() and fsync() can use up a lot of time.  Presumably
>> Postgres processes files individually instead of using sync() because
>> sync() may return before writing is done.  Also, sync() would affect
>> non-Postgres files.  However, it looks like Linux actually does wait
>> for writing to complete before returning from sync() [0].
>
> sync() has absolutely no way to report errors. So, we're never going to
> be able to use it.  Besides, even postgres' temp files would be a good
> reason to not use it.

Ah, I see.  Thanks for clarifying.

Nathan

  17. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-11-27T21:19:05Z

    On Tue, Nov 27, 2018 at 08:43:06PM +0000, Bossart, Nathan wrote:
> Don't we also need to check that errno is ENOENT here?

Yep.

> IIUC any time that the file does not exist, we will attempt to unlink
> it.  Regardless of whether unlinking fails or succeeds, we then
> proceed to give up archiving for now, but it's not clear why.  Perhaps
> we should retry unlinking a number of times (like we do for
> pgarch_archiveXlog()) when durable_unlink() fails and simply "break"
> to move on to the next .ready file if durable_unlink() succeeds.

Both suggestions sound reasonable to me.  (durable_unlink is not called
on HEAD in pgarch_archiveXlog).  How about 3 retries with a in-between
wait of 1s?  That's consistent with what pgarch_ArchiverCopyLoop does,
still I am not completely sure if we actually want to be consistent for
the purpose of removing orphaned ready files.
--
Michael

  18. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Nathan Bossart <bossartn@amazon.com> — 2018-11-27T21:49:29Z

    On 11/27/18, 3:20 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Tue, Nov 27, 2018 at 08:43:06PM +0000, Bossart, Nathan wrote:
>> IIUC any time that the file does not exist, we will attempt to unlink
>> it.  Regardless of whether unlinking fails or succeeds, we then
>> proceed to give up archiving for now, but it's not clear why.  Perhaps
>> we should retry unlinking a number of times (like we do for
>> pgarch_archiveXlog()) when durable_unlink() fails and simply "break"
>> to move on to the next .ready file if durable_unlink() succeeds.
>
> Both suggestions sound reasonable to me.  (durable_unlink is not called
> on HEAD in pgarch_archiveXlog).  How about 3 retries with a in-between
> wait of 1s?  That's consistent with what pgarch_ArchiverCopyLoop does,
> still I am not completely sure if we actually want to be consistent for
> the purpose of removing orphaned ready files.

That sounds good to me.  I was actually thinking of using the same
retry counter that we use for pgarch_archiveXlog(), but on second
thought, it is probably better to have two independent retry counters
for these two unrelated operations.

Nathan

  19. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-11-27T21:52:07Z

    On Tue, Nov 27, 2018 at 09:49:29PM +0000, Bossart, Nathan wrote:
> That sounds good to me.  I was actually thinking of using the same
> retry counter that we use for pgarch_archiveXlog(), but on second
> thought, it is probably better to have two independent retry counters
> for these two unrelated operations.

What I had in mind was two different variables if what I wrote was
unclear, possibly with the same value, as archiving failure and failure
with orphan file removals are two different concepts.
--
Michael

  20. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Nathan Bossart <bossartn@amazon.com> — 2018-11-29T15:00:42Z

    On 11/27/18, 3:53 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Tue, Nov 27, 2018 at 09:49:29PM +0000, Bossart, Nathan wrote:
>> That sounds good to me.  I was actually thinking of using the same
>> retry counter that we use for pgarch_archiveXlog(), but on second
>> thought, it is probably better to have two independent retry counters
>> for these two unrelated operations.
>
> What I had in mind was two different variables if what I wrote was
> unclear, possibly with the same value, as archiving failure and failure
> with orphan file removals are two different concepts.

+1

Nathan

  21. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-12-04T07:36:04Z

    On Thu, Nov 29, 2018 at 03:00:42PM +0000, Bossart, Nathan wrote:
> +1

Okay, here is an updated patch for this stuff, which does the following:
- Check for a WAL segment if it has a ".ready" status file, an orphaned
status file is removed only on ENOENT.
- If durable_unlink fails, retry 3 times.  If there are too many
failures, the archiver gives up on the orphan status file removal.  If
the removal works correctly, the archiver moves on to the next file.

(The variable names could be better.)
--
Michael

  22. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Nathan Bossart <bossartn@amazon.com> — 2018-12-04T20:40:40Z

    On 12/4/18, 1:36 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Okay, here is an updated patch for this stuff, which does the following:
> - Check for a WAL segment if it has a ".ready" status file, an orphaned
> status file is removed only on ENOENT.
> - If durable_unlink fails, retry 3 times.  If there are too many
> failures, the archiver gives up on the orphan status file removal.  If
> the removal works correctly, the archiver moves on to the next file.

Thanks for the updated patch!  The code looks good to me, the patch
applies cleanly and builds without warnings, and it seems to work well
in my manual tests.  I just have a few wording suggestions.

+			/*
+			 * In the event of a system crash, archive status files may be
+			 * left behind as their removals are not durable, cleaning up
+			 * orphan entries here is the cheapest method.  So check that
+			 * the segment trying to be archived still exists.  If it does
+			 * not, its orphan status file is removed.
+			 */

I would phrase this comment this way:

        Since archive_status files are not durably removed, a system
        crash could leave behind .ready files for WAL segments that
        have already been recycled or removed.  In this case, simply
        remove the orphan status file and move on.

+					ereport(WARNING,
+							(errmsg("removed orphan archive status file %s",
+									xlogready)));

I think we should put quotes around the file name like we do elsewhere
in pgarch_ArchiverCopyLoop().

+					ereport(WARNING,
+							(errmsg("failed removal of \"%s\" too many times, will try again later",
+									xlogready)));

I'd suggest mirroring the log statement for failed archiving commands
and saying something like, "removing orphan archive status file \"%s\"
failed too many times, will try again later."  IMO that makes it
clearer what is failing and why we are removing it in the first place.

Nathan

  23. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-12-05T01:34:43Z

    On Tue, Dec 04, 2018 at 08:40:40PM +0000, Bossart, Nathan wrote:
> Thanks for the updated patch!  The code looks good to me, the patch
> applies cleanly and builds without warnings, and it seems to work well
> in my manual tests.  I just have a few wording suggestions.

How are you testing this?  I just stop the server and manually touch
some fake status files in archive_status :)

> I would phrase this comment this way:
> 
>         Since archive_status files are not durably removed, a system
>         crash could leave behind .ready files for WAL segments that
>         have already been recycled or removed.  In this case, simply
>         remove the orphan status file and move on.

Fine for me.  Thanks!

> +     ereport(WARNING,
> +             (errmsg("removed orphan archive status file %s",
> +                     xlogready)));
> 
> I think we should put quotes around the file name like we do elsewhere
> in pgarch_ArchiverCopyLoop().

Done.

> +					ereport(WARNING,
> +							(errmsg("failed removal of \"%s\" too many times, will try again later",
> +									xlogready)));
> 
> I'd suggest mirroring the log statement for failed archiving commands
> and saying something like, "removing orphan archive status file \"%s\"
> failed too many times, will try again later."  IMO that makes it
> clearer what is failing and why we are removing it in the first place.

"removal of" is more consistent here I think, so changed this way with
your wording merged.
--
Michael

  24. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Nathan Bossart <bossartn@amazon.com> — 2018-12-05T16:11:23Z

    On 12/4/18, 7:35 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Tue, Dec 04, 2018 at 08:40:40PM +0000, Bossart, Nathan wrote:
>> Thanks for the updated patch!  The code looks good to me, the patch
>> applies cleanly and builds without warnings, and it seems to work well
>> in my manual tests.  I just have a few wording suggestions.
>
> How are you testing this?  I just stop the server and manually touch
> some fake status files in archive_status :)

That's almost exactly what I was doing, too.

>> I would phrase this comment this way:
>> 
>>         Since archive_status files are not durably removed, a system
>>         crash could leave behind .ready files for WAL segments that
>>         have already been recycled or removed.  In this case, simply
>>         remove the orphan status file and move on.
>
> Fine for me.  Thanks!
>
>> +     ereport(WARNING,
>> +             (errmsg("removed orphan archive status file %s",
>> +                     xlogready)));
>> 
>> I think we should put quotes around the file name like we do elsewhere
>> in pgarch_ArchiverCopyLoop().
>
> Done.
>
>> +					ereport(WARNING,
>> +							(errmsg("failed removal of \"%s\" too many times, will try again later",
>> +									xlogready)));
>> 
>> I'd suggest mirroring the log statement for failed archiving commands
>> and saying something like, "removing orphan archive status file \"%s\"
>> failed too many times, will try again later."  IMO that makes it
>> clearer what is failing and why we are removing it in the first place.
>
> "removal of" is more consistent here I think, so changed this way with
> your wording merged.

The v4 patch looks good to me!

Nathan

  25. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-12-06T04:55:46Z

    Hello.

At Wed, 5 Dec 2018 16:11:23 +0000, "Bossart, Nathan" <bossartn@amazon.com> wrote in <DB47EBD7-7291-4C39-9F8F-BE42E5193BD5@amazon.com>
> The v4 patch looks good to me!

durable_unlnk has two modes of faiure. Failure to unlink and
fsync. If once it fails at the fsync stage, subsequent
durable_unlink calls for the same file always fail to unlink with
ENOENT.  If durable_unlink is intended to be called repeatedly on
falure, perhaps it should return a different code for each
failure so that the caller can indentify what to do next.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  26. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-12-06T05:43:35Z

    On Thu, Dec 06, 2018 at 01:55:46PM +0900, Kyotaro HORIGUCHI wrote:
> durable_unlink has two modes of faiure. Failure to unlink and
> fsync. If once it fails at the fsync stage, subsequent
> durable_unlink calls for the same file always fail to unlink with
> ENOENT.  If durable_unlink is intended to be called repeatedly on
> falure, perhaps it should return a different code for each
> failure so that the caller can indentify what to do next.

Why?  A WARNING would be logged if the first unlink() fails, and
another, different WARNING would be logged if the subsequent fsync
fails.  It looks enough to me to make a distinction between both.  Now,
you may have a point in the fact that we could also live with only using
unlink() for this code path, as even on repetitive crashes this would
take care of removing orphan archive status files consistently.
--
Michael

  27. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-12-06T22:53:20Z

    On Thu, Dec 06, 2018 at 02:43:35PM +0900, Michael Paquier wrote:
> Why?  A WARNING would be logged if the first unlink() fails, and
> another, different WARNING would be logged if the subsequent fsync
> fails.  It looks enough to me to make a distinction between both.  Now,
> you may have a point in the fact that we could also live with only using
> unlink() for this code path, as even on repetitive crashes this would
> take care of removing orphan archive status files consistently.

After sleeping on that, using plain unlink() makes indeed the most
sense.  Any objections if I move on with that, adding a proper comment
explaining the choice?  I don't plan to finish wrapping this patch today
but Monday my time anyway.
--
Michael

  28. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Nathan Bossart <bossartn@amazon.com> — 2018-12-06T23:18:12Z

    On 12/6/18, 4:54 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Thu, Dec 06, 2018 at 02:43:35PM +0900, Michael Paquier wrote:
>> Why?  A WARNING would be logged if the first unlink() fails, and
>> another, different WARNING would be logged if the subsequent fsync
>> fails.  It looks enough to me to make a distinction between both.  Now,
>> you may have a point in the fact that we could also live with only using
>> unlink() for this code path, as even on repetitive crashes this would
>> take care of removing orphan archive status files consistently.
>
> After sleeping on that, using plain unlink() makes indeed the most
> sense.  Any objections if I move on with that, adding a proper comment
> explaining the choice?  I don't plan to finish wrapping this patch today
> but Monday my time anyway.

That seems reasonable to me.

Nathan

  29. Re: Use durable_unlink for .ready and .done files for WAL segment removal

    Michael Paquier <michael@paquier.xyz> — 2018-12-10T06:10:36Z

    On Thu, Dec 06, 2018 at 11:18:12PM +0000, Bossart, Nathan wrote:
> That seems reasonable to me.

Thanks, committed after renaming a couple of variables, after adding a
comment about the use of unlink() and also adding a comment explaining
what the different retry counters are here for.
--
Michael