Date: Wed, 13 Oct 2021 14:27:58 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amul Sul <sulamul@gmail.com>
Cc: "Bossart, Nathan" <bossartn@amazon.com>,
	"Jakub.Wartak@tomtom.com" <Jakub.Wartak@tomtom.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Ryo Matsumura <matsumura.ryo@fujitsu.com>,
	"andres@anarazel.de" <andres@anarazel.de>,
	"masao.fujii@oss.nttdata.com" <masao.fujii@oss.nttdata.com>,
	"mengjuan.cmj@alibaba-inc.com" <mengjuan.cmj@alibaba-inc.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: prevent immature WAL streaming
Message-ID: <202110131727.7oimptrkzkiv@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAAJ_b97QJH8UNAB=V2NOi-ZidntN5spDnhrU36B_y8yAywEFrg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202110131727.7oimptrkzkiv%40alvherre.pgsql>
Precedence: bulk

On 2021-Oct-13, Amul Sul wrote:

> I have one more question, regarding the need for other global
> variables i.e. abortedRecPtr.  (Sorry for coming back after so long.)
> 
> Instead of abortedRecPtr point, isn't enough to write
> overwrite-contrecord at XLogCtl->lastReplayedEndRecPtr?  I think both
> are pointing to the same location then can't we use
> lastReplayedEndRecPtr instead of abortedRecPtr to write
> overwrite-contrecord and remove need of extra global variable, like
> attached?

I'm a bit fuzzy on the difference "the end+1" and "the start of the next
record".  Are they always the same?  We do have XLogRecPtrToBytePos()
and XLogBytePosToEndRecPtr() to convert unadorned XLogRecPtr values to
"usable byte positions", which suggests to me that the proposed patch
may fail if end+1 is a page or segment boundary.

The other difference is that abortedRecPtr is set if we fail to read a
record, but XLogCtl->lastReplayedEndRecPtr is set even if we read the
record successfully.  So you'd have need a bool flag that the overwrite
contrecord record needs to be written.  Your patch is using the fact
that missingContrecPtr is non-invalid as such a flag ... I can't see
anything wrong with that.  So maybe your patch is okay in this aspect.

> You might wonder why I am so concerned about the global variable. The
> reason is that I am working on another thread[1] where we are trying
> to club all the WAL write operations that happen at the end of
> StartupXLOG into a separate function. In the future, we might want to
> allow executing this function from other processes (e.g.
> Checkpointer). For that, we need to remove the dependency of those WAL
> write operations having on the global variables which are mostly valid
> in the startup process.

Seems a fine goal.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/