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.z...@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.soko...@postgrespro.ru
> > <mailto:y.soko...@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
>
>
