Horton

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

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>, Postgres hackers <pgsql-hackers@lists.postgresql.org>
Date: 2019-05-02T02:19:13Z
Lists: pgsql-hackers 
Hi,

On 2019-05-01 22:01:53 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Well, as I said before, I think hiding the to-be-rebuilt index from the
> > list of indexes is dangerous too - if somebody added an actual
> > CatalogUpdate/Insert (rather than inplace_update) anywhere along the
> > index_build() path, we'd not get an assertion failure anymore, but just
> > an index without the new entry. And given the fragility with HOT hiding
> > that a lot of the time, that seems dangerous to me.
> 
> I think that argument is pretty pointless considering that "REINDEX TABLE
> pg_class" does it this way, and that code is nearly old enough to
> vote.

IMO the reindex_relation() case isn't comparable. By my read the main
purpose there is to prevent inserting into not-yet-rebuilt indexes. The
relevant comment says:
	 * ....  If we are processing pg_class itself, we want to make sure
	 * that the updates do not try to insert index entries into indexes we
	 * have not processed yet.  (When we are trying to recover from corrupted
	 * indexes, that could easily cause a crash.)

Note the *not processed yet* bit.  That's *not* comparable logic to
hiding the index that *already* has been rebuilt, in the middle of
reindex_index().  Yes, the way reindex_relation() is currently coded,
the RelationSetIndexList() *also* hides the already rebuilt index, but
that's hard for reindex_relation() to avoid, because it's outside of
reindex_index().


> +	 * If we are doing one index for reindex_relation, then we will find that
> +	 * the index is already not present in the index list.  In that case we
> +	 * don't have to do anything to the index list here, which we mark by
> +	 * clearing is_pg_class.
>  	 */

> -	RelationSetNewRelfilenode(iRel, persistence);
> +	is_pg_class = (RelationGetRelid(heapRelation) == RelationRelationId);
> +	if (is_pg_class)
> +	{
> +		allIndexIds = RelationGetIndexList(heapRelation);
> +		if (list_member_oid(allIndexIds, indexId))
> +		{
> +			otherIndexIds = list_delete_oid(list_copy(allIndexIds), indexId);
> +			/* Ensure rd_indexattr is valid; see comments for RelationSetIndexList */
> +			(void) RelationGetIndexAttrBitmap(heapRelation, INDEX_ATTR_BITMAP_ALL);
> +		}
> +		else
> +			is_pg_class = false;
> +	}

That's not pretty either :(

Greetings,

Andres Freund

Commits

  1. Remove reindex_catalog test from test schedules.

  2. Remove RelationSetIndexList().

  3. Fix reindexing of pg_class indexes some more.

  4. Run catalog reindexing test from 3dbb317d32 serially, to avoid deadlocks.

  5. Fix potential assertion failure when reindexing a pg_class index.

  6. Fix several recently introduced issues around handling new relation forks.

  7. Avoid VACUUM FULL altogether in initdb.

  8. Introduce replication progress tracking infrastructure.

  9. initdb: remove unnecessary VACUUM FULL