Hi, On 2019-05-02 12:59:55 -0400, Tom Lane wrote: > I wrote: > > Andres Freund <and...@anarazel.de> writes: > >> On 2019-05-02 11:41:28 -0400, Tom Lane wrote: > >>> But don't we need to worry about resetting relfrozenxid? > > >> Indexes don't have that though? We couldn't do it for pg_class itself, > >> but that's not a problem here. > > > Hmm. Again, that seems like the sort of assumption that could bite > > us later. But maybe we could add some assertions that the new values > > match the old? I'll experiment.
index_create() has Assert(relfrozenxid == InvalidTransactionId); Assert(relminmxid == InvalidMultiXactId); I think we should just add the same to reindex_index() (modulo accessing relcache rather than local vars, of course)? > Huh, this actually seems to work. The attached is a quick hack for > testing. It gets through check-world straight up and with > xxx_FORCE_RELEASE, and I've verified reindexing pg_class works with > CLOBBER_CACHE_ALWAYS, but it'll be a few hours before I have a full CCA > run. Great. > One interesting thing that turns up in check-world is that if wal_level > is minimal, we have to manually force an XID to be assigned, else > reindexing pg_class fails with "cannot commit a transaction that deleted > files but has no xid" :-(. Perhaps there's some other cleaner place to > do that? Hm. We could replace that RecordTransactionCommit() with an xid assignment or such. But that seems at least as fragile. Or we could expand the logic we have for LogStandbyInvalidations() a few lines below the elog to also be able to handle files. IIRC that was introduced to handle somewhat related issues about being able to run VACUUM (containing invalidations) without an xid. > + * If we're dealing with a mapped index, pg_class.relfilenode doesn't > + * change; instead we have to send the update to the relation mapper. > + * > + * For mapped indexes, we don't actually change the pg_class entry at > all; > + * this is essential when reindexing pg_class itself. That leaves us > with > + * possibly-inaccurate values of relpages etc, but those will be fixed > up > + * later. > */ > if (RelationIsMapped(relation)) > + { > + /* Since we're not updating pg_class, these had better not > change */ > + Assert(classform->relfrozenxid == freezeXid); > + Assert(classform->relminmxid == minmulti); > + Assert(classform->relpersistence == persistence); Hm. Could we additionally assert that we're dealing with an index? The above checks will trigger for tables right now, but I'm not sure that'll always be the case. > + /* > + * In some code paths it's possible that the tuple update we'd > + * otherwise do here is the only thing that would assign an XID > for > + * the current transaction. However, we must have an XID to > delete > + * files, so make sure one is assigned. > + */ > + (void) GetCurrentTransactionId(); Not pretty, but seems tolerable. > - /* These changes are safe even for a mapped relation */ > - if (relation->rd_rel->relkind != RELKIND_SEQUENCE) > - { > - classform->relpages = 0; /* it's empty until further > notice */ > - classform->reltuples = 0; > - classform->relallvisible = 0; > - } > - classform->relfrozenxid = freezeXid; > - classform->relminmxid = minmulti; > - classform->relpersistence = persistence; > + /* These changes are safe even for a mapped relation */ You'd probably have noticed that, but this one probably has to go. Greetings, Andres Freund