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