pgstat_heap() consults freed memory

Started by Noah Mischover 11 years ago1 messages
#1Noah Misch
noah@leadboat.com
1 attachment(s)

pgstat_heap() creates a BufferAccessStrategy and attaches it to a
HeapScanDesc. It continues to use that strategy after calling heap_endscan(),
which frees the strategy. This is only a risk when the table contains empty
pages at the end. I get a crash in an assert-enabled build with this test
procedure, after disabling autovacuum:

-- session 1
create table t (c) as select * from generate_series(1,20000);
delete from t where c > 10000;
-- session 2
begin; lock table t in access share mode;
-- session 1
vacuum t;
-- restart PostgreSQL to clear shared buffers
-- session 3
select * from pgstattuple('t');

The simplest fix is to move the heap_endscan() call past the last use of the
strategy. However, I don't think this function ought to be creating a
strategy explicitly. It should use the one that initscan() creates, if any.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

Attachments:

pgstat_heap-freed-memory-v1.patchtext/plain; charset=us-asciiDownload
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index edc603f..1007748 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -274,7 +274,6 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo)
 	BlockNumber tupblock;
 	Buffer		buffer;
 	pgstattuple_type stat = {0};
-	BufferAccessStrategy bstrategy;
 	SnapshotData SnapshotDirty;
 
 	/* Disable syncscan because we assume we scan from block zero upwards */
@@ -283,10 +282,6 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo)
 
 	nblocks = scan->rs_nblocks; /* # blocks to be scanned */
 
-	/* prepare access strategy for this table */
-	bstrategy = GetAccessStrategy(BAS_BULKREAD);
-	scan->rs_strategy = bstrategy;
-
 	/* scan the relation */
 	while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
 	{
@@ -320,26 +315,28 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo)
 		{
 			CHECK_FOR_INTERRUPTS();
 
-			buffer = ReadBufferExtended(rel, MAIN_FORKNUM, block, RBM_NORMAL, bstrategy);
+			buffer = ReadBufferExtended(rel, MAIN_FORKNUM, block,
+										RBM_NORMAL, scan->rs_strategy);
 			LockBuffer(buffer, BUFFER_LOCK_SHARE);
 			stat.free_space += PageGetHeapFreeSpace((Page) BufferGetPage(buffer));
 			UnlockReleaseBuffer(buffer);
 			block++;
 		}
 	}
-	heap_endscan(scan);
 
 	while (block < nblocks)
 	{
 		CHECK_FOR_INTERRUPTS();
 
-		buffer = ReadBufferExtended(rel, MAIN_FORKNUM, block, RBM_NORMAL, bstrategy);
+		buffer = ReadBufferExtended(rel, MAIN_FORKNUM, block,
+									RBM_NORMAL, scan->rs_strategy);
 		LockBuffer(buffer, BUFFER_LOCK_SHARE);
 		stat.free_space += PageGetHeapFreeSpace((Page) BufferGetPage(buffer));
 		UnlockReleaseBuffer(buffer);
 		block++;
 	}
 
+	heap_endscan(scan);
 	relation_close(rel, AccessShareLock);
 
 	stat.table_len = (uint64) nblocks *BLCKSZ;