Subject: Improve warnings around CREATE INDEX CONCURRENTLY

From: Greg Smith <[email-masked]>
Date: Tue, 24 May 2011 03:56:59 -0400

When running CREATE INDEX CONCURRENTLY, the DefineIndex() code in 
src/backend/commands/indexcmds.c does a few things that one would expect 
from the documentation.  And then at the end it executes code described 
like this:

"The index is now valid in the sense that it contains all currently 
interesting tuples.  But since it might not contain tuples deleted just 
before the reference snap was taken, we have to wait out any 
transactions that might have older snapshots.  Obtain a list of VXIDs of 
such transactions, and wait for them individually."

It's possible to end up with a long series in pg_locks waiting for 
virtualxid entries at this point, for as long as some set of giant 
queries takes to execute, and you'll only see them one at a time.  The 
documentation warns:

"PostgreSQL must perform two scans of the table, and in addition it must 
wait for all existing transactions that could potentially use the index 
to terminate."

That's correct, but easy to read the wrong way.  I always assumed that 
this meant it was going to wait behind anything that had a shared lock 
or such on the table, things that had already accessed it.  This is the 
case with some earlier parts of this same code path.  But when it comes 
to the end here, the scope is actually broader than that.  And since 
there's a session-level lock on the table the whole time this wait loop 
is executing, that makes considerable secondary havoc possible here.  
You can end up waiting an unbounded amount of time for some long-running 
transaction, one not even expected to enter into the rebuild work, to 
finish, which leaves you no idea what's happening unless you know just 
what to look for.  (Watching such havoc actually happen is what prompted 
this investigation)

What makes it worse is that the wait shows up as a virtualxid one, which 
doesn't pop up on many common samples of things to look for in 
pg_locks.  It would be reasonable but also incorrect for admins to 
assume a table one would be visible if running into the case alluded to 
in the docs.  The serial way locks are obtained is unexpected too.

Attached patch expands the warnings around this command to reflect both 
issues:

-The waiting time is not necessarily limited to just things that involve 
the table
-The locks it obtains while running this phase of the rebuild are unusual

-- 
Greg Smith   2ndQuadrant US    [email-masked]   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us

diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 43b6499..cb3334b 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -381,7 +381,17 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ <replaceable class="parameter">name</
    <para>
     In a concurrent index build, the index is actually entered into the
     system catalogs in one transaction, then the two table scans occur in a
-    second and third transaction.
+    second and third transaction.  All active transactions at the time the
+    second table scan starts, not just ones that already involve the table,
+    have the potential to block the concurrent index creation for however
+    long it takes for them to finish.  When checking for transactions that
+    could still use the original index, concurrent index creation advances
+    through potentially interfering older transactions one at a time,
+    obtaining locks on their virtual transaction identifiers to wait for
+    them to complete.
+   </para>
+
+   <para>
     If a problem arises while scanning the table, such as a
     uniqueness violation in a unique index, the <command>CREATE INDEX</>
     command will fail but leave behind an <quote>invalid</> index. This index

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 24 May 2011 16:48:07 -0400

> in the docs.  The serial way locks are obtained is unexpected too.

Incidentally, this is one of the things that Jim Nasby wanted exposed
somehow so that these problems are more easily diagnosed.  I dropped the
ball on that one, but I'll be picking it up again at some point.

-- 
Álvaro Herrera <[email-masked]>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

---

From: Greg Smith <[email-masked]>
Date: Tue, 24 May 2011 17:06:07 -0400

> ball on that one, but I'll be picking it up again at some point.

I don't remember seeing anything about that before, but then again I 
wasn't really looking for it until now.  Did you have a basic idea what 
you wanted to do there?  I have enough of this work queued up now that I 
may end up poking at the code myself here soon.

The first thing I was considering was dropping some DEBUG2 level logging 
about the various phases of the reindex into there.  Given that 
concurrent index creation has all these single instance requirements and 
long runtimes, I find myself putting them into scripts that do all the 
work as background processes.  If I could set client_min_messages=debug2 
when starting the script, and see more info about the progress as it 
happens appear in the script's output log, that is something I think 
many admins would choose to do.

Not the ideal UI for exposing this info, certainly.  But a really easy 
one to add, and realistically I think it would be enough to resolve most 
of the transparency complaints here.  The biggest problem is not even 
documenting where people should be looking toward suspiciously, which I 
think the doc patch I submitted helps with.

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 25 May 2011 13:57:19 -0400

> may end up poking at the code myself here soon.

Please see this thread:
http://archives.postgresql.org/message-id/[email-masked]

> about the various phases of the reindex into there.

That would take this system more or less to where autovacuum was in 8.1
(well, not quite that bad because you can change it locally, but still).
It seems to me we can do something a bit better than that.

---

From: Simon Riggs <[email-masked]>
Date: Wed, 25 May 2011 19:24:09 +0100

> It seems to me we can do something a bit better than that.

It would be fairly straightforward to add some text to the end of the
ps message for long running DDL commands to say "phase 2" etc..
Or pg_stat_activity query text.

That way we would have some visibility into the progress of long
running DDL without too much effort.

-- 
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

---

From: Greg Smith <[email-masked]>
Date: Wed, 25 May 2011 17:04:03 -0400

> http://archives.postgresql.org/message-id/[email-masked]

Ah, now I see--I was running a CommitFest that week, so of course I 
missed this message.  Those are all good ideas.

> It seems to me we can do something a bit better than that.

Sure.  Simon's command string idea might work better, and doing some 
extra lock decoration as you suggested in the above thread would be 
another level of improvement.  We should pick up redesign later on the 
main list.  You can at least count me in as someone who wants to see 
this improved now.

Back to the doc patch I submitted...is that a useful step toward making 
this issue visible enough to users for now to help?

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 26 May 2011 11:52:27 -0400

> main list.  You can at least count me in as someone who wants to see 

Great

> this issue visible enough to users for now to help?

Sure, why not?  I thought I could choose my bikeshed color while I was
here, how about

+    second and third transaction.  All active transactions at the time the
+    second table scan starts, not just ones that already involve the table,
+    have the potential to block the concurrent index creation until they
+    finish.  When checking for transactions that
+    could still use the original index, concurrent index creation advances
+    through potentially interfering older transactions one at a time,
+    obtaining shared locks on their virtual transaction identifiers to wait for
+    them to complete.

---

From: Greg Smith <[email-masked]>
Date: Fri, 27 May 2011 00:48:28 -0400

> Sure, why not?  I thought I could choose my bikeshed color while I was

That text is fine too.  I don't care exactly what color the bike shed 
is, I just want to get the bike out of the rain.

---

From: Robert Haas <[email-masked]>
Date: Mon, 13 Jun 2011 12:11:48 -0400

> +    obtaining shared locks on their virtual transaction identifiers to wait for

Alvaro, did you commit this?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 13 Jun 2011 17:26:39 -0400

> Alvaro, did you commit this?

I just did (9.0 and beyond), sorry for sitting on it for so long.