Re: ALTER TABLE lock strength reduction patch is unsafe

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.

I don't personally feel that it would matter to usability to keep
locks for those at AccessExclusiveLock, especially since they are
otherwise fast.

Some others might be kept higher also. I'm merely trying to balance
between requests to reduce to minimal theoretical level and fears that
anything less than AccessExclusiveLock is a problem.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services