On Sun, Nov 19, 2023 at 12:39 PM Andrey M. Borodin <x4...@yandex-team.ru> wrote: > > I’ve skimmed through the patch set. Here are some minor notes.
Thanks for the review > > 1. Cycles “for (slotno = bankstart; slotno < bankend; slotno++)” in > SlruSelectLRUPage() and SimpleLruReadPage_ReadOnly() now have identical > comments. I think a little of copy-paste is OK. > But SimpleLruReadPage_ReadOnly() does pgstat_count_slru_page_hit(), while > SlruSelectLRUPage() does not. This is not related to the patch set, just a > code nearby. Do you mean to say we need to modify the comments or you are saying pgstat_count_slru_page_hit() is missing in SlruSelectLRUPage(), if it is later then I can see the caller of SlruSelectLRUPage() is calling pgstat_count_slru_page_hit() and the SlruRecentlyUsed(). > 2. Do we really want these functions doing all the same? > extern bool check_multixact_offsets_buffers(int *newval, void > **extra,GucSource source); > extern bool check_multixact_members_buffers(int *newval, void > **extra,GucSource source); > extern bool check_subtrans_buffers(int *newval, void **extra,GucSource > source); > extern bool check_notify_buffers(int *newval, void **extra, GucSource source); > extern bool check_serial_buffers(int *newval, void **extra, GucSource source); > extern bool check_xact_buffers(int *newval, void **extra, GucSource source); > extern bool check_commit_ts_buffers(int *newval, void **extra,GucSource > source); I tried duplicating these by doing all the work inside the check_slru_buffers() function. But I think it is hard to make them a single function because there is no option to pass an SLRU name in the GUC check hook and IMHO in the check hook we need to print the GUC name, any suggestions on how we can avoid having so many functions? > 3. The name SimpleLruGetSLRUBankLock() contains meaning of SLRU twice. I’d > suggest truncating prefix of infix. > > I do not have hard opinion on any of this items. > I prefer SimpleLruGetBankLock() so that it is consistent with other external functions starting with "SimpleLruGet", are you fine with this name? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
