Feedback-ID: id4a34324:Fastmail
Date: Mon, 12 Feb 2024 16:11:50 -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: <20240213001150.4uqzh7tinuhvoopl@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>
 <ffd67cc0a7b6b4c06ad36967c4d7eb7825bf293a.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ffd67cc0a7b6b4c06ad36967c4d7eb7825bf293a.camel@j-davis.com>
Archived-At: <https://www.postgresql.org/message-id/20240213001150.4uqzh7tinuhvoopl%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2024-02-12 15:56:19 -0800, Jeff Davis wrote:
> On Mon, 2024-02-12 at 11:33 -0800, Jeff Davis wrote:
> > For 0002 & 0003, I'd like more clarity on how they will actually be
> > used by an extension.
>
> In patch 0002, I'm concerned about calling
> WaitXLogInsertionsToFinish(). It loops through all the locks, but
> doesn't have any early return path or advance any state.

I doubt it'd be too bad - we call that at much much higher frequency during
write heavy OLTP workloads (c.f. XLogFlush()).  It can be a performance issue
there, but only after increasing NUM_XLOGINSERT_LOCKS - before that the
limited number of writers is the limit.  Compared to that walsender shouldn't
be a significant factor.

However, I think it's a very bad idea to call WALReadFromBuffers() from
WALReadFromBuffers(). This needs to be at the caller, not down in
WALReadFromBuffers().

I don't see why we would want to weaken the error condition in
WaitXLogInsertionsToFinish() - I suspect it'd not work correctly to wait for
insertions that aren't yet in progress and it just seems like an API misuse.


> So if it's repeatedly called with the same or similar values it seems like
> it would be doing a lot of extra work.
>
> I'm not sure of the best fix. We could add something to LogwrtResult to
> track a new LSN that represents the highest known point where all
> inserters are finished (in other words, the latest return value of
> WaitXLogInsertionsToFinish()). That seems invasive, though.

FWIW, I think LogwrtResult is an anti-pattern, perhaps introduced due to
misunderstanding how cache coherency works. It's not fundamentally faster to
access non-shared memory. It'd make far more sense to allow lock-free access
to the shared LogwrtResult and

Greetings,

Andres Freund