On 9/7/24 22:45, Tom Lane wrote:
I wrote:
Now alternatively you could argue that a "new" page isn't usable free
space yet and so we should count it as zero, just as we don't count
dead tuples as usable free space. You need VACUUM to turn either of
those things into real free space. But that'd be a bigger definitional
change, and I'm not sure we want it. Thoughts?
On the third hand: the code in question is in statapprox_heap, which
is presumably meant to deliver numbers comparable to pgstat_heap.
And pgstat_heap takes no special care for "new" pages, it just applies
PageGetHeapFreeSpace (or PageGetExactFreeSpace after this patch).
So that leaves me feeling pretty strongly that this whole stanza
is wrong and we should just do PageGetExactFreeSpace here.
+1
v4 patch attached.
Best regards,
Frédéric
From a59c16d33ff37ce5c9d0e663809be190c608c75b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Yhuel?= <frederic.yh...@dalibo.com>
Date: Wed, 21 Aug 2024 15:05:11 +0200
Subject: [PATCH v4] pgstattuple: use PageGetExactFreeSpace()
instead of PageGetHeapFreeSpace() or PageGetFreeSpace()
We want the exact free space, we don't care if there's enough room for
another line pointer.
Also, statapprox_heap() shouldn't have to take any special care with new
pages.
---
contrib/pgstattuple/pgstatapprox.c | 9 +--------
contrib/pgstattuple/pgstatindex.c | 2 +-
contrib/pgstattuple/pgstattuple.c | 6 +++---
3 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/contrib/pgstattuple/pgstatapprox.c b/contrib/pgstattuple/pgstatapprox.c
index c84c642355..04457f4b79 100644
--- a/contrib/pgstattuple/pgstatapprox.c
+++ b/contrib/pgstattuple/pgstatapprox.c
@@ -106,14 +106,7 @@ statapprox_heap(Relation rel, output_type *stat)
page = BufferGetPage(buf);
- /*
- * It's not safe to call PageGetHeapFreeSpace() on new pages, so we
- * treat them as being free space for our purposes.
- */
- if (!PageIsNew(page))
- stat->free_space += PageGetHeapFreeSpace(page);
- else
- stat->free_space += BLCKSZ - SizeOfPageHeaderData;
+ stat->free_space += PageGetExactFreeSpace(page);
/* We may count the page as scanned even if it's new/empty */
scanned++;
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 5c06ba6db4..1b6b768cf8 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -311,7 +311,7 @@ pgstatindex_impl(Relation rel, FunctionCallInfo fcinfo)
max_avail = BLCKSZ - (BLCKSZ - ((PageHeader) page)->pd_special + SizeOfPageHeaderData);
indexStat.max_avail += max_avail;
- indexStat.free_space += PageGetFreeSpace(page);
+ indexStat.free_space += PageGetExactFreeSpace(page);
indexStat.leaf_pages++;
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 3bd8b96197..7e2a7262a3 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -372,7 +372,7 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo)
buffer = ReadBufferExtended(rel, MAIN_FORKNUM, block,
RBM_NORMAL, hscan->rs_strategy);
LockBuffer(buffer, BUFFER_LOCK_SHARE);
- stat.free_space += PageGetHeapFreeSpace((Page) BufferGetPage(buffer));
+ stat.free_space += PageGetExactFreeSpace((Page) BufferGetPage(buffer));
UnlockReleaseBuffer(buffer);
block++;
}
@@ -385,7 +385,7 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo)
buffer = ReadBufferExtended(rel, MAIN_FORKNUM, block,
RBM_NORMAL, hscan->rs_strategy);
LockBuffer(buffer, BUFFER_LOCK_SHARE);
- stat.free_space += PageGetHeapFreeSpace((Page) BufferGetPage(buffer));
+ stat.free_space += PageGetExactFreeSpace((Page) BufferGetPage(buffer));
UnlockReleaseBuffer(buffer);
block++;
}
@@ -565,7 +565,7 @@ pgstat_index_page(pgstattuple_type *stat, Page page,
{
OffsetNumber i;
- stat->free_space += PageGetFreeSpace(page);
+ stat->free_space += PageGetExactFreeSpace(page);
for (i = minoff; i <= maxoff; i = OffsetNumberNext(i))
{
--
2.39.2
