Thread: Small refactoring around vacuum_open_relation

I hate to be "that guy", but there are many places in sources where we use
LOCKMODE lockmode; variable and exactly one where we use LOCKMODE
lmode: it is vacuum_open_relation function.
Is it worth a patch?

On Fri, Aug 2, 2024 at 1:55 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> I hate to be "that guy", but there are many places in sources where we use
> LOCKMODE lockmode; variable and exactly one where we use LOCKMODE
> lmode: it is vacuum_open_relation function.

There are more instances of LOCKMODE lmode; I spotted one in plancat.c as well.

Case1:
toast_get_valid_index(Oid toastoid, LOCKMODE lock)
toast_close_indexes(Relation *toastidxs, int num_indexes, LOCKMODE lock)
GetLockmodeName(LOCKMETHODID lockmethodid, LOCKMODE mode)
LOCKMODE mode = 0;

Case 2:
qualified variable names like
LOCKMODE heapLockmode;
LOCKMODE memlockmode;
LOCKMODE table_lockmode;
LOCKMODE parentLockmode;
LOCKMODE cmd_lockmode = AccessExclusiveLock; /* default for compiler */
LOCK_PRINT(const char *where, const LOCK *lock, LOCKMODE type)

case3: some that have two LOCKMODE instances like
DoLockModesConflict(LOCKMODE mode1, LOCKMODE mode2)

> Is it worth a patch?

When I see a variable with name lockmode, I know it's of type
LOCKMODE. So changing the Case1 may be worth it. It's not a whole lot
of code churn as well. May be patch backbranches.

Case2 we should leave as is since the variable name has lockmode in it.

Case3, worth changing to lockmode1 and lockmode2.

--
Best Wishes,
Ashutosh Bapat

Thanks for review!

On Fri, 2 Aug 2024 at 14:31, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Fri, Aug 2, 2024 at 1:55 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
> >
> > I hate to be "that guy", but there are many places in sources where we use
> > LOCKMODE lockmode; variable and exactly one where we use LOCKMODE
> > lmode: it is vacuum_open_relation function.
>
> There are more instances of LOCKMODE lmode; I spotted one in plancat.c as well.

Nice catch!

> Case1:
> toast_get_valid_index(Oid toastoid, LOCKMODE lock)
> toast_close_indexes(Relation *toastidxs, int num_indexes, LOCKMODE lock)
> GetLockmodeName(LOCKMETHODID lockmethodid, LOCKMODE mode)
> LOCKMODE mode = 0;
> Case 2:
> qualified variable names like
> LOCKMODE heapLockmode;
> LOCKMODE memlockmode;
> LOCKMODE table_lockmode;
> LOCKMODE parentLockmode;
> LOCKMODE cmd_lockmode = AccessExclusiveLock; /* default for compiler */
> LOCK_PRINT(const char *where, const LOCK *lock, LOCKMODE type)
>
> case3: some that have two LOCKMODE instances like
> DoLockModesConflict(LOCKMODE mode1, LOCKMODE mode2)

Nice catch!

> > Is it worth a patch?
>
> When I see a variable with name lockmode, I know it's of type
> LOCKMODE. So changing the Case1 may be worth it. It's not a whole lot
> of code churn as well. May be patch backbranches.
>
> Case2 we should leave as is since the variable name has lockmode in it.
+1

> Case3, worth changing to lockmode1 and lockmode2.
Agree
> --
> Best Wishes,
> Ashutosh Bapat

Attached v2 patch with your suggestions addressed.