Re: REINDEX CONCURRENTLY 2.0

Thanks for your input, Jim!

On Wed, Oct 29, 2014 at 7:59 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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:
This sentence is not finished :) IMO, index_concurrent_swap looks good as
is, taking as arguments the index and its concurrent entry, and swapping
their relfilenode after taking AccessExclusiveLock that will be hold until
the end of this transaction.

> 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