From: Bossart, Nathan <bossartn@amazon.com>
To: Andres Freund <andres@anarazel.de>
Cc: alvherre@alvh.no-ip.org <alvherre@alvh.no-ip.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: x4mmm@yandex-team.ru <x4mmm@yandex-team.ru>
Cc: a.lubennikova@postgrespro.ru <a.lubennikova@postgrespro.ru>
Cc: hlinnaka@iki.fi <hlinnaka@iki.fi>
Cc: matsumura.ryo@fujitsu.com <matsumura.ryo@fujitsu.com>
Cc: masao.fujii@gmail.com <masao.fujii@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: archive status ".ready" files may be created too early
Date: Tue, 31 Aug 2021 06:45:06 +0000
Message-ID: <282F96BA-948C-41DD-8A68-3E5ACE4D92C5@amazon.com> (raw)
In-Reply-To: <20210831023839.7brycllwyyapbrfv@alap3.anarazel.de>
References: <20210829023722.cz6nbehyupp3hemk@alap3.anarazel.de>
<202108301951.x66z53s4lq6p@alvherre.pgsql>
<20210830210529.bd7wmqodt747xa2y@alap3.anarazel.de>
<C6234E05-6391-485F-92C3-10F7EE4D9612@amazon.com>
<20210831023839.7brycllwyyapbrfv@alap3.anarazel.de>
On 8/30/21, 7:39 PM, "Andres Freund" <andres@anarazel.de> wrote:
> On 2021-08-30 22:39:04 +0000, Bossart, Nathan wrote:
>> If we called NotifySegmentsReadyForArchive() before we updated the
>> flush location in shared memory, we might skip nudging the WAL writer
>> even though we should.
>
> That's trivial to address - just have a local variable saying whether we need
> to call NotifySegmentsReadyForArchive().
I think we can remove the race condition entirely by moving boundary
registration to before WALInsertLockRelease(). I attached a patch for
discussion.
>> > When s1 is fully written out, NotifySegmentsReadyForArchive() will set
>> > earliestSegBoundary = latestSegBoundary = s4 and create .reaady for .s1 -
>> > ok. But when when s2 is flushed, we'll afaict happily create .ready files
>> > for s2, s3 despite s4 not yet being written, because earliestSegBoundary is
>> > now s4.
>>
>> In this case, the .ready files for s2 and s3 wouldn't be created until
>> s4 is flushed to disk.
>
> I don't think that's true as the code stands today? The
> NotifySegmentsReadyForArchive() for s2 will update earliestSegBoundary to s4,
> because latestSegBoundary = 4 and earliestSegBoundary = 1, triggering the
> keep_latest branch. Any subsequent NotifySegmentsReadyForArchive() with a
> segment < 4 will then be able to flush s2 and s3?
When flushRecPtr is less than both of the segment boundaries,
NotifySegmentsReadyForArchive() will return without doing anything.
At least, that was the intent. If there is some reason it's not
actually working that way, I can work on fixing it.
> I don't think it's sensible to fix these separately. It'd be one thing to do
> that for HEAD, but on the back branches? And that this patch hasn't gotten any
> performance testing is scary.
Are there any specific performance tests you'd like to see? I don't
mind running a couple.
Nathan
Attachments:avoid_boundary_race.patch (application/octet-stream, 5.4K) downloaddiff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.cindex 24165ab03e..1e5b49595b 100644--- a/src/backend/access/transam/xlog.c+++ b/src/backend/access/transam/xlog.c@@ -933,6 +933,7 @@ static void RemoveXlogFile(const char *segname, XLogSegNo recycleSegNo,
static void UpdateLastRemovedPtr(char *filename);
static void ValidateXLOGDirectoryStructure(void);
static void RegisterSegmentBoundary(XLogSegNo seg, XLogRecPtr pos);
+static void NotifySegmentsReadyForArchive(XLogRecPtr flushRecPtr);
static void CleanupBackupHistory(void);
static void UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force);
static XLogRecord *ReadRecord(XLogReaderState *xlogreader,
@@ -1157,6 +1158,29 @@ XLogInsertRecord(XLogRecData *rdata,
*/
}
+ /*+ * Register our crossing the segment boundary if that occurred. We+ * do this before WALInsertLockRelease() so that the record cannot be+ * flushed to disk prior to registering the boundary (which could cause+ * us to miss marking segments as ready for archive).+ *+ * Note that we do not use XLByteToPrevSeg() for determining the ending+ * segment. This is so that a record that fits perfectly into the end+ * of the segment causes the segment to get marked ready for archival+ * immediately.+ */+ if (XLogArchivingActive())+ {+ XLogSegNo StartSeg;+ XLogSegNo EndSeg;++ XLByteToSeg(StartPos, StartSeg, wal_segment_size);+ XLByteToSeg(EndPos, EndSeg, wal_segment_size);++ if (StartSeg != EndSeg)+ RegisterSegmentBoundary(EndSeg, EndPos);+ }+
/*
* Done! Let others know that we're finished.
*/
@@ -1167,51 +1191,16 @@ XLogInsertRecord(XLogRecData *rdata,
END_CRIT_SECTION();
/*
- * If we crossed page boundary, update LogwrtRqst.Write; if we crossed- * segment boundary, register that and wake up walwriter.+ * If we crossed page boundary, update LogwrtRqst.Write.
*/
if (StartPos / XLOG_BLCKSZ != EndPos / XLOG_BLCKSZ)
{
- XLogSegNo StartSeg;- XLogSegNo EndSeg;-- XLByteToSeg(StartPos, StartSeg, wal_segment_size);- XLByteToSeg(EndPos, EndSeg, wal_segment_size);-- /*- * Register our crossing the segment boundary if that occurred.- *- * Note that we did not use XLByteToPrevSeg() for determining the- * ending segment. This is so that a record that fits perfectly into- * the end of the segment causes the latter to get marked ready for- * archival immediately.- */- if (StartSeg != EndSeg && XLogArchivingActive())- RegisterSegmentBoundary(EndSeg, EndPos);-- /*- * Advance LogwrtRqst.Write so that it includes new block(s).- *- * We do this after registering the segment boundary so that the- * comparison with the flushed pointer below can use the latest value- * known globally.- */
SpinLockAcquire(&XLogCtl->info_lck);
if (XLogCtl->LogwrtRqst.Write < EndPos)
XLogCtl->LogwrtRqst.Write = EndPos;
/* update local result copy while I have the chance */
LogwrtResult = XLogCtl->LogwrtResult;
SpinLockRelease(&XLogCtl->info_lck);
-- /*- * There's a chance that the record was already flushed to disk and we- * missed marking segments as ready for archive. If this happens, we- * nudge the WALWriter, which will take care of notifying segments as- * needed.- */- if (StartSeg != EndSeg && XLogArchivingActive() &&- LogwrtResult.Flush >= EndPos && ProcGlobal->walwriterLatch)- SetLatch(ProcGlobal->walwriterLatch);
}
/*
@@ -2720,6 +2709,9 @@ XLogWrite(XLogwrtRqst WriteRqst, bool flexible)
WalSndWakeupRequest();
LogwrtResult.Flush = LogwrtResult.Write;
++ if (XLogArchivingActive())+ NotifySegmentsReadyForArchive(LogwrtResult.Flush);
}
/*
@@ -2738,9 +2730,6 @@ XLogWrite(XLogwrtRqst WriteRqst, bool flexible)
XLogCtl->LogwrtRqst.Flush = LogwrtResult.Flush;
SpinLockRelease(&XLogCtl->info_lck);
}
-- if (XLogArchivingActive())- NotifySegmentsReadyForArchive(LogwrtResult.Flush);
}
/*
@@ -4434,7 +4423,7 @@ RegisterSegmentBoundary(XLogSegNo seg, XLogRecPtr endpos)
* Mark segments as ready for archival, given that it is safe to do so.
* This function is idempotent.
*/
-void+static void
NotifySegmentsReadyForArchive(XLogRecPtr flushRecPtr)
{
XLogSegNo latest_boundary_seg;
diff --git a/src/backend/postmaster/walwriter.c b/src/backend/postmaster/walwriter.cindex 6a1e16edc2..626fae8454 100644--- a/src/backend/postmaster/walwriter.c+++ b/src/backend/postmaster/walwriter.c@@ -248,13 +248,6 @@ WalWriterMain(void)
/* Process any signals received recently */
HandleWalWriterInterrupts();
- /*- * Notify the archiver of any WAL segments that are ready. We do this- * here to handle a race condition where WAL is flushed to disk prior- * to registering the segment boundary.- */- NotifySegmentsReadyForArchive(GetFlushRecPtr());-
/*
* Do what we're here for; then, if XLogBackgroundFlush() found useful
* work to do, reset hibernation counter.
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.hindex 6b6ae81c2d..0a8ede700d 100644--- a/src/include/access/xlog.h+++ b/src/include/access/xlog.h@@ -315,7 +315,6 @@ extern XLogRecPtr GetInsertRecPtr(void);
extern XLogRecPtr GetFlushRecPtr(void);
extern XLogRecPtr GetLastImportantRecPtr(void);
extern void RemovePromoteSignalFiles(void);
-extern void NotifySegmentsReadyForArchive(XLogRecPtr flushRecPtr);
extern bool PromoteIsTriggered(void);
extern bool CheckPromoteSignal(void);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bossartn@amazon.com, andres@anarazel.de, alvherre@alvh.no-ip.org, robertmhaas@gmail.com, horikyota.ntt@gmail.com, x4mmm@yandex-team.ru, a.lubennikova@postgrespro.ru, hlinnaka@iki.fi, matsumura.ryo@fujitsu.com, masao.fujii@gmail.com
Subject: Re: archive status ".ready" files may be created too early
In-Reply-To: <282F96BA-948C-41DD-8A68-3E5ACE4D92C5@amazon.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox