On 2021-Jul-31, Bossart, Nathan wrote:
> This is why I was trying to get away with just using info_lck for
> reading lastNotifiedSeg. ArchNotifyLock is mostly intended to protect
> RecordBoundaryMap. However, since lastNotifiedSeg is used in
> functions like GetLatestRecordBoundarySegment() that access the map, I
> found it easier to reason about things if I knew that it wouldn't
> change as long as I held ArchNotifyLock.
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.)
I haven't tried your repro with this yet.
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].
[1] https://postgr.es/m/20201014.090628.839639906081252194.horikyota.ntt@gmail.com
[2] https://postgr.es/m/EFF40306-8E8A-4259-B181-C84F3F06636C@amazon.com
I also just realized that Kyotaro's patch there also tried to handle the
streaming replication issue I was talking about.
> 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?
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
