|From:||Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>|
|To:||"Bossart, Nathan" <bossartn(at)amazon(dot)com>|
|Cc:||Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, "x4mmm(at)yandex-team(dot)ru" <x4mmm(at)yandex-team(dot)ru>, "a(dot)lubennikova(at)postgrespro(dot)ru" <a(dot)lubennikova(at)postgrespro(dot)ru>, "hlinnaka(at)iki(dot)fi" <hlinnaka(at)iki(dot)fi>, "matsumura(dot)ryo(at)fujitsu(dot)com" <matsumura(dot)ryo(at)fujitsu(dot)com>, "masao(dot)fujii(at)gmail(dot)com" <masao(dot)fujii(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: archive status ".ready" files may be created too early|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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  and before your patch
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/
|Next Message||Thomas Munro||2021-08-02 21:52:37||Re: Background writer and checkpointer in crash recovery|
|Previous Message||Zhihong Yu||2021-08-02 21:33:46||Re: Implementing Incremental View Maintenance|