Fix dynahash.c to suppress hash bucket splits while a hash_seq_search() scan

is in progress on the same hashtable.  This seems the least invasive way to
fix the recently-recognized problem that a split could cause the scan to
visit entries twice or (with much lower probability) miss them entirely.
The only field-reported problem caused by this is the "failed to re-find
shared lock object" PANIC in COMMIT PREPARED reported by Michel Dorochevsky,
which was caused by multiply visited entries.  However, it seems certain
that mdsync() is vulnerable to missing required fsync's due to missed
entries, and I am fearful that RelationCacheInitializePhase2() might be at
risk as well.  Because of that and the generalized hazard presented by this
bug, back-patch all the supported branches.

Along the way, fix pg_prepared_statement() and pg_cursor() to not assume
that the hashtables they are examining will stay static between calls.
This is risky regardless of the newly noted dynahash problem, because
hash_seq_search() has never promised to cope with deletion of table entries
other than the just-returned one.  There may be no bug here because the only
supported way to call these functions is via ExecMakeTableFunctionResult()
which will cycle them to completion before doing anything very interesting,
but it seems best to get rid of the assumption.  This affects 8.2 and HEAD
only, since those functions weren't there earlier.

Author: tglsfdc

src/backend/access/transam/xact.c

@@ -10,7 +10,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.239 2007/04/03 16:34:35 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.240 2007/04/26 23:24:44 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1631,6 +1631,7 @@ CommitTransaction(void)
 	/* smgrcommit already done */
 	AtEOXact_Files();
 	AtEOXact_ComboCid();
+	AtEOXact_HashTables(true);
 	pgstat_clear_snapshot();
 	pgstat_count_xact_commit();
 	pgstat_report_txn_timestamp(0);
@@ -1849,6 +1850,7 @@ PrepareTransaction(void)
 	/* smgrcommit already done */
 	AtEOXact_Files();
 	AtEOXact_ComboCid();
+	AtEOXact_HashTables(true);
 	pgstat_clear_snapshot();
 
 	CurrentResourceOwner = NULL;
@@ -2003,6 +2005,7 @@ AbortTransaction(void)
 	smgrabort();
 	AtEOXact_Files();
 	AtEOXact_ComboCid();
+	AtEOXact_HashTables(false);
 	pgstat_clear_snapshot();
 	pgstat_count_xact_rollback();
 	pgstat_report_txn_timestamp(0);
@@ -3716,6 +3719,7 @@ CommitSubTransaction(void)
 						  s->parent->subTransactionId);
 	AtEOSubXact_Files(true, s->subTransactionId,
 					  s->parent->subTransactionId);
+	AtEOSubXact_HashTables(true, s->nestingLevel);
 
 	/*
 	 * We need to restore the upper transaction's read-only state, in case the
@@ -3827,6 +3831,7 @@ AbortSubTransaction(void)
 							  s->parent->subTransactionId);
 		AtEOSubXact_Files(false, s->subTransactionId,
 						  s->parent->subTransactionId);
+		AtEOSubXact_HashTables(false, s->nestingLevel);
 	}
 
 	/*

src/backend/commands/prepare.c

@@ -10,7 +10,7 @@
  * Copyright (c) 2002-2007, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/prepare.c,v 1.73 2007/04/16 18:21:07 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/prepare.c,v 1.74 2007/04/26 23:24:44 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -21,7 +21,7 @@
 #include "catalog/pg_type.h"
 #include "commands/explain.h"
 #include "commands/prepare.h"
-#include "funcapi.h"
+#include "miscadmin.h"
 #include "parser/analyze.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_expr.h"
@@ -743,92 +743,99 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, ExplainStmt *stmt,
 Datum
 pg_prepared_statement(PG_FUNCTION_ARGS)
 {
-	FuncCallContext *funcctx;
-	HASH_SEQ_STATUS *hash_seq;
-	PreparedStatement *prep_stmt;
-
-	/* stuff done only on the first call of the function */
-	if (SRF_IS_FIRSTCALL())
-	{
-		TupleDesc	tupdesc;
-		MemoryContext oldcontext;
-
-		/* create a function context for cross-call persistence */
-		funcctx = SRF_FIRSTCALL_INIT();
+	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+	TupleDesc	tupdesc;
+	Tuplestorestate *tupstore;
+	MemoryContext per_query_ctx;
+	MemoryContext oldcontext;
+
+	/* check to see if caller supports us returning a tuplestore */
+	if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("set-valued function called in context that cannot accept a set")));
+	if (!(rsinfo->allowedModes & SFRM_Materialize))
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("materialize mode required, but it is not " \
+						"allowed in this context")));
 
