Re: REINDEX CONCURRENTLY 2.0 - Mailing list pgsql-hackers

Thanks for your input, Jim!

On Wed, Oct 29, 2014 at 7:59 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
> Patch applies against current HEAD and builds, but I'm getting 37 failed
> tests (mostly parallel, but also misc and WITH; results attached). Is that
> expected?
This is caused by the recent commit 7b1c2a0 (that I actually participated in reviewing :p) because of a missing inclusion of ruleutils.h in index.c.

> The "mark the concurrent index" bit is rather confusing; it sounds like it's
> referring to the new index instead of the old. Now that I've read the code I
> understand what's going on here between the concurrent index *entry* and the
> filenode swap, but I don't think the docs make this sufficiently clear to
> users.
>
> How about something like this:
>
> The following steps occur in a concurrent index build, each in a separate
> transaction. Note that if there are multiple indexes to be rebuilt then each
> step loops through all the indexes we're rebuilding, using a separate
> transaction for each one.
>

> 1. [blah]

Definitely a good idea! I took your text and made it more precise, listing the actions done for each step, the pg_index flags switched, using <orderedlist> to make the list of steps described in a separate paragraph more exhaustive for the user. At the same time I reworked the docs removing a part that was somewhat duplicated about dealing with the constraints having invalid index entries and how to drop them.

> + * 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.
>
> Last bit presumably should be "on which the index is based".
What about "Create a concurrent index based on the definition of the one provided by caller"?

> +       /* Build the list of column names, necessary for index_create */
> Instead of all this work wouldn't it be easier to create a version of
> index_create/ConstructTupleDescriptor that will use the IndexInfo for the
> old index? ISTM index_concurrent_create() is doing a heck of a lot of work
> to marshal data into one form just to have it get marshaled yet again. Worst
> case, if we do have to play this game, there should be a stand-alone
> function to get the columns/expressions for an existing index; you're
> duplicating a lot of code from pg_get_indexdef_worker().

Yes, this definitely sucks and the approach creating a function to get all the column names is not productive as well. Then let's define an additional argument in index_create to pass a potential TupleDesc instead of this whole wart. I noticed as well that we need to actually reset attcacheoff to be able to use a fresh version of the tuple descriptor of the old index. I added a small API for this purpose in tupdesc.h called ResetTupleDescCache. Would it make sense instead to extend CreateTupleDescCopyConstr or CreateTupleDescCopy with a boolean flag?

> index_concurrent_swap(): Perhaps it'd be better to create
> index_concurrent_swap_setup() and index_concurrent_swap_cleanup() and
> refactor the duplicated code out... the actual function would then become:

> ReindexRelationConcurrently()
>
> + * Process REINDEX CONCURRENTLY for given relation Oid. The relation can be
> + * either an index or a table. If a table is specified, each step of
> REINDEX
> + * CONCURRENTLY is done in parallel with all the table's indexes as well as
> + * its dependent toast indexes.
> This comment is a bit misleading; we're not actually doing anything in
> parallel, right? AFAICT index_concurrent_build is going to block while each
> index is built the first time.
Yes, parallel may be misleading. What is meant here is that each step of the process is done one by one on all the valid indexes a table may have.

> +        * 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.
>
> This comment is a bit unclear to me... at minimum I think it should be "* on
> parent relations" instead of "* parent relations", but I think it needs to
> elaborate on why/when we're also taking session level locks.
Hum, done as follows:
@@ -896,9 +896,11 @@ ReindexRelationConcurrently(Oid relationOid)
         * 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
+        * on 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.
+        * the session lock is taken on each relation, session lock used to
+        * similarly protect from any schema change that could happen within the
+        * multiple transactions that are used during this process.
         */

> I also wordsmithed this comment a bit...
>          * Here begins the process for concurrently rebuilding the index
> and this one...
>          * During this phase the concurrent indexes catch up with any new
Slight differences indeed. Thanks and included.

> I'd change that to "explosion in the number of indexes a parent relation
> could have if this operation fails."
Well, implosion was more... I don't recall my state of mind when writing that. So changed the way you recommend.

> Phase 4, 5 and 6 are rather confusing if you don't understand that each
> "concurrent index" entry is meant to be thrown away. I think the Phase 4
> comment should elaborate on that.
OK, done.

> The comment in check_exclusion_constraint() is good; shouldn't the related
> comment on this line in index_create() mention that
> check_exclusion_constraint() needs to be changed if we ever support
> concurrent builds of exclusion indexes?
>
>         if (concurrent && is_exclusion && !is_reindex)
OK, what about that then:
        /*
-        * This case is currently not supported, but there's no way to ask for it
-        * in the grammar anyway, so it can't happen.
+        * 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. If support for exclusion constraints is added in the future,
+        * the check similar to this one in check_exclusion_constraint should as
+        * well be changed accordingly.

Updated patch is attached.

Thanks again.

Regards,
--
Michael