Update outdated references to SLRU ControlLock

Started by Julien Rouhaud8 months ago8 messageshackers
Jump to latest
#1Julien Rouhaud
rjuju123@gmail.com

Hi,

I just noticed that some references so the now removed SLRU ControlLock were
still present in slru.[ch]. I tried to improve the situation with the attached
patch by using the new "bank control lock" name that is used nearby.

Note that the main comment of slru.c still has one paragraph that mentions
"bank control lock" consistently before switching to just "control lock" in the
next paragraph. I'm assuming that it's ok in that context as it seems clear to
me that those are the same thing, just spelled with a less verbose name.

Attachments:

0001-Update-outdated-references-to-SLRU-ControlLock.patchtext/plain; charset=us-asciiDownload+5-6
#2Michael Paquier
michael@paquier.xyz
In reply to: Julien Rouhaud (#1)
Re: Update outdated references to SLRU ControlLock

On Mon, Sep 01, 2025 at 11:32:41AM +0800, Julien Rouhaud wrote:

Note that the main comment of slru.c still has one paragraph that mentions
"bank control lock" consistently before switching to just "control lock" in the
next paragraph. I'm assuming that it's ok in that context as it seems clear to
me that those are the same thing, just spelled with a less verbose name.

Good catch, right.

I am not seeing "control" used much as a term (3 times on HEAD).
There is a lot of "bank lock" or "SLRU bank lock", both being mixed
depending on the parts of the code using SLRUs (multixact, predicates,
etc.).

"SLRU bank lock" speaks a bit better to me, as the concept relates
to.. SLRUs, but that's mostly a matter of taste between the three
wordings, I guess. Do you have a preference?

Not saying that one is better than the other as they're all used, just
noticed the difference in the comments, with all the terms referring
to the same thing.
--
Michael

#3Julien Rouhaud
rjuju123@gmail.com
In reply to: Michael Paquier (#2)
Re: Update outdated references to SLRU ControlLock

On Mon, Sep 01, 2025 at 02:05:56PM +0900, Michael Paquier wrote:

On Mon, Sep 01, 2025 at 11:32:41AM +0800, Julien Rouhaud wrote:

Note that the main comment of slru.c still has one paragraph that mentions
"bank control lock" consistently before switching to just "control lock" in the
next paragraph. I'm assuming that it's ok in that context as it seems clear to
me that those are the same thing, just spelled with a less verbose name.

Good catch, right.

I am not seeing "control" used much as a term (3 times on HEAD).
There is a lot of "bank lock" or "SLRU bank lock", both being mixed
depending on the parts of the code using SLRUs (multixact, predicates,
etc.).

Yes, and some other parts simply mentions "lock" (eg TransactionIdGetStatus)

"SLRU bank lock" speaks a bit better to me, as the concept relates
to.. SLRUs, but that's mostly a matter of taste between the three
wordings, I guess. Do you have a preference?

I don't really have a preference. Bank lock is shorter but may be a bit more
obscure, especially outside slru.c, so using "SLRU bank lock" could be better
indeed.

#4Michael Paquier
michael@paquier.xyz
In reply to: Julien Rouhaud (#3)
Re: Update outdated references to SLRU ControlLock

On Mon, Sep 01, 2025 at 01:19:41PM +0800, Julien Rouhaud wrote:

I don't really have a preference. Bank lock is shorter but may be a bit more
obscure, especially outside slru.c, so using "SLRU bank lock" could be better
indeed.

Okay, I have used "SLRU bank lock" and backpatched that down to v17
where it matters.
--
Michael

#5Julien Rouhaud
rjuju123@gmail.com
In reply to: Michael Paquier (#4)
Re: Update outdated references to SLRU ControlLock

On Wed, Sep 03, 2025 at 10:21:54AM +0900, Michael Paquier wrote:

On Mon, Sep 01, 2025 at 01:19:41PM +0800, Julien Rouhaud wrote:

I don't really have a preference. Bank lock is shorter but may be a bit more
obscure, especially outside slru.c, so using "SLRU bank lock" could be better
indeed.

Okay, I have used "SLRU bank lock" and backpatched that down to v17
where it matters.

Thanks a lot!

#6Julien Rouhaud
rjuju123@gmail.com
In reply to: Julien Rouhaud (#5)
Re: Update outdated references to SLRU ControlLock

Hi,

On Wed, Sep 03, 2025 at 10:22:37AM +0800, Julien Rouhaud wrote:

On Wed, Sep 03, 2025 at 10:21:54AM +0900, Michael Paquier wrote:

Okay, I have used "SLRU bank lock" and backpatched that down to v17
where it matters.

I caught a couple of other minor outdated things in the comments when finishing
rebasing my own work:

- long_segment_names has been added to SimpleLruInit
- SlruRecentlyUsed is not a macro anymore

Simple patch attached.

Attachments:

0001-Fix-a-few-outdated-SLRU-comments.patchtext/plain; charset=us-asciiDownload+2-2
#7Michael Paquier
michael@paquier.xyz
In reply to: Julien Rouhaud (#6)
Re: Update outdated references to SLRU ControlLock

On Fri, Sep 05, 2025 at 09:47:24AM +0800, Julien Rouhaud wrote:

I caught a couple of other minor outdated things in the comments when finishing
rebasing my own work:

- long_segment_names has been added to SimpleLruInit
- SlruRecentlyUsed is not a macro anymore

Right. The source of each mistake is different, both affect v17~.
Applied.
--
Michael

#8Julien Rouhaud
rjuju123@gmail.com
In reply to: Michael Paquier (#7)
Re: Update outdated references to SLRU ControlLock

On Fri, Sep 05, 2025 at 02:11:18PM +0900, Michael Paquier wrote:

On Fri, Sep 05, 2025 at 09:47:24AM +0800, Julien Rouhaud wrote:

I caught a couple of other minor outdated things in the comments when finishing
rebasing my own work:

- long_segment_names has been added to SimpleLruInit
- SlruRecentlyUsed is not a macro anymore

Right. The source of each mistake is different, both affect v17~.
Applied.

Thanks!