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.
