On Fri, Aug 2, 2024 at 4:00 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> 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.
Sorry for reviewing late. The patch looks ok.
I found some more
static const struct
{
LOCKMODE hwlock;
int lockstatus;
int updstatus;
}
tupleLockExtraInfo[MaxLockTupleMode + 1] =
hwlock should be hwlockmode?
In vacuum_rel(), get_relation_info(), LOCK_PRINT(), pg_lock_status(),
toast_close_indexes(), toast_get_valid_index(),
toast_open_indexestoast_open_indexes().
Please create a CF entry.
--
Best Wishes,
Ashutosh Bapat
