From: | Fabrízio de Royes Mello <fabriziomello(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. ); |
Date: | 2015-07-31 19:12:14 |
Message-ID: | CAFcNs+pzfjG8q3VL0RZ1dF7xVoON0F_xcKOxyGQUrZw8-dRYAg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Jul 31, 2015 at 10:01 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)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.
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
Attachment | Content-Type | Size |
---|---|---|
alter-table-set-reduce-lock-level-for-autovac-reloptions_v5.patch | text/x-diff | 25.5 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2015-07-31 19:29:39 | Re: [PROPOSAL] VACUUM Progress Checker. |
Previous Message | Robert Haas | 2015-07-31 19:00:49 | Re: 64-bit XIDs again |