Feedback-ID: id4a34324:Fastmail
Date: Mon, 12 Feb 2024 12:18:53 -0800
From: Andres Freund <andres@anarazel.de>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Dilip Kumar <dilipbalaut@gmail.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org,
	Nathan Bossart <nathandbossart@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: Improve WALRead() to suck data directly from WAL buffers when
 possible
Message-ID: <20240212201853.r6dl25onohrwagxt@awork3.anarazel.de>
References: <CALj2ACW4oZzmg6joL0ppQN=cCGOLaVKJknc3OLqmo8Q+cGDRKw@mail.gmail.com>
 <202401301731.o3cthlcry4ve@alvherre.pgsql>
 <CALj2ACVSR9cgFW=OMxd4mtLceAmucVYHJ2anRAoG3y6OSyLR+g@mail.gmail.com>
 <db1e59dbdaf0c2fe722f1c5dd534c7cb048ba606.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <db1e59dbdaf0c2fe722f1c5dd534c7cb048ba606.camel@j-davis.com>
Archived-At: <https://www.postgresql.org/message-id/20240212201853.r6dl25onohrwagxt%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2024-02-12 11:33:24 -0800, Jeff Davis wrote:
> On Wed, 2024-01-31 at 14:30 +0530, Bharath Rupireddy wrote:
> > Please see the attached v22 patch set.
>
> Committed 0001.

Yay, I think this is very cool. There are plenty other improvements than can
be based on this...


One thing I'm a bit confused in the code is the following:

+    /*
+     * Don't read past the available WAL data.
+     *
+     * Check using local copy of LogwrtResult. Ordinarily it's been updated by
+     * the caller when determining how far to read; but if not, it just means
+     * we'll read less data.
+     *
+     * XXX: the available WAL could be extended to the WAL insert pointer by
+     * calling WaitXLogInsertionsToFinish().
+     */
+    upto = Min(startptr + count, LogwrtResult.Write);
+    nbytes = upto - startptr;

Shouldn't it pretty much be a bug to ever encounter this? There aren't
equivalent checks in WALRead(), so any user of WALReadFromBuffers() that then
falls back to WALRead() is just going to send unwritten data.

ISTM that this should be an assertion or error.

Greetings,

Andres Freund