Re: Catalog invalidations vs catalog scans vs ScanPgRelation() - Mailing list pgsql-hackers
From | Andres Freund |
---|---|
Subject | Re: Catalog invalidations vs catalog scans vs ScanPgRelation() |
Date | |
Msg-id | 20200407072418.ccvnyjbrktyi3rzc@alap3.anarazel.de Whole thread Raw |
In response to | Re: Catalog invalidations vs catalog scans vs ScanPgRelation() (Andres Freund <andres@anarazel.de>) |
List | pgsql-hackers |
Hi, Robert, Tom, it'd be great if you could look through this thread. I think there's a problem here (and it has gotten worse after the introduction of catalog snapshots). Both of you at least dabbled in related code. On 2020-02-29 12:17:07 -0800, Andres Freund wrote: > On 2020-02-28 22:10:52 -0800, Andres Freund wrote: > > So, um. What happens is that doDeletion() does a catalog scan, which > > sets a snapshot. The results of that catalog scan are then used to > > perform modifications. But at that point there's no guarantee that we > > still hold *any* snapshot, as e.g. invalidations can trigger the catalog > > snapshot being released. > > > > I don't see how that's safe. Without ->xmin preventing that, > > intermediate row versions that we did look up could just get vacuumed > > away, and replaced with a different row. That does seem like a serious > > issue? > > > > I think there's likely a lot of places that can hit this? What makes it > > safe for InvalidateCatalogSnapshot()->SnapshotResetXmin() to release > > ->xmin when there previously has been *any* catalog access? Because in > > contrast to normal table modifications, there's currently nothing at all > > forcing us to hold a snapshot between catalog lookups an their > > modifications? > > > > Am I missing something? Or is this a fairly significant hole in our > > arrangements? > > I still think that's true. In a first iteration I hacked around the > problem by explicitly registering a catalog snapshot in > RemoveTempRelations(). That *sometimes* allows to get through the > regression tests without the assertions triggering. The attached two patches (they're not meant to be applied) reliably get through the regression tests. But I suspect I'd have to at least do a CLOBBER_CACHE_ALWAYS run to find all the actually vulnerable places. > But I don't think that's good enough (even if we fixed the other > potential crashes similarly). The only reason that avoids the asserts is > because in nearly all other cases there's also a user snapshot that's > pushed. But that pushed snapshot can have an xmin that's newer than the > catalog snapshot, which means we're still in danger of tids from catalog > scans being outdated. > > My preliminary conclusion is that it's simply not safe to do > SnapshotResetXmin() from within InvalidateCatalogSnapshot(), > PopActiveSnapshot(), UnregisterSnapshotFromOwner() etc. Instead we need > to defer the SnapshotResetXmin() call until at least > CommitTransactionCommand()? Outside of that there ought (with exception > of multi-transaction commands, but they have to be careful anyway) to be > no "in progress" sequences of related catalog lookups/modifications. > > Alternatively we could ensure that all catalog lookup/mod sequences > ensure that the first catalog snapshot is registered. But that seems > like a gargantuan task? I also just noticed comments of this style in a few places * Start a transaction so we can access pg_database, and get a snapshot. * We don't have a use for the snapshot itself, but we're interested in * the secondary effect that it sets RecentGlobalXmin. (This is critical * for anything that reads heap pages, because HOT may decide to prune * them even if the process doesn't attempt to modify any tuples.) followed by code like StartTransactionCommand(); (void) GetTransactionSnapshot(); rel = table_open(DatabaseRelationId, AccessShareLock); scan = table_beginscan_catalog(rel, 0, NULL); which is not safe at all, unfortunately. The snapshot is not pushed/active, therefore invalidations processed e.g. as part of the table_open() could execute a InvalidateCatalogSnapshot(), which in turn would remove the catalog snapshot from the pairing heap and SnapshotResetXmin(). And poof, the backend's xmin is gone. Greetings, Andres Freund
Attachment
pgsql-hackers by date: