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?
Attachments:
v1-0001-Rename-vacuum_open_relation-argument-to-lockmode.patchapplication/octet-stream; name=v1-0001-Rename-vacuum_open_relation-argument-to-lockmode.patchDownload
From 465197f21ed8e82bc8045bbce6a7ab56aa305ada Mon Sep 17 00:00:00 2001
From: reshke kirill <reshke@double.cloud>
Date: Fri, 2 Aug 2024 08:23:10 +0000
Subject: [PATCH v1] Rename vacuum_open_relation argument to lockmode.
---
src/backend/commands/vacuum.c | 6 +++---
src/include/commands/vacuum.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 48f8eab202..2b000d944c 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -756,7 +756,7 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
*/
Relation
vacuum_open_relation(Oid relid, RangeVar *relation, bits32 options,
- bool verbose, LOCKMODE lmode)
+ bool verbose, LOCKMODE lockmode)
{
Relation rel;
bool rel_lock = true;
@@ -774,8 +774,8 @@ vacuum_open_relation(Oid relid, RangeVar *relation, bits32 options,
* in non-blocking mode, before calling try_relation_open().
*/
if (!(options & VACOPT_SKIP_LOCKED))
- rel = try_relation_open(relid, lmode);
- else if (ConditionalLockRelationOid(relid, lmode))
+ rel = try_relation_open(relid, lockmode);
+ else if (ConditionalLockRelationOid(relid, lockmode))
rel = try_relation_open(relid, NoLock);
else
{
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 759f9a87d3..328f2b1223 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -344,7 +344,7 @@ extern bool vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
bits32 options);
extern Relation vacuum_open_relation(Oid relid, RangeVar *relation,
bits32 options, bool verbose,
- LOCKMODE lmode);
+ LOCKMODE lockmode);
extern IndexBulkDeleteResult *vac_bulkdel_one_index(IndexVacuumInfo *ivinfo,
IndexBulkDeleteResult *istat,
TidStore *dead_items,
--
2.34.1
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.
Attachments:
v2-0001-Rename-LOCKMODE-type-vairables-to-lockmode.patchapplication/octet-stream; name=v2-0001-Rename-LOCKMODE-type-vairables-to-lockmode.patchDownload
From eb90a298c3910db3659c48d3d06f0aa56250c34b Mon Sep 17 00:00:00 2001
From: reshke kirill <reshke@double.cloud>
Date: Fri, 2 Aug 2024 08:23:10 +0000
Subject: [PATCH v2] Rename LOCKMODE type vairables to lockmode.
This way code looks more consistent.
---
src/backend/commands/vacuum.c | 6 +++---
src/backend/storage/lmgr/lock.c | 10 +++++-----
src/include/commands/vacuum.h | 2 +-
src/include/storage/lock.h | 4 ++--
4 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 48f8eab202..2b000d944c 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -756,7 +756,7 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
*/
Relation
vacuum_open_relation(Oid relid, RangeVar *relation, bits32 options,
- bool verbose, LOCKMODE lmode)
+ bool verbose, LOCKMODE lockmode)
{
Relation rel;
bool rel_lock = true;
@@ -774,8 +774,8 @@ vacuum_open_relation(Oid relid, RangeVar *relation, bits32 options,
* in non-blocking mode, before calling try_relation_open().
*/
if (!(options & VACOPT_SKIP_LOCKED))
- rel = try_relation_open(relid, lmode);
- else if (ConditionalLockRelationOid(relid, lmode))
+ rel = try_relation_open(relid, lockmode);
+ else if (ConditionalLockRelationOid(relid, lockmode))
rel = try_relation_open(relid, NoLock);
else
{
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 0400a50777..02408cc022 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -567,11 +567,11 @@ ProcLockHashCode(const PROCLOCKTAG *proclocktag, uint32 hashcode)
* Given two lock modes, return whether they would conflict.
*/
bool
-DoLockModesConflict(LOCKMODE mode1, LOCKMODE mode2)
+DoLockModesConflict(LOCKMODE lockmode1, LOCKMODE lockmode2)
{
LockMethod lockMethodTable = LockMethods[DEFAULT_LOCKMETHOD];
- if (lockMethodTable->conflictTab[mode1] & LOCKBIT_ON(mode2))
+ if (lockMethodTable->conflictTab[lockmode1] & LOCKBIT_ON(lockmode2))
return true;
return false;
@@ -4057,11 +4057,11 @@ GetRunningTransactionLocks(int *nlocks)
/* Provide the textual name of any lock mode */
const char *
-GetLockmodeName(LOCKMETHODID lockmethodid, LOCKMODE mode)
+GetLockmodeName(LOCKMETHODID lockmethodid, LOCKMODE lockmode)
{
Assert(lockmethodid > 0 && lockmethodid < lengthof(LockMethods));
- Assert(mode > 0 && mode <= LockMethods[lockmethodid]->numLockModes);
- return LockMethods[lockmethodid]->lockModeNames[mode];
+ Assert(lockmode > 0 && lockmode <= LockMethods[lockmethodid]->numLockModes);
+ return LockMethods[lockmethodid]->lockModeNames[lockmode];
}
#ifdef LOCK_DEBUG
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 759f9a87d3..328f2b1223 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -344,7 +344,7 @@ extern bool vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
bits32 options);
extern Relation vacuum_open_relation(Oid relid, RangeVar *relation,
bits32 options, bool verbose,
- LOCKMODE lmode);
+ LOCKMODE lockmode);
extern IndexBulkDeleteResult *vac_bulkdel_one_index(IndexVacuumInfo *ivinfo,
IndexBulkDeleteResult *istat,
TidStore *dead_items,
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index cc1f6e78c3..adf31ae327 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -548,7 +548,7 @@ extern void InitLocks(void);
extern LockMethod GetLocksMethodTable(const LOCK *lock);
extern LockMethod GetLockTagsMethodTable(const LOCKTAG *locktag);
extern uint32 LockTagHashCode(const LOCKTAG *locktag);
-extern bool DoLockModesConflict(LOCKMODE mode1, LOCKMODE mode2);
+extern bool DoLockModesConflict(LOCKMODE lockmode1, LOCKMODE lockmode2);
extern LockAcquireResult LockAcquire(const LOCKTAG *locktag,
LOCKMODE lockmode,
bool sessionLock,
@@ -589,7 +589,7 @@ extern LockData *GetLockStatusData(void);
extern BlockedProcsData *GetBlockerStatusData(int blocked_pid);
extern xl_standby_lock *GetRunningTransactionLocks(int *nlocks);
-extern const char *GetLockmodeName(LOCKMETHODID lockmethodid, LOCKMODE mode);
+extern const char *GetLockmodeName(LOCKMETHODID lockmethodid, LOCKMODE lockmode);
extern void lock_twophase_recover(TransactionId xid, uint16 info,
void *recdata, uint32 len);
--
2.34.1
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 BapatAttached 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
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")
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
On Thu, Jan 9, 2025 at 5:30 PM Daniel Gustafsson <daniel@yesql.se> wrote:
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.
I agree about most of the changes however
I found some more
static const struct
{
LOCKMODE hwlock;
int lockstatus;
int updstatus;
}tupleLockExtraInfo[MaxLockTupleMode + 1] =
hwlock should be hwlockmode?
this one looks useful. Variable name hwlock indicates some kind of
lock (object) not a mode in which it should be locked. This is
especially so when the next variable is named lockstatus indicating
status of the lock.
--
Best Wishes,
Ashutosh Bapat
On Thu, Jan 09, 2025 at 01:00:06PM +0100, Daniel Gustafsson wrote:
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.
Same here, let's leave things as they are. I've checked the patch and
it's kind of clear what these variables mean in the context of the
code where they are declared.
--
Michael