On 8/2/21, 2:42 PM, "Alvaro Herrera" <alvherre@alvh.no-ip.org> wrote:
> I think it's okay to make lastNotifiedSeg protected by just info_lck,
> and RecordBoundaryMap protected by just ArchNotifyLock. It's okay to
> acquire the spinlock inside the lwlock-protected area, as long as we
> make sure never to do the opposite. (And we sure don't want to hold
> info_lck long enough that a LWLock acquisition would occur in the
> meantime). So I modified things that way, and also added another
> function to set the seg if it's unset, with a single spinlock
> acquisition (rather than acqquire, read, release, acqquire, set,
> release, which sounds like it would have trouble behaving.)
The patch looks good to me.
> I find it highly suspicious that the patch does an archiver notify (i.e.
> creation of the .ready file) in XLogInsertRecord(). Is that a sane
> thing to do? Sounds to me like that should be attempted in XLogFlush
> only. This appeared after Kyotaro's patch at [1] and before your patch
> at [2].
I believe my worry was that we'd miss notifying a segment as soon as
possible if the record was somehow flushed prior to registering the
record boundary in the map. If that's actually impossible, then I
would agree that the extra call to NotifySegmentsReadyForArchive() is
unnecessary.
>> I think the main downside of making lastNotifiedSeg an atomic is that
>> the value we first read in NotifySegmentsReadyForArchive() might not
>> be up-to-date, which means we might hold off creating .ready files
>> longer than necessary.
>
> I'm not sure I understand how this would be a problem. If we block
> somebody from setting a newer value, they'll just set the value
> immediately after we release the lock. Will we reread the value
> afterwards to see if it changed?
I think you are right. If we see an old value for lastNotifiedSeg,
the worst case is that we take the ArchNotifyLock, read
lastNotifiedSeg again (which should then be up-to-date), and then
basically do nothing. I suspect initializing lastNotifiedSeg might
still be a little tricky, though. Do you think it is important to try
to avoid this spinlock for lastNotifiedSeg? IIUC it's acquired at the
end of every call to XLogWrite() already, and we'd still need to
acquire it for the flush pointer, anyway.
Nathan
