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

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
Date: 2021-08-02 21:41:39
Message-ID: 202108022141.f7ynln3h4e3a@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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].


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 —

Attachment Content-Type Size
v3-0001-Avoid-creating-archive-status-.ready-files-too-ea.patch text/x-diff 18.6 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
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