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