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

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