On Fri, Jul 31, 2015 at 10:01 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > > On Fri, Jul 31, 2015 at 11:41 AM, Fabrízio de Royes Mello wrote: > >> We usually don't compare lock values that way, i.e. there's not > >> guaranteed to be a strict monotonicity between lock levels. I don't > >> really agree with that policy, but it's nonetheless there. > > > > And how is the better way to compare lock values to get the highest lock > > level? Perhaps creating a function to compare lock levels? > > I guess that this is exactly what Andres has in mind, aka something > like LockModeCompare(lockmode, lockmode) that returns {-1,0,1} > depending on which lock is higher on the hierarchy. This would do > exactly what your patch is doing though, except that this will > localize the comparison operators in lock.c. Though I am seeing at > quick glance a couple of places already do such comparisons: > backend/commands/tablecmds.c: if (cmd_lockmode > lockmode) > backend/storage/lmgr/lock.c: lockmode > RowExclusiveLock) > backend/storage/lmgr/lock.c: if (lockmode >= AccessExclusiveLock && > backend/access/heap/heapam.c: Assert(lockmode >= NoLock && lockmode > < MAX_LOCKMODES); > backend/access/heap/heapam.c: Assert(lockmode >= NoLock && lockmode > < MAX_LOCKMODES); > backend/access/heap/heapam.c: Assert(lockmode >= NoLock && lockmode > < MAX_LOCKMODES); > backend/access/index/indexam.c: Assert(lockmode >= NoLock && > lockmode < MAX_LOCKMODES); > All of them are just sanity checks, except the one in tablecmds.c is > not (2dbbda0). Hence I am thinking that this is not really a problem > this patch should tackle by itself... >
I did it in the attached version of the patch... But I don't know if the names are good so fell free to suggest others if you dislike of my choice.
In this patch I didn't change all lockmode comparison places previous pointed by you, but I can change it maybe adding other method called LockModeIsValid(lockmode) to do the comparison "lockmode >= NoLock && lockmode < MAX_LOCKMODES" used in many places.