On 2024-Feb-04, Andrey M. Borodin wrote: > This patch uses wording "banks" in comments before banks start to > exist. But as far as I understand, it is expected to be committed > before "banks" patch.
True -- changed that to use ControlLock. > Besides this patch looks good to me. Many thanks for reviewing. On 2024-Feb-05, Dilip Kumar wrote: > > (We also have SimpleLruTruncate, but I think it's not as critical to > > have a barrier there anyhow: accessing a slightly outdated page number > > could only be a problem if a bug elsewhere causes us to try to truncate > > in the current page. I think we only have this code there because we > > did have such bugs in the past, but IIUC this shouldn't happen anymore.) > > +1, I agree with this theory in general. But the below comment in > SimpleLruTrucate in your v3 patch doesn't seem correct, because here > we are checking if the latest_page_number is smaller than the cutoff > if so we log it as wraparound and skip the whole thing and that is > fine even if we are reading with atomic variable and slightly outdated > value should not be a problem but the comment claim that this safe > because we have the same bank lock as SimpleLruZeroPage(), but that's > not true here we will be acquiring different bank locks one by one > based on which slotno we are checking. Am I missing something? I think you're correct. I reworded this comment, so now it says this: /* * An important safety check: the current endpoint page must not be * eligible for removal. This check is just a backstop against wraparound * bugs elsewhere in SLRU handling, so we don't care if we read a slightly * outdated value; therefore we don't add a memory barrier. */ Pushed with those changes. Thank you! Now I'll go rebase the rest of the patch on top. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Having your biases confirmed independently is how scientific progress is made, and hence made our great society what it is today" (Mary Gardiner)