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.z...@intel.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.z...@intel.com > > <mailto: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> > > > <mailto: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 <https://www.postgresql.org/ > > message-id/2266698.1704854297%40sss.pgh.pa.us> > > > >