-		/*
-		 * switch to memory context appropriate for multiple function calls
-		 */
-		oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+	/* need to build tuplestore in query context */
+	per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
+	oldcontext = MemoryContextSwitchTo(per_query_ctx);
 
-		/* allocate memory for user context */
-		if (prepared_queries)
-		{
-			hash_seq = (HASH_SEQ_STATUS *) palloc(sizeof(HASH_SEQ_STATUS));
-			hash_seq_init(hash_seq, prepared_queries);
-			funcctx->user_fctx = (void *) hash_seq;
-		}
-		else
-			funcctx->user_fctx = NULL;
+	/*
+	 * build tupdesc for result tuples. This must match the definition of
+	 * the pg_prepared_statements view in system_views.sql
+	 */
+	tupdesc = CreateTemplateTupleDesc(5, false);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
+					   TEXTOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
+					   TEXTOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 3, "prepare_time",
+					   TIMESTAMPTZOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 4, "parameter_types",
+					   REGTYPEARRAYOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
+					   BOOLOID, -1, 0);
 
-		/*
-		 * build tupdesc for result tuples. This must match the definition of
-		 * the pg_prepared_statements view in system_views.sql
-		 */
-		tupdesc = CreateTemplateTupleDesc(5, false);
-		TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
-						   TEXTOID, -1, 0);
-		TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
-						   TEXTOID, -1, 0);
-		TupleDescInitEntry(tupdesc, (AttrNumber) 3, "prepare_time",
-						   TIMESTAMPTZOID, -1, 0);
-		TupleDescInitEntry(tupdesc, (AttrNumber) 4, "parameter_types",
-						   REGTYPEARRAYOID, -1, 0);
-		TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
-						   BOOLOID, -1, 0);
-
-		funcctx->tuple_desc = BlessTupleDesc(tupdesc);
-		MemoryContextSwitchTo(oldcontext);
-	}
+	/*
+	 * We put all the tuples into a tuplestore in one scan of the hashtable.
+	 * This avoids any issue of the hashtable possibly changing between calls.
+	 */
+	tupstore = tuplestore_begin_heap(true, false, work_mem);
 
