Re: Support for REINDEX CONCURRENTLY

Hi,

Have you benchmarked the toastrelidx removal stuff in any way? If not,
thats fine, but if yes I'd be interested.

On 2013-03-04 22:33:53 +0900, Michael Paquier wrote:
> --- a/src/backend/access/heap/tuptoaster.c
> +++ b/src/backend/access/heap/tuptoaster.c
> @@ -1238,7 +1238,7 @@ toast_save_datum(Relation rel, Datum value,
>  				 struct varlena * oldexternal, int options)
>  {
>  	Relation	toastrel;
> -	Relation	toastidx;
> +	Relation   *toastidxs;
>  	HeapTuple	toasttup;
>  	TupleDesc	toasttupDesc;
>  	Datum		t_values[3];
> @@ -1257,15 +1257,26 @@ toast_save_datum(Relation rel, Datum value,
>  	char	   *data_p;
>  	int32		data_todo;
>  	Pointer		dval = DatumGetPointer(value);
> +	ListCell   *lc;
> +	int			count = 0;

I find count a confusing name for a loop iteration variable... i of orr,
idxno, or ...

> +	int			num_indexes;
>  
>  	/*
>  	 * Open the toast relation and its index.  We can use the index to check
>  	 * uniqueness of the OID we assign to the toasted item, even though it has
> -	 * additional columns besides OID.
> +	 * additional columns besides OID. A toast table can have multiple identical
> +	 * indexes associated to it.
>  	 */
>  	toastrel = heap_open(rel->rd_rel->reltoastrelid, RowExclusiveLock);
>  	toasttupDesc = toastrel->rd_att;
> -	toastidx = index_open(toastrel->rd_rel->reltoastidxid, RowExclusiveLock);
> +	if (toastrel->rd_indexvalid == 0)
> +		RelationGetIndexList(toastrel);

Hm, I think we should move this into a macro, this is cropping up at
more and more places.


> -		index_insert(toastidx, t_values, t_isnull,
> -					 &(toasttup->t_self),
> -					 toastrel,
> -					 toastidx->rd_index->indisunique ?
> -					 UNIQUE_CHECK_YES : UNIQUE_CHECK_NO);
> +		for (count = 0; count < num_indexes; count++)
> +			index_insert(toastidxs[count], t_values, t_isnull,
> +						 &(toasttup->t_self),
> +						 toastrel,
> +						 toastidxs[count]->rd_index->indisunique ?
> +						 UNIQUE_CHECK_YES : UNIQUE_CHECK_NO);

The indisunique check looks like a copy & pasto to me, albeit not
yours...

>  
>  	/*
>  	 * Create the TOAST pointer value that we'll return
> @@ -1475,10 +1493,13 @@ toast_delete_datum(Relation rel, Datum value)
>  	struct varlena *attr = (struct varlena *) DatumGetPointer(value);
>  	struct varatt_external toast_pointer;
> +	/*
> +	 * We actually use only the first index but taking a lock on all is
> +	 * necessary.
> +	 */

Hm, is it guaranteed that the first index is valid?
> +	foreach(lc, toastrel->rd_indexlist)
> +		toastidxs[count++] = index_open(lfirst_oid(lc), RowExclusiveLock);



>  	/*
> -	 * If we're swapping two toast tables by content, do the same for their
> -	 * indexes.
> +	 * If we're swapping two toast tables by content, do the same for all of
> +	 * their indexes. The swap can actually be safely done only if all the indexes
> +	 * have valid Oids.
>  	 */

What's an index without a valid oid?

>  	if (swap_toast_by_content &&
> -		relform1->reltoastidxid && relform2->reltoastidxid)
> -		swap_relation_files(relform1->reltoastidxid,
> -							relform2->reltoastidxid,
> -							target_is_pg_class,
> -							swap_toast_by_content,
> -							InvalidTransactionId,
> -							InvalidMultiXactId,
> -							mapped_tables);
> +		relform1->reltoastrelid &&
> +		relform2->reltoastrelid)
> +	{
> +		Relation toastRel1, toastRel2;
> +
> +		/* Open relations */
> +		toastRel1 = heap_open(relform1->reltoastrelid, RowExclusiveLock);
> +		toastRel2 = heap_open(relform2->reltoastrelid, RowExclusiveLock);

Shouldn't those be Access Exlusive Locks?

> +		/* Obtain index list if necessary */
> +		if (toastRel1->rd_indexvalid == 0)
> +			RelationGetIndexList(toastRel1);
> +		if (toastRel2->rd_indexvalid == 0)
> +			RelationGetIndexList(toastRel2);
> +
> +		/* Check if the swap is possible for all the toast indexes */

So there's no error being thrown if this turns out not to be possible?

> +		if (!list_member_oid(toastRel1->rd_indexlist, InvalidOid) &&
> +			!list_member_oid(toastRel2->rd_indexlist, InvalidOid) &&
> +			list_length(toastRel1->rd_indexlist) == list_length(toastRel2->rd_indexlist))
> +		{
> +			ListCell *lc1, *lc2;
> +
> +			/* Now swap each couple */
> +			lc2 = list_head(toastRel2->rd_indexlist);
> +			foreach(lc1, toastRel1->rd_indexlist)
> +			{
> +				Oid indexOid1 = lfirst_oid(lc1);
> +				Oid indexOid2 = lfirst_oid(lc2);
> +				swap_relation_files(indexOid1,
> +									indexOid2,
> +									target_is_pg_class,
> +									swap_toast_by_content,
> +									InvalidTransactionId,
> +									InvalidMultiXactId,
> +									mapped_tables);
> +				lc2 = lnext(lc2);
> +			}
> +		}
> +
> +		heap_close(toastRel1, RowExclusiveLock);
> +		heap_close(toastRel2, RowExclusiveLock);
> +	}

>  			/* rename the toast table ... */
> @@ -1528,11 +1563,23 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
>  			RenameRelationInternal(newrel->rd_rel->reltoastrelid,
>  								   NewToastName);
>  
> -			/* ... and its index too */
> -			snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u_index",
> -					 OIDOldHeap);
> -			RenameRelationInternal(toastidx,
> -								   NewToastName);
> +			/* ... and its indexes too */
> +			foreach(lc, toastrel->rd_indexlist)
> +			{
> +				/*
> +				 * The first index keeps the former toast name and the
> +				 * following entries are thought as being concurrent indexes.
> +				 */
> +				if (count == 0)
> +					snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u_index",
> +							 OIDOldHeap);
> +				else
> +					snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u_index_cct%d",
> +							 OIDOldHeap, count);
> +				RenameRelationInternal(lfirst_oid(lc),
> +									   NewToastName);
> +				count++;
> +			}

