Re: ALTER TABLE lock strength reduction patch is unsafe

On Mon, Mar 3, 2014 at 11:29 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 3 March 2014 16:06, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Sun, Mar 2, 2014 at 4:50 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> v20 includes slightly re-ordered checks in GetLockLevel, plus more
>>> detailed comments on each group of subcommands.
>>>
>>> Also corrects grammar as noted by Vik.
>>>
>>> Plus adds an example of usage to the docs.
>>
>> This patch contains a one line change to
>> src/bin/pg_dump/pg_backup_archiver.c which seems not to belong.
>>
>> This hunk in ATRewriteCatalogs() looks scary:
>>
>> + /*
>> + * If we think we might need to add/re-add toast tables then
>> + * we currently need to hold an AccessExclusiveLock.
>> + */
>> + if (lockmode < AccessExclusiveLock)
>> + return;
>>
>> It would make sense to me to add an Assert() or elog() check inside
>> the subsequent loop to verify that the lock level is adequate ... but
>> just returning silently seems like a bad idea.
>
> OK, I will check elog.
>
>> I have my doubts about whether it's safe to do AT_AddInherit,
>> AT_DropInherit, AT_AddOf, or AT_DropOf with a full lock. All of those
>> can change the tuple descriptor, and we discussed, back when we did
>> this the first time, the fact that the executor may get *very* unhappy
>> if the tuple descriptor changes in mid-execution. I strongly suspect
>> these are unsafe with less than a full AccessExclusiveLock.
>
> I'm happy to change those if you feel there is insufficient evidence.

Not sure what that means, but yes, I think the lock levels on those
need to be increased.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company