On Thu, Nov 16, 2023 at 3:11 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > > I just noticed that 0003 does some changes to > TransactionGroupUpdateXidStatus() that haven't been adequately > explained AFAICS. How do you know that these changes are safe?
IMHO this is safe as well as logical to do w.r.t. performance. It's safe because whenever we are updating any page in a group we are acquiring the respective bank lock in exclusive mode and in extreme cases if there are pages from different banks then we do switch the lock as well before updating the pages from different groups. And, we do not wake any process in a group unless we have done the status update for all the processes so there could not be any race condition as well. Also, It should not affect the performance adversely as well and this will not remove the need for group updates. The main use case of group update is that it will optimize a situation when most of the processes are contending for status updates on the same page and processes that are waiting for status updates on different pages will go to different groups w.r.t. that page, so in short in a group on best effort basis we are trying to have the processes which are waiting to update the same clog page that mean logically all the processes in the group will be waiting on the same bank lock. In an extreme situation if there are processes in the group that are trying to update different pages or even pages from different banks then we are handling it well by changing the lock. Although someone may raise a concern that in cases where there are processes that are waiting for different bank locks then after releasing one lock why not wake up those processes, I think that is not required because that is the situation we are trying to avoid where there are processes trying to update different are in the same group so there is no point in adding complexity to optimize that case. > 0001 contains one typo in the docs, "cotents". > > I'm not a fan of the fact that some CLOG sizing macros moved to clog.h, > leaving others in clog.c. Maybe add commentary cross-linking both. > Alternatively, perhaps allowing xact_buffers to grow beyond 65536 up to > the slru.h-defined limit of 131072 is not that bad, even if it's more > than could possibly be needed for xact_buffers; nobody is going to use > 64k buffers, since useful values are below a couple thousand anyhow. I agree, that allowing xact_buffers to grow beyond 65536 up to the slru.h-defined limit of 131072 is not that bad, so I will change that in the next version. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com