Hm. It seems wrong that this layer needs to know about _cct.


>  /*
> - * Calculate total on-disk size of a TOAST relation, including its index.
> + * Calculate total on-disk size of a TOAST relation, including its indexes.
>   * Must not be applied to non-TOAST relations.
>   */
>  static int64
> @@ -340,8 +340,8 @@ calculate_toast_table_size(Oid toastrelid)
>  {
> ...
> +	/* Size is evaluated based on the first index available */

Uh. Why? Imo all indexes should be counted.

> +	foreach(lc, toastRel->rd_indexlist)
> +	{
> +		Relation	toastIdxRel;
> +		toastIdxRel = relation_open(lfirst_oid(lc),
> +									AccessShareLock);
> +		for (forkNum = 0; forkNum <= MAX_FORKNUM; forkNum++)
> +			size += calculate_relation_size(&(toastIdxRel->rd_node),
> +											toastIdxRel->rd_backend, forkNum);
> +
> +		relation_close(toastIdxRel, AccessShareLock);
> +	}

> -#define CATALOG_VERSION_NO	201302181
> +#define CATALOG_VERSION_NO	20130219

Think you forgot a digit here ;)



>  	/*
>  	 * This case is currently not supported, but there's no way to ask for it
> -	 * in the grammar anyway, so it can't happen.
> +	 * in the grammar anyway, so it can't happen. This might be called during a
> +	 * conccurrent reindex operation, in this case sufficient locks are already
> +	 * taken on the related relations.
>  	 */

