Re: REINDEX CONCURRENTLY 2.0

On Thu, Mar 30, 2017 at 5:13 AM, Michael Banck
<michael(dot)banck(at)credativ(dot)de> wrote:
> 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.
>
> 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):

Thanks, Michael. I have done a pass on it

> [review comments]

Here are more comments:

+ <para>
+ <command>REINDEX SYSTEM</command> does not support
+ <command>CONCURRENTLY</command>.
+ </para>
It would be nice to mention that REINDEX SCHEMA pg_catalog is not
supported, or just tell that concurrent reindexing of any system
catalog indexes.

When running REINDEX SCHEMA CONCURRENTLY public on the regression
database I am bumping into a bunch of these warnings:
WARNING: 01000: snapshot 0x7fa5e6000040 still active
LOCATION: AtEOXact_Snapshot, snapmgr.c:1123
WARNING: 01000: snapshot 0x7fa5e6000040 still active
LOCATION: AtEOXact_Snapshot, snapmgr.c:1123

+ * Reset attcacheoff for a TupleDesc
+ */
+void
+ResetTupleDescCache(TupleDesc tupdesc)
+{
+ int i;
+
+ for (i = 0; i < tupdesc->natts; i++)
+ tupdesc->attrs[i]->attcacheoff = -1;
+}
I think that it would be better to merge that with TupleDescInitEntry
to be sure that the initialization of a TupleDesc's attribute goes
through only one code path.

+ /*
+ * Copy contraint flags for old index. This is safe because the old index
+ * guaranteed uniquness.
+ */
s/uniquness/uniqueness/ and s/contraint/constraint/.

+ /*
+ * Move contstraints and triggers over to the new index
+ */
s/contstraints/constraints/.

-REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM
} <replaceable class="PARAMETER">name</replaceable>
+REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM
} [ CONCURRENTLY ] <replaceable class="PARAMETER">name</replaceable>
I am taking the war path with such a sentence... But what about adding
CONCURRENTLY to the list of options in parenthesis instead?

With this patch, we are reaching the 9th boolean argument for
create_index(). Any opinions about refactoring that into a set of
bitwise flags? Fairly unrelated to this patch.

+ /*
+ * Move all dependencies on the old index to the new
+ */
Sentence unfinished.

It has been years since I looked at this code (I wrote it in
majority), but here is what I would explore if I were to work on that
for the next release cycle:
- Explore the use of SQL-level interfaces to mark an index as inactive
at creation.
- Remove work done in changeDependencyForAll, and replace it by
something similar to what tablecmds.c does. There is I think here some
place for refactoring if that's not with CREATE TABLE LIKE. This
requires to the same work of creation, renaming and drop of the old
triggers and constraints.
- Do a per-index rebuild and not a per-relation rebuild for concurrent
indexing. Doing a per-relation reindex has the disadvantage that many
objects need to be created at the same time, and in the case of
REINDEX CONCURRENTLY time of the operation is not what matters, it is
how intrusive the operation is. Relations with many indexes would also
result in much object locks taken at each step.
The first and second points require a bit of thoughts for sure, but in
the long term that would pay in maintenance if we don't reinvent the
wheel, or at least try not to.
--
Michael