Postgres Pro
Downloads
Home   >   mailing lists

Thread: Small refactoring around vacuum_open_relation

Small refactoring around vacuum_open_relation

From
Kirill Reshke
Date:
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?
Attachment

Re: Small refactoring around vacuum_open_relation

From
Ashutosh Bapat
Date:
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

Re: Small refactoring around vacuum_open_relation

From
Kirill Reshke
Date:
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.
Attachment

Re: Small refactoring around vacuum_open_relation

From
Ashutosh Bapat
Date:
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

Re: Small refactoring around vacuum_open_relation

From
Alvaro Herrera
Date:
On 2025-Jan-09, Ashutosh Bapat wrote:

> Sorry for reviewing late. The patch looks ok.

Dunno what others think, this seems useless churn to me.

> 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().

Eh, and right here it is when things snowball and now the whole tree is
under duress because of a consistency argument of dubious value.

Heck, I see even fixing typos as problematic, because there comes the
time when somebody needs to make a backpatch and they find there's a
conflict to fix because of a typo fix.  IMO if you really want to fix
typos, then the committer should apply such fixes to all live branches
where they apply, so that any later backpatching is not bothered by it.

If you're patching the source code for other reasons, then by all means
fix inconsistencies, typos, etc all you want.  Otherwise, please leave
things alone _or_ backpatch such fixes.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El que vive para el futuro es un iluso, y el que vive para el pasado,
un imbécil" (Luis Adler, "Los tripulantes de la noche")

Re: Small refactoring around vacuum_open_relation

From
Daniel Gustafsson
Date:
> On 9 Jan 2025, at 11:45, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> Dunno what others think, this seems useless churn to me.

I agree, I don't see this providing enough value to warrant the changes.

--
Daniel Gustafsson