Re: ALTER TABLE lock strength reduction patch is unsafe

On Sun, Mar 2, 2014 at 4:50 AM, Simon Riggs <simon@2ndquadrant.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.

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.

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