I'd rather change that to something like

/*
 * This case is currently only supported during a concurrent index
 * rebuild, but there is no way to ask for it in the grammar otherwise
 * anyway.
 */

Or similar.

> +
> +/*
> + * index_concurrent_create
> + *
> + * Create an index based on the given one that will be used for concurrent
> + * operations. The index is inserted into catalogs and needs to be built later
> + * on. This is called during concurrent index processing. The heap relation
> + * on which is based the index needs to be closed by the caller.
> + */
> +Oid
> +index_concurrent_create(Relation heapRelation, Oid indOid, char *concurrentName)
> +{
> ...
> +	/*
> +	 * Determine if index is initdeferred, this depends on its dependent
> +	 * constraint.
> +	 */
> +	if (OidIsValid(constraintOid))
> +	{
> +		/* Look for the correct value */
> +		HeapTuple			constTuple;
> +		Form_pg_constraint	constraint;
> +
> +		constTuple = SearchSysCache1(CONSTROID,
> +									 ObjectIdGetDatum(constraintOid));
> +		if (!HeapTupleIsValid(constTuple))
> +			elog(ERROR, "cache lookup failed for constraint %u",
> +				 constraintOid);
> +		constraint = (Form_pg_constraint) GETSTRUCT(constTuple);
> +		initdeferred = constraint->condeferred;
> +
> +		ReleaseSysCache(constTuple);
> +	}

Very, very nitpicky, but I find "constTuple" to be confusing, I thought
at first it meant that the tuple shouldn't be modified or something.


> +	/*
> +	 * Index is considered as a constraint if it is PRIMARY KEY or EXCLUSION.
> +	 */
> +	isconstraint = 	indexRelation->rd_index->indisprimary ||
> +		indexRelation->rd_index->indisexclusion;

unique constraints aren't mattering here?

> +/*
> + * index_concurrent_swap
> + *
> + * Replace old index by old index in a concurrent context. For the time being
> + * what is done here is switching the relation relfilenode of the indexes. If
> + * extra operations are necessary during a concurrent swap, processing should
> + * be added here. AccessExclusiveLock is taken on the index relations that are
> + * swapped until the end of the transaction where this function is called.
> + */
> +void
> +index_concurrent_swap(Oid newIndexOid, Oid oldIndexOid)
> +{
> +	Relation		oldIndexRel, newIndexRel, pg_class;
> +	HeapTuple		oldIndexTuple, newIndexTuple;
> +	Form_pg_class	oldIndexForm, newIndexForm;
> +	Oid				tmpnode;
> +
> +	/*
> +	 * Take an exclusive lock on the old and new index before swapping them.
> +	 */
> +	oldIndexRel = relation_open(oldIndexOid, AccessExclusiveLock);
> +	newIndexRel = relation_open(newIndexOid, AccessExclusiveLock);
> +
> +	/* Now swap relfilenode of those indexes */

Any chance to reuse swap_relation_files here? Not sure whether it would
be beneficial given that it is more generic and normally works on a
relation level...

We probably should remove the fsm of the index altogether after this?

> +	pg_class = heap_open(RelationRelationId, RowExclusiveLock);
> +
> +	oldIndexTuple = SearchSysCacheCopy1(RELOID,
> +										ObjectIdGetDatum(oldIndexOid));
> +	if (!HeapTupleIsValid(oldIndexTuple))
> +		elog(ERROR, "could not find tuple for relation %u", oldIndexOid);
> +	newIndexTuple = SearchSysCacheCopy1(RELOID,
> +										ObjectIdGetDatum(newIndexOid));
> +	if (!HeapTupleIsValid(newIndexTuple))
> +		elog(ERROR, "could not find tuple for relation %u", newIndexOid);
> +	oldIndexForm = (Form_pg_class) GETSTRUCT(oldIndexTuple);
> +	newIndexForm = (Form_pg_class) GETSTRUCT(newIndexTuple);
> +
> +	/* Here is where the actual swapping happens */
> +	tmpnode = oldIndexForm->relfilenode;
> +	oldIndexForm->relfilenode = newIndexForm->relfilenode;
> +	newIndexForm->relfilenode = tmpnode;
> +
> +	/* Then update the tuples for each relation */
> +	simple_heap_update(pg_class, &oldIndexTuple->t_self, oldIndexTuple);
> +	simple_heap_update(pg_class, &newIndexTuple->t_self, newIndexTuple);
> +	CatalogUpdateIndexes(pg_class, oldIndexTuple);
> +	CatalogUpdateIndexes(pg_class, newIndexTuple);
> +
> +	/* Close relations and clean up */
> +	heap_close(pg_class, RowExclusiveLock);
> +
> +	/* The lock taken previously is not released until the end of transaction */
> +	relation_close(oldIndexRel, NoLock);
> +	relation_close(newIndexRel, NoLock);

It might be worthwile adding a heap_freetuple here for (old,
new)IndexTuple, just to spare the reader the thinking whether it needs
to be done.


> +/*
> + * index_concurrent_drop
> + *
> + * Drop a single index concurrently as the last step of an index concurrent
> + * process Deletion is done through performDeletion or dependencies of the
> + * index are not dropped. At this point all the indexes are already considered
> + * as invalid and dead so they can be dropped without using any concurrent
> + * options.
> + */

"or dependencies of the index would not get dropped"?

> +void
> +index_concurrent_drop(Oid indexOid)
> +{
> +	Oid				constraintOid = get_index_constraint(indexOid);
> +	ObjectAddress	object;
> +	Form_pg_index	indexForm;
> +	Relation		pg_index;
> +	HeapTuple		indexTuple;
> +	bool			indislive;
> +
> +	/*
> +	 * Check that the index dropped here is not alive, it might be used by
> +	 * other backends in this case.
> +	 */
> +	pg_index = heap_open(IndexRelationId, RowExclusiveLock);
> +
> +	indexTuple = SearchSysCacheCopy1(INDEXRELID,
> +									 ObjectIdGetDatum(indexOid));
> +	if (!HeapTupleIsValid(indexTuple))
> +		elog(ERROR, "cache lookup failed for index %u", indexOid);
> +	indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
> +	indislive = indexForm->indislive;
> +
> +	/* Clean up */
> +	heap_close(pg_index, RowExclusiveLock);
> +
> +	/* Leave if index is still alive */
> +	if (indislive)
> +		return;

This seems like a confusing path? Why is it valid to get here with a
valid index and why is it ok to silently ignore that case?


>  /*
> + * ReindexRelationConcurrently
> + *
> + * Process REINDEX CONCURRENTLY for given relation Oid. The relation can be
> + * either an index or a table. If a table is specified, each reindexing step
> + * is done in parallel with all the table's indexes as well as its dependent
> + * toast indexes.
> + */
> +bool
> +ReindexRelationConcurrently(Oid relationOid)
> +{
> +	List	   *concurrentIndexIds = NIL,
> +			   *indexIds = NIL,
> +			   *parentRelationIds = NIL,
> +			   *lockTags = NIL,
> +			   *relationLocks = NIL;
> +	ListCell   *lc, *lc2;
> +	Snapshot	snapshot;
> +
> +	/*
> +	 * Extract the list of indexes that are going to be rebuilt based on the
> +	 * list of relation Oids given by caller. For each element in given list,
> +	 * If the relkind of given relation Oid is a table, all its valid indexes
> +	 * will be rebuilt, including its associated toast table indexes. If
> +	 * relkind is an index, this index itself will be rebuilt. The locks taken
> +	 * parent relations and involved indexes are kept until this transaction
> +	 * is committed to protect against schema changes that might occur until
> +	 * the session lock is taken on each relation.
> +	 */
> +	switch (get_rel_relkind(relationOid))
> +	{
> +		case RELKIND_RELATION:
> +			{
> +				/*
> +				 * In the case of a relation, find all its indexes
> +				 * including toast indexes.
> +				 */
> +				Relation	heapRelation = heap_open(relationOid,
> +													ShareUpdateExclusiveLock);
> +
> +				/* Track this relation for session locks */
> +				parentRelationIds = lappend_oid(parentRelationIds, relationOid);
> +
> +				/* Relation on which is based index cannot be shared */
> +				if (heapRelation->rd_rel->relisshared)
> +					ereport(ERROR,
> +							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +							 errmsg("concurrent reindex is not supported for shared relations")));
> +
> +				/* Add all the valid indexes of relation to list */
> +				foreach(lc2, RelationGetIndexList(heapRelation))

Hm. This means we will not notice having about-to-be dropped indexes
around. Which seems safe because locks will prevent that anyway...

> +		default:
> +			/* nothing to do */
> +			break;

Shouldn't we error out?


> +	foreach(lc, indexIds)
> +	{
> +		Relation	indexRel;
> +		Oid			indOid = lfirst_oid(lc);
> +		Oid			concurrentOid = lfirst_oid(lc2);
> +		bool		primary;
> +
> +		/* Move to next concurrent item */
> +		lc2 = lnext(lc2);

forboth()

> +	/*
> +	 * Phase 3 of REINDEX CONCURRENTLY
> +	 *
> +	 * During this phase the concurrent indexes catch up with the INSERT that
> +	 * might have occurred in the parent table and are marked as valid once done.
> +	 *
> +	 * We once again wait until no transaction can have the table open with
> +	 * the index marked as read-only for updates. Each index validation is done
> +	 * with a separate transaction to avoid opening transaction for an
> +	 * unnecessary too long time.
> +	 */

Maybe I am being dumb because I have the feeling I said differently in
the past, but why do we not need a WaitForMultipleVirtualLocks() here?
The comment seems to say we need to do so.

> +	/*
> +	 * Perform a scan of each concurrent index with the heap, then insert
> +	 * any missing index entries.
> +	 */
> +	foreach(lc, concurrentIndexIds)
> +	{
> +		Oid indOid = lfirst_oid(lc);
> +		Oid relOid;
> +
> +		/* Open separate transaction to validate index */
> +		StartTransactionCommand();
> +
> +		/* Get the parent relation Oid */
> +		relOid = IndexGetRelation(indOid, false);
> +
> +		/*
> +		 * Take the reference snapshot that will be used for the concurrent indexes
> +		 * validation.
> +		 */
> +		snapshot = RegisterSnapshot(GetTransactionSnapshot());
> +		PushActiveSnapshot(snapshot);
> +
> +		/* Validate index, which might be a toast */
> +		validate_index(relOid, indOid, snapshot);
> +
> +		/*
> +		 * This concurrent index is now valid as they contain all the tuples
> +		 * necessary. However, it might not have taken into account deleted tuples
> +		 * before the reference snapshot was taken, so we need to wait for the
> +		 * transactions that might have older snapshots than ours.
> +		 */
> +		WaitForOldSnapshots(snapshot);
> +
> +		/*
> +		 * Concurrent index can now be marked as valid -- update pg_index
> +		 * entries.
> +		 */
> +		index_set_state_flags(indOid, INDEX_CREATE_SET_VALID);
> +
> +		/*
> +		 * The pg_index update will cause backends to update its entries for the
> +		 * concurrent index but it is necessary to do the same thing for cache.
> +		 */
> +		CacheInvalidateRelcacheByRelid(relOid);
> +
> +		/* we can now do away with our active snapshot */
> +		PopActiveSnapshot();
> +
> +		/* And we can remove the validating snapshot too */
> +		UnregisterSnapshot(snapshot);
> +
> +		/* Commit this transaction to make the concurrent index valid */
> +		CommitTransactionCommand();
> +	}


> +	/*
> +	 * Phase 5 of REINDEX CONCURRENTLY
> +	 *
> +	 * The concurrent indexes now hold the old relfilenode of the other indexes
> +	 *  transactions that might use them. Each operation is performed with a
> +	 * separate transaction.
> +	 */
> +
> +	/* Now mark the concurrent indexes as not ready */
> +	foreach(lc, concurrentIndexIds)
> +	{
> +		Oid			indOid = lfirst_oid(lc);
> +		Oid			relOid;
> +
> +		StartTransactionCommand();
> +		relOid = IndexGetRelation(indOid, false);
> +
> +		/*
> +		 * Finish the index invalidation and set it as dead. It is not
> +		 * necessary to wait for virtual locks on the parent relation as it
> +		 * is already sure that this session holds sufficient locks.s
> +		 */

tiny typo (lock.s)

> +	/*
> +	 * Phase 6 of REINDEX CONCURRENTLY
> +	 *
> +	 * Drop the concurrent indexes. This needs to be done through
> +	 * performDeletion or related dependencies will not be dropped for the old
> +	 * indexes. The internal mechanism of DROP INDEX CONCURRENTLY is not used
> +	 * as here the indexes are already considered as dead and invalid, so they
> +	 * will not be used by other backends.
> +	 */
> +	foreach(lc, concurrentIndexIds)
> +	{
> +		Oid indexOid = lfirst_oid(lc);
> +
> +		/* Start transaction to drop this index */
> +		StartTransactionCommand();
> +
> +		/* Get fresh snapshot for next step */
> +		PushActiveSnapshot(GetTransactionSnapshot());
> +
> +		/*
> +		 * Open transaction if necessary, for the first index treated its
> +		 * transaction has been already opened previously.
> +		 */
> +		index_concurrent_drop(indexOid);
> +
> +		/*
> +		 * For the last index to be treated, do not commit transaction yet.
> +		 * This will be done once all the locks on indexes and parent relations
> +		 * are released.
> +		 */

Hm. This doesn't seem to commit the last transaction at all right now?
Not sure why UnlockRelationIdForSession needs to be run in a transaction
anyway?


> +		if (indexOid != llast_oid(concurrentIndexIds))
> +		{
> +			/* We can do away with our snapshot */
> +			PopActiveSnapshot();
> +
> +			/* Commit this transaction to make the update visible. */
> +			CommitTransactionCommand();
> +		}
> +	}
> +
> +	/*
> +	 * Last thing to do is release the session-level lock on the parent table
> +	 * and the indexes of table.
> +	 */
> +	foreach(lc, relationLocks)
> +	{
> +		LockRelId lockRel = * (LockRelId *) lfirst(lc);
> +		UnlockRelationIdForSession(&lockRel, ShareUpdateExclusiveLock);
> +	}
> +
> +	return true;
> +}
> +
> +


> +	/*
> +	 * Check the case of a system index that might have been invalidated by a
> +	 * failed concurrent process and allow its drop.
> +	 */

This is only possible for toast indexes right now, right? If so, the
comment should mention that.

> +	if (IsSystemClass(classform) &&
> +		relkind == RELKIND_INDEX)
> +	{
> +		HeapTuple		locTuple;
> +		Form_pg_index	indexform;
> +		bool			indisvalid;
> +
> +		locTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(state->heapOid));
> +		if (!HeapTupleIsValid(locTuple))
> +		{
> +			ReleaseSysCache(tuple);
> +			return;
> +		}
> +
> +		indexform = (Form_pg_index) GETSTRUCT(locTuple);
> +		indisvalid = indexform->indisvalid;
> +		ReleaseSysCache(locTuple);
> +
> +		/* Leave if index entry is not valid */
> +		if (!indisvalid)
> +		{
> +			ReleaseSysCache(tuple);
> +			return;
> +		}
> +	}
> +

Ok, thats what I have for now...

Greetings,

Andres Freund

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services