postgres.git / summary / log / commit / refs

commit    24ace4053d42e2c48af8c15d598622e488fb9502
Author:   Tom Lane <tgl@sss.pgh.pa.us>
Date:     Sat Nov 02 20:45:42 2013 +0000
First released in: 9.4.0, 9.2.6, 9.3.2

    Retry after buffer locking failure during SPGiST index creation.
    
    The original coding thought this case was impossible, but it can happen
    if the bgwriter or checkpointer processes decide to write out an index
    page while creation is still proceeding, leading to a bogus "unexpected
    spgdoinsert() failure" error.  Problem reported by Jonathan S. Katz.
    
    Teodor Sigaev


 src/backend/access/spgist/spgdoinsert.c |  9 ++++++---
 src/backend/access/spgist/spginsert.c   | 15 +++++++++++----
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
index 1fd331cbdfd..9f425ca3558 100644
--- a/src/backend/access/spgist/spgdoinsert.c
+++ b/src/backend/access/spgist/spgdoinsert.c
@@ -1946,9 +1946,12 @@ spgdoinsert(Relation index, SpGistState *state,
 			 * Attempt to acquire lock on child page.  We must beware of
 			 * deadlock against another insertion process descending from that
 			 * page to our parent page (see README).  If we fail to get lock,
-			 * abandon the insertion and tell our caller to start over.  XXX
-			 * this could be improved; perhaps it'd be worth sleeping a bit
-			 * before giving up?
+			 * abandon the insertion and tell our caller to start over.
+			 *
+			 * XXX this could be improved, because failing to get lock on a
+			 * buffer is not proof of a deadlock situation; the lock might be
+			 * held by a reader, or even just background writer/checkpointer
+			 * process.  Perhaps it'd be worth retrying after sleeping a bit?
 			 */
 			if (!ConditionalLockBuffer(current.buffer))
 			{
diff --git a/src/backend/access/spgist/spginsert.c b/src/backend/access/spgist/spginsert.c
index f4d0fe5a0c2..2a50d87c74b 100644
--- a/src/backend/access/spgist/spginsert.c
+++ b/src/backend/access/spgist/spginsert.c
@@ -45,10 +45,17 @@ spgistBuildCallback(Relation index, HeapTuple htup, Datum *values,
 	/* Work in temp context, and reset it after each tuple */
 	oldCtx = MemoryContextSwitchTo(buildstate->tmpCtx);
 
-	/* No concurrent insertions can be happening, so failure is unexpected */
-	if (!spgdoinsert(index, &buildstate->spgstate, &htup->t_self,
-					 *values, *isnull))
-		elog(ERROR, "unexpected spgdoinsert() failure");
+	/*
+	 * Even though no concurrent insertions can be happening, we still might
+	 * get a buffer-locking failure due to bgwriter or checkpointer taking a
+	 * lock on some buffer.  So we need to be willing to retry.  We can flush
+	 * any temp data when retrying.
+	 */
+	while (!spgdoinsert(index, &buildstate->spgstate, &htup->t_self,
+						*values, *isnull))
+	{
+		MemoryContextReset(buildstate->tmpCtx);
+	}
 
 	MemoryContextSwitchTo(oldCtx);
 	MemoryContextReset(buildstate->tmpCtx);


[parent: bffd1ce92cb4]