Re: REINDEX CONCURRENTLY 2.0 - Mailing list pgsql-hackers

Hi,

I had a look at this.

On Mon, Mar 13, 2017 at 03:11:50AM +0100, Andreas Karlsson wrote:
> Spotted one of my TODO comments there so I have attached a patch where I
> have cleaned up that function. I also fixed the the code to properly support
> triggers.

The patch applies with quite a few offsets on top of current (2fd8685)
master, I have not verified that those are all ok.

Regression tests pass, also the included isolation tests.

I hope that Michael will post a full review as he worked on the code
extensively, but here are some some code comments, mostly on the
comments (note that I'm not a native speaker, so I might be wrong on
some of them as well):

> diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
> index 3908ade37b..3449c0af73 100644
> --- a/doc/src/sgml/ref/reindex.sgml
> +++ b/doc/src/sgml/ref/reindex.sgml
> @@ -68,9 +68,12 @@ REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } <replacea
>        An index build with the <literal>CONCURRENTLY</> option failed, leaving
>        an <quote>invalid</> index. Such indexes are useless but it can be
>        convenient to use <command>REINDEX</> to rebuild them. Note that
> -      <command>REINDEX</> will not perform a concurrent build. To build the
> -      index without interfering with production you should drop the index and
> -      reissue the <command>CREATE INDEX CONCURRENTLY</> command.
> +      <command>REINDEX</> will perform a concurrent build if <literal>
> +      CONCURRENTLY</> is specified. To build the index without interfering
> +      with production you should drop the index and reissue either the
> +      <command>CREATE INDEX CONCURRENTLY</> or <command>REINDEX CONCURRENTLY</>
> +      command. Indexes of toast relations can be rebuilt with <command>REINDEX
> +      CONCURRENTLY</>.

I think the "To build the index[...]" part should be rephrased, the
current diff makes it sound like you should drop the index first even if
you reindex concurrently. What about "Note that <command>REINDEX</> will
only perform a concurrent build if <literal> CONCURRENTLY</> is
specified"?

Anyway, this part is only about reindexing invalid indexes, so
mentioning that reindex is not concurrently or the part about create-
index-concurrently-then-rename only for this case is a bit weird, but
this is a pre-existing condition.

> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
> index 8d42a347ea..c40ac0b154 100644
> --- a/src/backend/catalog/index.c
> +++ b/src/backend/catalog/index.c
>  /*
> + * index_concurrent_create_copy
> + *
> + * Create a concurrent index based on the definition of the one provided by
> + * caller 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 reindex processing. The heap relation on which is based the index
> + * needs to be closed by the caller.
> + */

That should be "The heap relation on which the index is based ..." I
think.

> +    /*
> +     * We have to re-build the IndexInfo struct, since it was lost in
> +     * commit of transaction where this concurrent index was created
> +     * at the catalog level.
> +     */
> +    indexInfo = BuildIndexInfo(indexRelation);

Looks like indexInfo starts with lowercase, but the comment above has
upper case `IndexInfo'.

> +/*
> + * index_concurrent_swap
> + *
> + * Swap name, dependencies and constraints of the old index over to the new
> + * index, while marking the old index as invalid and the new as valid.
> + */

The `while' looks slightly odd to me, ISTM this is just another
operation this function performs, whereas "while" makes it sound like
the marking happens concurrently; so maybe ". Also, mark the old index
as invalid[...]"?

