Re: VACUUM FULL versus unsafe order-of-operations in DDL commands

On Sun, Aug 14, 2011 at 2:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> So, as the testing rolls on, I started to see some failures in various
> ALTER-FOREIGN-thingy commands.  The cause proved to be that numerous
> places in foreigncmds.c do this:
>
>        tuple = SearchSysCacheCopy(...);
>
>        ... alter the tuple as needed ...
>
>        rel = heap_open(target-catalog, RowExclusiveLock);
>
>        simple_heap_update(rel, &tuple->t_self, tuple);
>
>        heap_close(rel, RowExclusiveLock);
>
> rather than the more common pattern in which the catalog is opened
> first.

Interesting. I vaguely recall flipping some of those around (to put
the lock acquisition first) before committing the 9.1-era foreign
table patch; it didn't seem like an entirely healthy thing to do. But
I didn't really have any concrete notion of why it might be dangerous.

> foreigncmds.c is not hard to fix, but the scary aspect of this is the
> possibility that we've made the same mistake elsewhere, or might do so
> again in future.  Some desultory examination of simple_heap_update and
> simple_heap_delete calls didn't find any other instances, but I am not
> sure I didn't miss anything.  And this seems like an easy trap to fall
> into when refactoring (the current work to try to unify operations like
> ALTER OWNER could easily get into this kind of problem, for instance).
>
> I tried to think of some practical way to mechanically test for this
> type of error, but came up with nothing.  Any ideas?

Hmm. How about setting the TID to an illegal value of some kind when
a catcache tuple is extracted without a table lock? Then any
subsequent update or delete using that tuple would blow up. I think
that'd be way too expensive to do in normal running but perhaps we
could have a #define...

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