Re: Support for REINDEX CONCURRENTLY - Mailing list pgsql-hackers

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:
> +     ListCell   *lc;
> +     int                     count = 0;

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

> +     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.

> +             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...

> +     /*
> +      * 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?

> +      * 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?

> +             /* 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 (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.

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

Think you forgot a digit here ;)

/*
 * 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.

> +             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?

> +     /* 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
> + */

"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?

> +             case RELKIND_RELATION:
> +                     {
> +                             /*
> +                              * In the case of a relation, find all its indexes
> +                              * including toast indexes.
> +                              */
> +                             Relation        heapRelation = heap_open(relationOid,
> +                                                                                                     ShareUpdateExclusiveLock);

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.

> +             /*
> +              * 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?

> +     /*
> +      * 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.