Re: archive status ".ready" files may be created too early

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/