Re: [RFC] Lock-free XLog Reservation from WAL - Mailing list pgsql-hackers

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.zhou@intel.com 
> <mailto:zhiguo.zhou@intel.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.sokolov@postgrespro.ru <mailto:y.sokolov@postgrespro.ru>
>      > <mailto:y.sokolov@postgrespro.ru
>     <mailto:y.sokolov@postgrespro.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>
>