Re: REINDEX INDEX results in a crash for an index of pg_class since 9.6

Hi,

On 2019-05-02 12:59:55 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres(at)anarazel(dot)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