Re: findDependentObjects() mutual exclusion vs. MVCC catalog scans

On Tue, Jul 16, 2013 at 9:50 AM, Noah Misch <noah@leadboat.com> wrote:
> Consider this sequence of commands:
>
>     create type rowtype as (c int, d int);
>     create temp table t of rowtype;
>     \c -
>     drop type rowtype cascade;
>
> Since the switch to MVCC catalog scans, it exhibits the following error about
> 10% of the time on my system:
>
> CREATE TYPE
> CREATE TABLE
> You are now connected to database "test" as user "nm".
> ERROR:  XX000: cache lookup failed for relation 17009
> LOCATION:  getRelationDescription, objectaddress.c:2186
>
> With "\c", in general, you may end up executing commands under the new session
> before the old backend has finished exiting.  For this test case specifically,
> the two backends' attempts to drop table "t" regularly overlap.  The old
> backend would drop it within RemoveTempRelationsCallback(), and the new
> backend would cascade from "rowtype" to drop it.  findDependentObjects() deals
> with concurrent deletion attempts by acquiring a lock on each object it will
> delete, then calling systable_recheck_tuple() to determine whether another
> deleter was successful while the current backend was waiting for the lock.
> systable_recheck_tuple() uses the scan snapshot, which really only works if
> that snapshot is SnapshotNow or some other that changes its decision in
> response to concurrent transaction commits.  The switch to MVCC snapshots left
> this mutual exclusion protocol ineffective.
>
> Let's fix this by having systable_recheck_tuple() acquire a fresh catalog MVCC
> snapshot and recheck against that.  I believe it would also be fully safe to
> use SnapshotNow here; however, I'm assuming we would otherwise manage to
> remove SnapshotNow entirely.

I recommend reworking the header comment to avoid mention of
SnapshotNow, since if we get rid of SnapshotNow, the reference might
not be too clear to far-future hackers.

+    /*
+     * For a scan using a non-MVCC snapshot like SnapshotSelf, we would simply
+     * reuse the old snapshot.  So far, the only caller uses MVCC snapshots.
+     */
+    freshsnap = GetCatalogSnapshot(RelationGetRelid(sysscan->heap_rel));

This comment is not very clear, because it doesn't describe what the
code actually does, but rather speculates about what the code could do
if the intention of some future caller were different.  I recommend
adding Assert(IsMVCCSnapshot(scan->xs_snapshot)) and changing the
comment to something like this: "For now, we don't handle the case of
a non-MVCC scan snapshot.  This is adequate for existing uses of this
function, but might need to be changed in the future."

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