Re: [RFC] Lock-free XLog Reservation from WAL

HI Zhiguo
> Maybe we could leave the NUM_XLOGINSERT_LOCKS unchanged in this patch,
> as it is not a hard dependency of the lock-free algorithm. And when this
> patch has been fully accepted, we could then investigate the more proper
> way of increasing NUM_XLOGINSERT_LOCKS. WDYT?
If the value is not a strong dependency, then the best way is not to change
it.

Thanks

On Mon, Jan 6, 2025 at 4:49 PM Zhou, Zhiguo <zhiguo(dot)zhou(at)intel(dot)com> wrote:

> Maybe we could leave the NUM_XLOGINSERT_LOCKS unchanged in this patch,
> as it is not a hard dependency of the lock-free algorithm. And when this
> patch has been fully accepted, we could then investigate the more proper
> way of increasing NUM_XLOGINSERT_LOCKS. WDYT?
>
> On 1/6/2025 4:35 PM, wenhui qiu wrote:
> > HI Zhiguo
> > Thank you for your reply ,Then you'll have to prove that 128 is the
> > optimal value, otherwise they'll have a hard time agreeing with you on
> > this patch.
> >
> > Thanks
> >
> > On Mon, Jan 6, 2025 at 2:46 PM Zhou, Zhiguo <zhiguo(dot)zhou(at)intel(dot)com
> > <mailto:zhiguo(dot)zhou(at)intel(dot)com>> wrote:
> >
> > Hi Yura and Wenhui,
> >
> > Thanks for kindly reviewing this work!
> >
> > On 1/3/2025 9:01 PM, wenhui qiu wrote:
> > > Hi
> > > Thank you for your path，NUM_XLOGINSERT_LOCKS increase to
> > 128，I
> > > think it will be challenged，do we make it guc ？
> > >
> >
> > I noticed there have been some discussions (for example, [1] and its
> > responses) about making NUM_XLOGINSERT_LOCKS a GUC, which seems to
> be a
> > controversial proposal. Given that, we may first focus on the
> lock-free
> > XLog reservation implementation, and leave the increase of
> > NUM_XLOGINSERT_LOCKS for a future patch, where we would provide more
> > quantitative evidence for the various implementations. WDYT?
> >
> >
> > > On Fri, 3 Jan 2025 at 20:36, Yura Sokolov
> > <y(dot)sokolov(at)postgrespro(dot)ru <mailto:y(dot)sokolov(at)postgrespro(dot)ru>
> > > <mailto:y(dot)sokolov(at)postgrespro(dot)ru
> > <mailto:y(dot)sokolov(at)postgrespro(dot)ru>>> wrote:
> > >
> > > Good day, Zhiguo.
> > >
> > > Idea looks great.
> > >
> > > Minor issue:
> > > - you didn't remove use of `insertpos_lck` from
> > `ReserveXLogSwitch`.
> > >
> > > I initially thought it became un-synchronized against
> > > `ReserveXLogInsertLocation`, but looking closer I found it is
> > > synchronized with `WALInsertLockAcquireExclusive`.
> > > Since there are no other `insertpos_lck` usages after your
> > patch, I
> > > don't see why it should exists and be used in
> > `ReserveXLogSwitch`.
> > >
> > > Still I'd prefer to see CAS loop in this place to be
> > consistent with
> > > other non-locking access. And it will allow to get rid of
> > > `WALInsertLockAcquireExclusive`, (though probably it is not a
> big
> > > issue).
> > >
> >
> > Exactly, it should be safe to remove `insertpos_lck`. And I agree
> with
> > you on getting rid of `WALInsertLockAcquireExclusive` with CAS loop
> > which should significantly reduce the synchronization cost here
> > especially when we intend to increase NUM_XLOGINSERT_LOCKS. I will
> try
> > it in the next version of patch.
> >
> >
> > > Major issue:
> > > - `SetPrevRecPtr` and `GetPrevRecPtr` do non-atomic write/
> > read with on
> > > platforms where MAXALIGN != 8 or without native 64 load/
> > store. Branch
> > > with 'memcpy` is rather obvious, but even pointer de-
> > referencing on
> > > "lucky case" is not safe either.
> > >
> > > I have no idea how to fix it at the moment.
> > >
> >
> > Indeed, non-atomic write/read operations can lead to safety issues in
> > some situations. My initial thought is to define a bit near the
> > prev-link to flag the completion of the update. In this way, we could
> > allow non-atomic or even discontinuous write/read operations on the
> > prev-link, while simultaneously guaranteeing its atomicity through
> > atomic operations (as well as memory barriers) on the flag bit. What
> do
> > you think of this as a viable solution?
> >
> >
> > > Readability issue:
> > > - It would be good to add `Assert(ptr >= upto)` into
> > `GetXLogBuffer`.
> > > I had hard time to recognize `upto` is strictly not in the
> > future.
> > > - Certainly, final version have to have fixed and improved
> > comments.
> > > Many patch's ideas are strictly non-obvious. I had hard time
> to
> > > recognize patch is not a piece of ... (excuse me for the swear
> > > sentence).
> >
> > Thanks for the suggestion and patience. It's really more readable
> after
> > inserting the assertion, I will fix it and improve other comments in
> > the
> > following patches.
> >
> >
> > > Indeed, patch is much better than it looks on first sight.
> > > I came with alternative idea yesterday, but looking closer to
> > your
> > > patch
> > > today I see it is superior to mine (if atomic access will be
> > fixed).
> > >
> > > ----
> > >
> > > regards,
> > > Yura Sokolov aka funny-falcon
> > >
> > >
> >
> > Regards,
> > Zhiguo
> >
> >
> > [1] https://www.postgresql.org/message-
> > id/2266698.1704854297%40sss.pgh.pa.us <https://www.postgresql.org/
> > message-id/2266698.1704854297%40sss.pgh.pa.us>
> >
>
>