-	/* stuff done on every call of the function */
-	funcctx = SRF_PERCALL_SETUP();
-	hash_seq = (HASH_SEQ_STATUS *) funcctx->user_fctx;
+	/* hash table might be uninitialized */
+	if (prepared_queries)
+	{
+		HASH_SEQ_STATUS hash_seq;
+		PreparedStatement *prep_stmt;
 
-	/* if the hash table is uninitialized, we're done */
-	if (hash_seq == NULL)
-		SRF_RETURN_DONE(funcctx);
+		hash_seq_init(&hash_seq, prepared_queries);
+		while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL)
+		{
+			HeapTuple	tuple;
+			Datum		values[5];
+			bool		nulls[5];
 
-	prep_stmt = hash_seq_search(hash_seq);
-	if (prep_stmt)
-	{
-		Datum		result;
-		HeapTuple	tuple;
-		Datum		values[5];
-		bool		nulls[5];
+			/* generate junk in short-term context */
+			MemoryContextSwitchTo(oldcontext);
 
-		MemSet(nulls, 0, sizeof(nulls));
+			MemSet(nulls, 0, sizeof(nulls));
 
-		values[0] = DirectFunctionCall1(textin,
+			values[0] = DirectFunctionCall1(textin,
 									  CStringGetDatum(prep_stmt->stmt_name));
 
-		if (prep_stmt->plansource->query_string == NULL)
-			nulls[1] = true;
-		else
-			values[1] = DirectFunctionCall1(textin,
+			if (prep_stmt->plansource->query_string == NULL)
+				nulls[1] = true;
+			else
+				values[1] = DirectFunctionCall1(textin,
 						CStringGetDatum(prep_stmt->plansource->query_string));
 
-		values[2] = TimestampTzGetDatum(prep_stmt->prepare_time);
-		values[3] = build_regtype_array(prep_stmt->plansource->param_types,
-										prep_stmt->plansource->num_params);
-		values[4] = BoolGetDatum(prep_stmt->from_sql);
+			values[2] = TimestampTzGetDatum(prep_stmt->prepare_time);
+			values[3] = build_regtype_array(prep_stmt->plansource->param_types,
+											prep_stmt->plansource->num_params);
+			values[4] = BoolGetDatum(prep_stmt->from_sql);
+
+			tuple = heap_form_tuple(tupdesc, values, nulls);
 
-		tuple = heap_form_tuple(funcctx->tuple_desc, values, nulls);
-		result = HeapTupleGetDatum(tuple);
-		SRF_RETURN_NEXT(funcctx, result);
+			/* switch to appropriate context while storing the tuple */
+			MemoryContextSwitchTo(per_query_ctx);
+			tuplestore_puttuple(tupstore, tuple);
+		}
 	}
 
-	SRF_RETURN_DONE(funcctx);
+	/* clean up and return the tuplestore */
+	tuplestore_donestoring(tupstore);
+
+	MemoryContextSwitchTo(oldcontext);
+
+	rsinfo->returnMode = SFRM_Materialize;
+	rsinfo->setResult = tupstore;
+	rsinfo->setDesc = tupdesc;
+
+	return (Datum) 0;
 }
 
 /*

src/backend/executor/nodeSubplan.c

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/executor/nodeSubplan.c,v 1.87 2007/02/27 01:11:25 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/executor/nodeSubplan.c,v 1.88 2007/04/26 23:24:44 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -569,16 +569,20 @@ findPartialMatch(TupleHashTable hashtable, TupleTableSlot *slot)
 	TupleHashIterator hashiter;
 	TupleHashEntry entry;
 
-	ResetTupleHashIterator(hashtable, &hashiter);
+	InitTupleHashIterator(hashtable, &hashiter);
 	while ((entry = ScanTupleHashTable(&hashiter)) != NULL)
 	{
 		ExecStoreMinimalTuple(entry->firstTuple, hashtable->tableslot, false);
 		if (!execTuplesUnequal(slot, hashtable->tableslot,
 							   numCols, keyColIdx,
 							   hashtable->cur_eq_funcs,
 							   hashtable->tempcxt))
+		{
+			TermTupleHashIterator(&hashiter);
 			return true;
+		}
 	}
+	/* No TermTupleHashIterator call needed here */
 	return false;
 }
 

src/backend/nodes/tidbitmap.c

@@ -23,7 +23,7 @@
  * Copyright (c) 2003-2007, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/nodes/tidbitmap.c,v 1.11 2007/01/05 22:19:30 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/nodes/tidbitmap.c,v 1.12 2007/04/26 23:24:44 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -907,7 +907,11 @@ tbm_lossify(TIDBitmap *tbm)
 		tbm_mark_page_lossy(tbm, page->blockno);
 
 		if (tbm->nentries <= tbm->maxentries)
-			return;				/* we have done enough */
+		{
+			/* we have done enough */
+			hash_seq_term(&status);
+			break;
+		}
 
 		/*
 		 * Note: tbm_mark_page_lossy may have inserted a lossy chunk into the