Re: v12.0: segfault in reindex CONCURRENTLY

On 2019-Oct-17, Michael Paquier wrote:

> On Thu, Oct 17, 2019 at 05:33:22AM -0300, Alvaro Herrera wrote:
> > Hmm, I wonder if it isn't the right solution to set 'progress' to false
> > in that spot, instead.  index_drop says it must only be called by the
> > dependency machinery; are we depending on that to pass-through the need
> > to update progress status?  I'm going over that code now.
> 
> pgstat_progress_end_command() is done for REINDEX CONCURRENTLY after
> the concurrent drop, so it made sense to me to still report any PID
> REINDEX CONC is waiting for at this stage.

Yeah, okay.  So let's talk about your proposed new comment.  First,
there are two spots where WaitForLockers is called in index_drop and
you're proposing to patch the second one.  I think we should patch the
first one and reference that one from the second one.  I propose
something like this (sorry for crude pasting):

     * 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
     * detect deadlock and error out properly.
     * 
     * Note: we report progress through WaitForLockers() unconditionally
     * here, even though it will only be used by REINDEX CONCURRENTLY and
     * not DROP INDEX CONCURRENTLY.
     */

and then

    /*
     * Wait till every transaction that saw the old index state has
-    * finished.
+    * finished.  See above about progress reporting.
     */

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services