> +void
> +index_concurrent_swap(Oid newIndexOid, Oid oldIndexOid, const char *oldName)
> +{

[...]

> +    /*
> +     * Copy contraint flags for old index. This is safe because the old index
> +     * guaranteed uniquness.
> +     */

"uniqueness".

> +    /* Mark old index as valid and new is invalid as index_set_state_flags */

"new as invalid". Also, this comment style is different to this one:

> +    /*
> +     * Move contstraints and triggers over to the new index
> +     */

I guess the latter could be changed to a one-line comment as the former,
but maybe there is a deeper sense (locality of comment?) in this.

> +            /* make a modifiable copy */

I think comments should start capitalized?


> +/*
> + * 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 would not get dropped. At this point all the indexes are already
> + * considered as invalid and dead so they can be dropped without using any
> + * concurrent options as it is sure that they will not interact with other
> + * server sessions.
> + */

I'd write "as it is certain" instead of "as it is sure", but I can't
explain why. Maybe persons are sure, but situations are certain?

> @@ -1483,41 +1939,13 @@ index_drop(Oid indexId, bool concurrent)
>           * Note: the reason we use actual lock acquisition here, rather than
>           * just checking the ProcArray and sleeping, is that deadlock is
>           * possible if one of the transactions in question is blocked trying
> -         * to acquire an exclusive lock on our table.  The lock code will
> +         * to acquire an exclusive lock on our table. The lock code will

Gratuitous whitespace change, seeing that other comments added in this
patch have the extra whitespace after full stops as well.

> diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
> index d0ee851215..9dce6420df 100644
> --- a/src/backend/catalog/pg_depend.c
> +++ b/src/backend/catalog/pg_depend.c
> @@ -377,6 +377,94 @@ changeDependencyFor(Oid classId, Oid objectId,
>  }
>  
>  /*
> + * Adjust all dependency records to point to a different object of the same type
> + *
> + * refClassId/oldRefObjectId specify the old referenced object.
> + * newRefObjectId is the new referenced object (must be of class refClassId).
> + *
> + * Returns the number of records updated.
> + */
> +long
> +changeDependencyForAll(Oid refClassId, Oid oldRefObjectId,
> +                       Oid newRefObjectId)

This one is mostly a copy-paste of changeDependencyFor(), did you
consider refactoring that into handling the All case as well?

> @@ -722,3 +810,58 @@ get_index_constraint(Oid indexId)
>  
>      return constraintId;
>  }
> +
> +/*
> + * get_index_ref_constraints
> + *        Given the OID of an index, return the OID of all foreign key
> + *        constraints which reference the index.
> + */
> +List *
> +get_index_ref_constraints(Oid indexId)

Same for this one, but there's two similar functions
(get_constraint_index() and get_index_constraint()) already so I guess
it's fine?

> diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
> index 72bb06c760..7a51c25d98 100644
> --- a/src/backend/commands/indexcmds.c
> +++ b/src/backend/commands/indexcmds.c
> @@ -283,6 +285,87 @@ CheckIndexCompatible(Oid oldId,
>      return ret;
>  }
>  
> +
> +/*
> + * WaitForOlderSnapshots
> + *
> + * Wait for transactions that might have older snapshot than the given xmin

"an older snapshot" maybe?

> + * limit, because it might not contain tuples deleted just before it has
> + * been taken. Obtain a list of VXIDs of such transactions, and wait for them
> + * individually.
> + *
> + * We can exclude any running transactions that have xmin > the xmin given;
> + * their oldest snapshot must be newer than our xmin limit.
> + * We can also exclude any transactions that have xmin = zero, since they

Probably an empty line between the two paragraphs is in order, or just
keep it one paragraph.

> + * evidently have no live snapshot at all (and any one they might be in
> + * process of taking is certainly newer than ours).  

Please rephrase what's in the paranthesis, I am not quite sure what
it means, maybe "(and any they are currently taking is..."?

> + * We can also exclude autovacuum processes and processes running manual
> + * lazy VACUUMs, because they won't be fazed by missing index entries
> + * either. (Manual ANALYZEs, however, can't be excluded because they
> + * might be within transactions that are going to do arbitrary operations
> + * later.)

Weird punctuation, I think the full stop before the paranthesis should
be put after it, the full stop at the end of the paranthesis be dropped
and the beginning of the parenthesis should not be capitalized.

Oh hrm, I now see that the patch just moves the comments, so maybe don't
bother.

> @@ -1739,7 +1735,7 @@ ChooseIndexColumnNames(List *indexElems)
>   *        Recreate a specific index.
>   */
>  Oid
> -ReindexIndex(RangeVar *indexRelation, int options)
> +ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
>  {
>      Oid            indOid;
>      Oid            heapOid = InvalidOid;
> @@ -1751,8 +1747,9 @@ ReindexIndex(RangeVar *indexRelation, int options)
>       * obtain lock on table first, to avoid deadlock hazard.  The lock level
>       * used here must match the index lock obtained in reindex_index().
>       */
> -    indOid = RangeVarGetRelidExtended(indexRelation, AccessExclusiveLock,
> -                                      false, false,
> +    indOid = RangeVarGetRelidExtended(indexRelation,
> +                                      concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock,
> +                                      concurrent, concurrent,

I find the way the bool is passed for the third and fourth argument a
bit weird, but ok.  I would still suggest to explain in the comment
above why the two other arguments to RangeVarGetRelidExtended()
(`missing_ok' and `nowait') are dependent on concurrent reindexing; it's
not super-obvious to me.

>  
>      /* The lock level used here should match reindex_relation(). */
> -    heapOid = RangeVarGetRelidExtended(relation, ShareLock, false, false,
> +    heapOid = RangeVarGetRelidExtended(relation,
> +                                       concurrent ? ShareUpdateExclusiveLock : ShareLock,
> +                                       concurrent, concurrent,
>                                         RangeVarCallbackOwnsTable, NULL);

Same here.

> +    if (concurrent)
> +        result = ReindexRelationConcurrently(heapOid, options);
> +    else
> +        result = reindex_relation(heapOid,
> +                                  REINDEX_REL_PROCESS_TOAST |
> +                                  REINDEX_REL_CHECK_CONSTRAINTS,
> +                                  options);

This somewhat begs the question why those two cases are so different
(one is implemented in src/backend/catalog/index.c, the other in
src/backend/commands/indexcmds.c, and their naming scheme is different).
I guess that's ok, but it might also be a hint that
ReindexRelationConcurrently() is implemented at the wrong level.

> @@ -2011,3 +2040,597 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
>  
>      MemoryContextDelete(private_context);
>  }
> +
> +
> +/*
> + * ReindexRelationConcurrently
> + *
> + * Process REINDEX CONCURRENTLY for given relation Oid. The relation can be
> + * either an index or a table. If a table is specified, each phase is processed
> + * one by done for each table's indexes as well as its dependent toast indexes

"one by one".

> +static bool
> +ReindexRelationConcurrently(Oid relationOid, int options)
> +{

[...]

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

capitalization, "If" is in the middle of a sentence.

> +     * will be rebuilt, including its associated toast table indexes. If
> +     * relkind is an index, this index itself will be rebuilt. 

Maybe mention here that system catalogs and shared relations cannot be
reindexed concurrently?

> +        /* Create concurrent index based on given index */
> +        concurrentOid = index_concurrent_create_copy(indexParentRel,
> +                                                     indOid,
> +                                                     concurrentName);

AIUI, this creates/copies some meta-data for the concurrent index, but
does not yet create the index itself, right?  If so, the comment is
somewhat misleading.

> +        /*
> +         * Now open the relation of concurrent index, a lock is also needed on
> +         * it
> +         */

Multi-line comments should end with a full-stop I think?

> +    /*
> +     * Phase 3 of REINDEX CONCURRENTLY

[...]

> +        /*
> +         * This concurrent index is now valid as they contain all the tuples
> +         * necessary. However, it might not have taken into account deleted tuples

"as they contain" should be "as it contains" I guess, since the rest of
the comment is talking about a singular index.

> +    /*
> +     * Phase 4 of REINDEX CONCURRENTLY
> +     *
> +     * Now that the concurrent indexes have been validated, it is necessary
> +     * to swap each concurrent index with its corresponding old index.
> +     *
> +     * We mark the new indexes as valid and the old indexes dead at the same
> +     * time to make sure we get only get constraint violations from the

"we only get"

> +    /*
> +     * Phase 5 of REINDEX CONCURRENTLY
> +     *
> +     * The indexes hold now a fresh relfilenode of their respective concurrent

I'd write "now hold" instead of "hold now".

> +     * entries indexes. It is time to mark the now-useless concurrent entries
> +     * as not ready so as they can be safely discarded from write operations
> +     * that may occur on them.

So the "concurrent entries" is the original index, as that one should be
now-useless?  If so, that's a bit confusing terminology to me and it was
called "old index" in the previous phases.

> +    /*
> +     * Phase 6 of REINDEX CONCURRENTLY
> +     *
> +     * Drop the concurrent indexes, with actually the same code path as

Again, I'd have written "Drop the old indexes". Also, "with actually the
same" sounds a bit awkward, maybe "actually using the same" would be
better.

> +    /*
> +     * Last thing to do is to release the session-level lock on the parent table
> +     * and the indexes of table.

"and on the indexes of the table"?  Or what exactly is meant with the
last bit?

> diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
> index 20b5273405..c76dacc44a 100644
> --- a/src/backend/tcop/utility.c
> +++ b/src/backend/tcop/utility.c
> @@ -773,16 +773,20 @@ standard_ProcessUtility(PlannedStmt *pstmt,
> -                        ReindexIndex(stmt->relation, stmt->options);
> +                        ReindexIndex(stmt->relation, stmt->options, stmt->concurrent);
> -                        ReindexTable(stmt->relation, stmt->options);
> +                        ReindexTable(stmt->relation, stmt->options, stmt->concurrent);
> -                        ReindexMultipleTables(stmt->name, stmt->kind, stmt->options);
> +                        ReindexMultipleTables(stmt->name, stmt->kind, stmt->options, stmt->concurrent);

Those lines are now in excess of 80 chars.


Cheers,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer