Re: v12.0: reindex CONCURRENTLY: lock ShareUpdateExclusiveLock on object 14185/39327/0 is already held

On Wed, Oct 23, 2019 at 07:18:33PM +0900, Michael Paquier wrote:
> I can confirm that this is an issue related to session locks which are
> not cleaned up when in an out-of-transaction state, state that can be
> reached between a transaction commit or start while holding at least
> one session lock within one single command of VACUUM, CIC or REINDEX
> CONCURRENTLY.

Please let me back-pedal a bit on this one after sleeping on it.
Actually, if you look at CIC and VACUUM, those code paths are much
more careful regarding the position of CHECK_FOR_INTERRUPTS() than
REINDEX CONCURRENTLY is in the fact that they happen only within a
transaction context.  In the case of REINDEX CONCURRENTLY and the
failure reported here, the current code is careless: it depends of
course on the timing of statement_timeout, but session locks would
remain behind when hitting an interruption at the beginning of phase 2
or 3 in indexcmds.c.  So the answer is simple: by moving the interrupt
checks within a transaction context, the problem gets solved.  This
also fixes a second issue as the original code would cause xact.c to
generate some useless warnings.

Please see the attached.  Justin, does it fix your problems regarding
the locks?  For me it does.

> The failure is actually pretty easy to reproduce if you
> add an elog(ERROR) after a CommitTransactionCommand() call and then
> shut down the connection.  I am starting a new thread about that.  The
> problem is larger than it looks, and exists for a long time.

I am still wondering if we could put more safeguards in this area
though...
--
Michael