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
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 4448b986b6..e1bd81a1d6 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -3057,11 +3057,16 @@ ReindexRelationConcurrently(Oid relationOid, int options) Oid newIndexId = lfirst_oid(lc2); Oid heapId; - CHECK_FOR_INTERRUPTS(); - /* Start new transaction for this index's concurrent build */ StartTransactionCommand(); + /* + * Check for user-requested abort. This is inside a transaction + * so as xact.c does not issue a useless WARNING, and ensures that + * session-level locks are cleaned up on abort. + */ + CHECK_FOR_INTERRUPTS(); + /* Set ActiveSnapshot since functions in the indexes may need it */ PushActiveSnapshot(GetTransactionSnapshot()); @@ -3101,10 +3106,15 @@ ReindexRelationConcurrently(Oid relationOid, int options) TransactionId limitXmin; Snapshot snapshot; - CHECK_FOR_INTERRUPTS(); - StartTransactionCommand(); + /* + * Check for user-requested abort. This is inside a transaction + * so as xact.c does not issue a useless WARNING, and ensures that + * session-level locks are cleaned up on abort. + */ + CHECK_FOR_INTERRUPTS(); + heapId = IndexGetRelation(newIndexId, false); /* @@ -3166,6 +3176,11 @@ ReindexRelationConcurrently(Oid relationOid, int options) Oid newIndexId = lfirst_oid(lc2); Oid heapId; + /* + * Check for user-requested abort. This is inside a transaction + * so as xact.c does not issue a useless WARNING, and ensures that + * session-level locks are cleaned up on abort. + */ CHECK_FOR_INTERRUPTS(); heapId = IndexGetRelation(oldIndexId, false); @@ -3221,7 +3236,13 @@ ReindexRelationConcurrently(Oid relationOid, int options) Oid oldIndexId = lfirst_oid(lc); Oid heapId; + /* + * Check for user-requested abort. This is inside a transaction + * so as xact.c does not issue a useless WARNING, and ensures that + * session-level locks are cleaned up on abort. + */ CHECK_FOR_INTERRUPTS(); + heapId = IndexGetRelation(oldIndexId, false); index_concurrently_set_dead(heapId, oldIndexId); }
signature.asc
Description: PGP signature
