Remove unneeded cast in heap_xlog_lock.
Hi!
I was looking at how PostgreSQL handles VM map bits, when I noticed $subj.
PFA small refactoring patch.
--
Best regards,
Kirill Reshke
Attachments:
v1-0001-Remove-unneeded-cast-in-heap_xlog_lock.patchapplication/octet-stream; name=v1-0001-Remove-unneeded-cast-in-heap_xlog_lock.patchDownload
From 60e7d72784283b65bee18d790b682f56cdb3459f Mon Sep 17 00:00:00 2001
From: reshke <reshke@double.cloud>
Date: Thu, 21 Aug 2025 14:06:34 +0000
Subject: [PATCH v1] Remove unneeded cast in heap_xlog_lock.
---
src/backend/access/heap/heapam_xlog.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/access/heap/heapam_xlog.c b/src/backend/access/heap/heapam_xlog.c
index 978030048b6..54237cb19c9 100644
--- a/src/backend/access/heap/heapam_xlog.c
+++ b/src/backend/access/heap/heapam_xlog.c
@@ -1028,7 +1028,7 @@ heap_xlog_lock(XLogReaderState *record)
if (XLogReadBufferForRedo(record, 0, &buffer) == BLK_NEEDS_REDO)
{
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
offnum = xlrec->offnum;
if (PageGetMaxOffsetNumber(page) >= offnum)
--
2.43.0
On Aug 21, 2025, at 22:10, Kirill Reshke <reshkekirill@gmail.com> wrote:
Hi!
I was looking at how PostgreSQL handles VM map bits, when I noticed $subj.
PFA small refactoring patch.--
Best regards,
Kirill Reshke
<v1-0001-Remove-unneeded-cast-in-heap_xlog_lock.patch>
LGTM. BufferGetPage() returns a “Page” type, and the receiving variable “page” is of “Page” type as well, so the cast is not needed.
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Fri, Aug 22, 2025 at 9:44 AM Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 21, 2025, at 22:10, Kirill Reshke <reshkekirill@gmail.com> wrote:
I was looking at how PostgreSQL handles VM map bits, when I noticed $subj.
PFA small refactoring patch.
LGTM. BufferGetPage() returns a “Page” type, and the receiving variable “page” is of “Page” type as well, so the cast is not needed.
If you run 'git grep', you'll find a lot more instances where the
result of BufferGetPage() is explicitly cast to Page.
git grep -rn "(Page) BufferGetPage" | wc -l
69
Although these casts are unnecessary for sure, I'm not sure if it's
worth making the code changes to fix them.
Thanks
Richard
On Fri, Aug 22, 2025 at 10:41:15AM +0900, Richard Guo wrote:
Although these casts are unnecessary for sure, I'm not sure if it's
worth making the code changes to fix them.
That's sort of the point. This is not code that needs to be fixed,
because it's not broken.
--
Michael
On Fri, 22 Aug 2025 at 06:41, Richard Guo <guofenglinux@gmail.com> wrote:
If you run 'git grep', you'll find a lot more instances where the
result of BufferGetPage() is explicitly cast to Page.git grep -rn "(Page) BufferGetPage" | wc -l
69Although these casts are unnecessary for sure, I'm not sure if it's
worth making the code changes to fix them.
I can see your point. But I can see that there is a great amount of
commits in HEAD (every now and then), which are just pure refactoring
and standardizing things.
I am uncertain about the delineation between when we make changes and
when we refrain from doing so.
I do not insist on this modification. I just spotted two completely
same codes in [0]https://github.com/postgres/postgres/blob/13b935c/src/backend/access/heap/heapam_xlog.c#L1007-L1050 & [1]https://github.com/postgres/postgres/blob/13b935c/src/backend/access/heap/heapam_xlog.c#L1083-L1121, which only differ in BufferGetPage cast. And
I merely tried to do something with it.
v2 attached with all 69 casts removed, but I see there is a little
chance of this committed.
[0]: https://github.com/postgres/postgres/blob/13b935c/src/backend/access/heap/heapam_xlog.c#L1007-L1050
[1]: https://github.com/postgres/postgres/blob/13b935c/src/backend/access/heap/heapam_xlog.c#L1083-L1121
--
Best regards,
Kirill Reshke
Attachments:
v2-0001-Remove-unneeded-cast-in-BufferGetPage.patchapplication/octet-stream; name=v2-0001-Remove-unneeded-cast-in-BufferGetPage.patchDownload
From adf2ed10dde3e8f48863b5d941de40218cd76066 Mon Sep 17 00:00:00 2001
From: reshke <reshke@double.cloud>
Date: Thu, 21 Aug 2025 14:06:34 +0000
Subject: [PATCH v2] Remove unneeded cast in BufferGetPage
---
contrib/amcheck/verify_gin.c | 4 ++--
contrib/bloom/blvacuum.c | 2 +-
contrib/pgstattuple/pgstatindex.c | 2 +-
contrib/pgstattuple/pgstattuple.c | 4 ++--
src/backend/access/brin/brin_xlog.c | 12 +++++-----
src/backend/access/gin/ginvacuum.c | 2 +-
src/backend/access/gin/ginxlog.c | 4 ++--
src/backend/access/gist/gist.c | 14 ++++++------
src/backend/access/gist/gistbuild.c | 4 ++--
src/backend/access/gist/gistvacuum.c | 4 ++--
src/backend/access/gist/gistxlog.c | 10 ++++-----
src/backend/access/hash/hash_xlog.c | 18 +++++++--------
src/backend/access/heap/heapam.c | 2 +-
src/backend/access/heap/heapam_handler.c | 2 +-
src/backend/access/heap/heapam_xlog.c | 8 +++----
src/backend/access/heap/pruneheap.c | 2 +-
src/backend/access/heap/visibilitymap.c | 2 +-
src/backend/access/nbtree/nbtxlog.c | 28 ++++++++++++------------
src/backend/access/spgist/spgvacuum.c | 4 ++--
src/backend/access/spgist/spgxlog.c | 6 ++---
src/backend/access/transam/xlogutils.c | 2 +-
src/backend/commands/sequence.c | 2 +-
22 files changed, 69 insertions(+), 69 deletions(-)
diff --git a/contrib/amcheck/verify_gin.c b/contrib/amcheck/verify_gin.c
index c615d950736..5c3eb4d0fd4 100644
--- a/contrib/amcheck/verify_gin.c
+++ b/contrib/amcheck/verify_gin.c
@@ -174,7 +174,7 @@ gin_check_posting_tree_parent_keys_consistency(Relation rel, BlockNumber posting
buffer = ReadBufferExtended(rel, MAIN_FORKNUM, stack->blkno,
RBM_NORMAL, strategy);
LockBuffer(buffer, GIN_SHARE);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
Assert(GinPageIsData(page));
@@ -434,7 +434,7 @@ gin_check_parent_keys_consistency(Relation rel,
buffer = ReadBufferExtended(rel, MAIN_FORKNUM, stack->blkno,
RBM_NORMAL, strategy);
LockBuffer(buffer, GIN_SHARE);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
maxoff = PageGetMaxOffsetNumber(page);
rightlink = GinPageGetOpaque(page)->rightlink;
diff --git a/contrib/bloom/blvacuum.c b/contrib/bloom/blvacuum.c
index 86b15a75f6f..9e5f0574fad 100644
--- a/contrib/bloom/blvacuum.c
+++ b/contrib/bloom/blvacuum.c
@@ -192,7 +192,7 @@ blvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
buffer = ReadBufferExtended(index, MAIN_FORKNUM, blkno,
RBM_NORMAL, info->strategy);
LockBuffer(buffer, BUFFER_LOCK_SHARE);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
if (PageIsNew(page) || BloomPageIsDeleted(page))
{
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 4b9d76ec4e4..40823d54fca 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -647,7 +647,7 @@ pgstathashindex(PG_FUNCTION_ARGS)
buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL,
bstrategy);
LockBuffer(buf, BUFFER_LOCK_SHARE);
- page = (Page) BufferGetPage(buf);
+ page = BufferGetPage(buf);
if (PageIsNew(page))
stats.unused_pages++;
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 0d9c2b0b653..b5de68b7232 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -378,7 +378,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 += PageGetExactFreeSpace((Page) BufferGetPage(buffer));
+ stat.free_space += PageGetExactFreeSpace(BufferGetPage(buffer));
UnlockReleaseBuffer(buffer);
block++;
}
@@ -391,7 +391,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 += PageGetExactFreeSpace((Page) BufferGetPage(buffer));
+ stat.free_space += PageGetExactFreeSpace(BufferGetPage(buffer));
UnlockReleaseBuffer(buffer);
block++;
}
diff --git a/src/backend/access/brin/brin_xlog.c b/src/backend/access/brin/brin_xlog.c
index 85db2f0fd5a..55348140fad 100644
--- a/src/backend/access/brin/brin_xlog.c
+++ b/src/backend/access/brin/brin_xlog.c
@@ -31,7 +31,7 @@ brin_xlog_createidx(XLogReaderState *record)
/* create the index' metapage */
buf = XLogInitBufferForRedo(record, 0);
Assert(BufferIsValid(buf));
- page = (Page) BufferGetPage(buf);
+ page = BufferGetPage(buf);
brin_metapage_init(page, xlrec->pagesPerRange, xlrec->version);
PageSetLSN(page, lsn);
MarkBufferDirty(buf);
@@ -82,7 +82,7 @@ brin_xlog_insert_update(XLogReaderState *record,
Assert(tuple->bt_blkno == xlrec->heapBlk);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
offnum = xlrec->offnum;
if (PageGetMaxOffsetNumber(page) + 1 < offnum)
elog(PANIC, "brin_xlog_insert_update: invalid max offset number");
@@ -104,7 +104,7 @@ brin_xlog_insert_update(XLogReaderState *record,
ItemPointerData tid;
ItemPointerSet(&tid, regpgno, xlrec->offnum);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
brinSetHeapBlockItemptr(buffer, xlrec->pagesPerRange, xlrec->heapBlk,
tid);
@@ -146,7 +146,7 @@ brin_xlog_update(XLogReaderState *record)
Page page;
OffsetNumber offnum;
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
offnum = xlrec->oldOffnum;
@@ -185,7 +185,7 @@ brin_xlog_samepage_update(XLogReaderState *record)
brintuple = (BrinTuple *) XLogRecGetBlockData(record, 0, &tuplen);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
offnum = xlrec->offnum;
@@ -254,7 +254,7 @@ brin_xlog_revmap_extend(XLogReaderState *record)
*/
buf = XLogInitBufferForRedo(record, 1);
- page = (Page) BufferGetPage(buf);
+ page = BufferGetPage(buf);
brin_page_init(page, BRIN_PAGETYPE_REVMAP);
PageSetLSN(page, lsn);
diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c
index fbbe3a6dd70..2d833d6d618 100644
--- a/src/backend/access/gin/ginvacuum.c
+++ b/src/backend/access/gin/ginvacuum.c
@@ -753,7 +753,7 @@ ginvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
buffer = ReadBufferExtended(index, MAIN_FORKNUM, blkno,
RBM_NORMAL, info->strategy);
LockBuffer(buffer, GIN_SHARE);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
if (GinPageIsRecyclable(page))
{
diff --git a/src/backend/access/gin/ginxlog.c b/src/backend/access/gin/ginxlog.c
index 55a1ec09776..4478e928204 100644
--- a/src/backend/access/gin/ginxlog.c
+++ b/src/backend/access/gin/ginxlog.c
@@ -30,7 +30,7 @@ ginRedoClearIncompleteSplit(XLogReaderState *record, uint8 block_id)
if (XLogReadBufferForRedo(record, block_id, &buffer) == BLK_NEEDS_REDO)
{
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
GinPageGetOpaque(page)->flags &= ~GIN_INCOMPLETE_SPLIT;
PageSetLSN(page, lsn);
@@ -50,7 +50,7 @@ ginRedoCreatePTree(XLogReaderState *record)
Page page;
buffer = XLogInitBufferForRedo(record, 0);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
GinInitBuffer(buffer, GIN_DATA | GIN_LEAF | GIN_COMPRESSED);
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 7b24380c978..a96796d5c7d 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -696,7 +696,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace,
gistcheckpage(state.r, stack->buffer);
}
- stack->page = (Page) BufferGetPage(stack->buffer);
+ stack->page = BufferGetPage(stack->buffer);
stack->lsn = xlocked ?
PageGetLSN(stack->page) : BufferGetLSNAtomic(stack->buffer);
Assert(!RelationNeedsWAL(state.r) || !XLogRecPtrIsInvalid(stack->lsn));
@@ -783,7 +783,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace,
LockBuffer(stack->buffer, GIST_UNLOCK);
LockBuffer(stack->buffer, GIST_EXCLUSIVE);
xlocked = true;
- stack->page = (Page) BufferGetPage(stack->buffer);
+ stack->page = BufferGetPage(stack->buffer);
if (PageGetLSN(stack->page) != stack->lsn)
{
@@ -847,7 +847,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace,
LockBuffer(stack->buffer, GIST_UNLOCK);
LockBuffer(stack->buffer, GIST_EXCLUSIVE);
xlocked = true;
- stack->page = (Page) BufferGetPage(stack->buffer);
+ stack->page = BufferGetPage(stack->buffer);
stack->lsn = PageGetLSN(stack->page);
if (stack->blkno == GIST_ROOT_BLKNO)
@@ -938,7 +938,7 @@ gistFindPath(Relation r, BlockNumber child, OffsetNumber *downlinkoffnum)
buffer = ReadBuffer(r, top->blkno);
LockBuffer(buffer, GIST_SHARE);
gistcheckpage(r, buffer);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
if (GistPageIsLeaf(page))
{
@@ -1033,7 +1033,7 @@ gistFindCorrectParent(Relation r, GISTInsertStack *child, bool is_build)
GISTInsertStack *ptr;
gistcheckpage(r, parent->buffer);
- parent->page = (Page) BufferGetPage(parent->buffer);
+ parent->page = BufferGetPage(parent->buffer);
maxoff = PageGetMaxOffsetNumber(parent->page);
/* Check if the downlink is still where it was before */
@@ -1098,7 +1098,7 @@ gistFindCorrectParent(Relation r, GISTInsertStack *child, bool is_build)
parent->buffer = ReadBuffer(r, parent->blkno);
LockBuffer(parent->buffer, GIST_EXCLUSIVE);
gistcheckpage(r, parent->buffer);
- parent->page = (Page) BufferGetPage(parent->buffer);
+ parent->page = BufferGetPage(parent->buffer);
}
/*
@@ -1121,7 +1121,7 @@ gistFindCorrectParent(Relation r, GISTInsertStack *child, bool is_build)
while (ptr)
{
ptr->buffer = ReadBuffer(r, ptr->blkno);
- ptr->page = (Page) BufferGetPage(ptr->buffer);
+ ptr->page = BufferGetPage(ptr->buffer);
ptr = ptr->parent;
}
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 9e707167d98..9b2ec9815f1 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -969,7 +969,7 @@ gistProcessItup(GISTBuildState *buildstate, IndexTuple itup,
buffer = ReadBuffer(indexrel, blkno);
LockBuffer(buffer, GIST_EXCLUSIVE);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
childoffnum = gistchoose(indexrel, page, itup, giststate);
iid = PageGetItemId(page, childoffnum);
idxtuple = (IndexTuple) PageGetItem(page, iid);
@@ -1448,7 +1448,7 @@ gistGetMaxLevel(Relation index)
* pro forma.
*/
LockBuffer(buffer, GIST_SHARE);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
if (GistPageIsLeaf(page))
{
diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.c
index dca236b6e57..b925eda2b9b 100644
--- a/src/backend/access/gist/gistvacuum.c
+++ b/src/backend/access/gist/gistvacuum.c
@@ -330,7 +330,7 @@ restart:
* exclusive lock.
*/
LockBuffer(buffer, GIST_EXCLUSIVE);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
if (gistPageRecyclable(page))
{
@@ -528,7 +528,7 @@ gistvacuum_delete_empty_pages(IndexVacuumInfo *info, GistVacState *vstate)
RBM_NORMAL, info->strategy);
LockBuffer(buffer, GIST_SHARE);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
if (PageIsNew(page) || GistPageIsDeleted(page) || GistPageIsLeaf(page))
{
diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
index b354e4ba5d1..42fee1f0764 100644
--- a/src/backend/access/gist/gistxlog.c
+++ b/src/backend/access/gist/gistxlog.c
@@ -83,7 +83,7 @@ gistRedoPageUpdateRecord(XLogReaderState *record)
data = begin = XLogRecGetBlockData(record, 0, &datalen);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
if (xldata->ntodelete == 1 && xldata->ntoinsert == 1)
{
@@ -201,7 +201,7 @@ gistRedoDeleteRecord(XLogReaderState *record)
if (XLogReadBufferForRedo(record, 0, &buffer) == BLK_NEEDS_REDO)
{
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
PageIndexMultiDelete(page, toDelete, xldata->ntodelete);
@@ -280,7 +280,7 @@ gistRedoPageSplitRecord(XLogReaderState *record)
}
buffer = XLogInitBufferForRedo(record, i + 1);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
data = XLogRecGetBlockData(record, i + 1, &datalen);
tuples = decodePageSplitRecord(data, datalen, &num);
@@ -348,7 +348,7 @@ gistRedoPageDelete(XLogReaderState *record)
if (XLogReadBufferForRedo(record, 0, &leafBuffer) == BLK_NEEDS_REDO)
{
- Page page = (Page) BufferGetPage(leafBuffer);
+ Page page = BufferGetPage(leafBuffer);
GistPageSetDeleted(page, xldata->deleteXid);
@@ -358,7 +358,7 @@ gistRedoPageDelete(XLogReaderState *record)
if (XLogReadBufferForRedo(record, 1, &parentBuffer) == BLK_NEEDS_REDO)
{
- Page page = (Page) BufferGetPage(parentBuffer);
+ Page page = BufferGetPage(parentBuffer);
PageIndexTupleDelete(page, xldata->downlinkOffset);
diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c
index 8d97067fe54..d963a0c3702 100644
--- a/src/backend/access/hash/hash_xlog.c
+++ b/src/backend/access/hash/hash_xlog.c
@@ -38,7 +38,7 @@ hash_xlog_init_meta_page(XLogReaderState *record)
Assert(BufferIsValid(metabuf));
_hash_init_metabuffer(metabuf, xlrec->num_tuples, xlrec->procid,
xlrec->ffactor, true);
- page = (Page) BufferGetPage(metabuf);
+ page = BufferGetPage(metabuf);
PageSetLSN(page, lsn);
MarkBufferDirty(metabuf);
@@ -235,7 +235,7 @@ hash_xlog_add_ovfl_page(XLogReaderState *record)
if (XLogReadBufferForRedo(record, 2, &mapbuffer) == BLK_NEEDS_REDO)
{
- Page mappage = (Page) BufferGetPage(mapbuffer);
+ Page mappage = BufferGetPage(mapbuffer);
uint32 *freep = NULL;
uint32 *bitmap_page_bit;
@@ -538,7 +538,7 @@ hash_xlog_move_page_contents(XLogReaderState *record)
data = begin = XLogRecGetBlockData(record, 1, &datalen);
- writepage = (Page) BufferGetPage(writebuf);
+ writepage = BufferGetPage(writebuf);
if (xldata->ntups > 0)
{
@@ -584,7 +584,7 @@ hash_xlog_move_page_contents(XLogReaderState *record)
ptr = XLogRecGetBlockData(record, 2, &len);
- page = (Page) BufferGetPage(deletebuf);
+ page = BufferGetPage(deletebuf);
if (len > 0)
{
@@ -670,7 +670,7 @@ hash_xlog_squeeze_page(XLogReaderState *record)
data = begin = XLogRecGetBlockData(record, 1, &datalen);
- writepage = (Page) BufferGetPage(writebuf);
+ writepage = BufferGetPage(writebuf);
if (xldata->ntups > 0)
{
@@ -807,7 +807,7 @@ hash_xlog_squeeze_page(XLogReaderState *record)
/* replay the record for bitmap page */
if (XLogReadBufferForRedo(record, 5, &mapbuf) == BLK_NEEDS_REDO)
{
- Page mappage = (Page) BufferGetPage(mapbuf);
+ Page mappage = BufferGetPage(mapbuf);
uint32 *freep = NULL;
char *data;
uint32 *bitmap_page_bit;
@@ -895,7 +895,7 @@ hash_xlog_delete(XLogReaderState *record)
ptr = XLogRecGetBlockData(record, 1, &len);
- page = (Page) BufferGetPage(deletebuf);
+ page = BufferGetPage(deletebuf);
if (len > 0)
{
@@ -946,7 +946,7 @@ hash_xlog_split_cleanup(XLogReaderState *record)
{
HashPageOpaque bucket_opaque;
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
bucket_opaque = HashPageGetOpaque(page);
bucket_opaque->hasho_flag &= ~LH_BUCKET_NEEDS_SPLIT_CLEANUP;
@@ -1029,7 +1029,7 @@ hash_xlog_vacuum_one_page(XLogReaderState *record)
if (action == BLK_NEEDS_REDO)
{
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
PageIndexMultiDelete(page, toDelete, xldata->ntuples);
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index e5da051aaf0..18d6c691762 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6091,7 +6091,7 @@ heap_finish_speculative(Relation relation, ItemPointer tid)
buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(tid));
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
offnum = ItemPointerGetOffsetNumber(tid);
if (PageGetMaxOffsetNumber(page) >= offnum)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index cb4bc35c93e..bcbac844bb6 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2280,7 +2280,7 @@ heapam_scan_sample_next_tuple(TableScanDesc scan, SampleScanState *scanstate,
if (!pagemode)
LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_SHARE);
- page = (Page) BufferGetPage(hscan->rs_cbuf);
+ page = BufferGetPage(hscan->rs_cbuf);
all_visible = PageIsAllVisible(page) &&
!scan->rs_snapshot->takenDuringRecovery;
maxoffset = PageGetMaxOffsetNumber(page);
diff --git a/src/backend/access/heap/heapam_xlog.c b/src/backend/access/heap/heapam_xlog.c
index 978030048b6..6bdfcaac574 100644
--- a/src/backend/access/heap/heapam_xlog.c
+++ b/src/backend/access/heap/heapam_xlog.c
@@ -79,7 +79,7 @@ heap_xlog_prune_freeze(XLogReaderState *record)
&buffer);
if (action == BLK_NEEDS_REDO)
{
- Page page = (Page) BufferGetPage(buffer);
+ Page page = BufferGetPage(buffer);
OffsetNumber *redirected;
OffsetNumber *nowdead;
OffsetNumber *nowunused;
@@ -600,7 +600,7 @@ heap_xlog_multi_insert(XLogReaderState *record)
tupdata = XLogRecGetBlockData(record, 0, &len);
endptr = tupdata + len;
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
for (i = 0; i < xlrec->ntuples; i++)
{
@@ -802,7 +802,7 @@ heap_xlog_update(XLogReaderState *record, bool hot_update)
else if (XLogRecGetInfo(record) & XLOG_HEAP_INIT_PAGE)
{
nbuffer = XLogInitBufferForRedo(record, 0);
- page = (Page) BufferGetPage(nbuffer);
+ page = BufferGetPage(nbuffer);
PageInit(page, BufferGetPageSize(nbuffer), 0);
newaction = BLK_NEEDS_REDO;
}
@@ -1028,7 +1028,7 @@ heap_xlog_lock(XLogReaderState *record)
if (XLogReadBufferForRedo(record, 0, &buffer) == BLK_NEEDS_REDO)
{
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
offnum = xlrec->offnum;
if (PageGetMaxOffsetNumber(page) >= offnum)
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index a8025889be0..7ebd22f00a3 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -1563,7 +1563,7 @@ heap_page_prune_execute(Buffer buffer, bool lp_truncate_only,
OffsetNumber *nowdead, int ndead,
OffsetNumber *nowunused, int nunused)
{
- Page page = (Page) BufferGetPage(buffer);
+ Page page = BufferGetPage(buffer);
OffsetNumber *offnum;
HeapTupleHeader htup PG_USED_FOR_ASSERTS_ONLY;
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index 8f918e00af7..953ad4a4843 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -259,7 +259,7 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
#endif
Assert(InRecovery || XLogRecPtrIsInvalid(recptr));
- Assert(InRecovery || PageIsAllVisible((Page) BufferGetPage(heapBuf)));
+ Assert(InRecovery || PageIsAllVisible(BufferGetPage(heapBuf)));
Assert((flags & VISIBILITYMAP_VALID_BITS) == flags);
/* Must never set all_frozen bit without also setting all_visible bit */
diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index d31dd56732d..69ea668bb0d 100644
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/nbtxlog.c
@@ -143,7 +143,7 @@ _bt_clear_incomplete_split(XLogReaderState *record, uint8 block_id)
if (XLogReadBufferForRedo(record, block_id, &buf) == BLK_NEEDS_REDO)
{
- Page page = (Page) BufferGetPage(buf);
+ Page page = BufferGetPage(buf);
BTPageOpaque pageop = BTPageGetOpaque(page);
Assert(P_INCOMPLETE_SPLIT(pageop));
@@ -287,7 +287,7 @@ btree_xlog_split(bool newitemonleft, XLogReaderState *record)
/* Reconstruct right (new) sibling page from scratch */
rbuf = XLogInitBufferForRedo(record, 1);
datapos = XLogRecGetBlockData(record, 1, &datalen);
- rpage = (Page) BufferGetPage(rbuf);
+ rpage = BufferGetPage(rbuf);
_bt_pageinit(rpage, BufferGetPageSize(rbuf));
ropaque = BTPageGetOpaque(rpage);
@@ -314,7 +314,7 @@ btree_xlog_split(bool newitemonleft, XLogReaderState *record)
* checking possible. See also _bt_restore_page(), which does the
* same for the right page.
*/
- Page origpage = (Page) BufferGetPage(buf);
+ Page origpage = BufferGetPage(buf);
BTPageOpaque oopaque = BTPageGetOpaque(origpage);
OffsetNumber off;
IndexTuple newitem = NULL,
@@ -439,7 +439,7 @@ btree_xlog_split(bool newitemonleft, XLogReaderState *record)
if (XLogReadBufferForRedo(record, 2, &sbuf) == BLK_NEEDS_REDO)
{
- Page spage = (Page) BufferGetPage(sbuf);
+ Page spage = BufferGetPage(sbuf);
BTPageOpaque spageop = BTPageGetOpaque(spage);
spageop->btpo_prev = rightpagenumber;
@@ -470,7 +470,7 @@ btree_xlog_dedup(XLogReaderState *record)
if (XLogReadBufferForRedo(record, 0, &buf) == BLK_NEEDS_REDO)
{
char *ptr = XLogRecGetBlockData(record, 0, NULL);
- Page page = (Page) BufferGetPage(buf);
+ Page page = BufferGetPage(buf);
BTPageOpaque opaque = BTPageGetOpaque(page);
OffsetNumber offnum,
minoff,
@@ -614,7 +614,7 @@ btree_xlog_vacuum(XLogReaderState *record)
{
char *ptr = XLogRecGetBlockData(record, 0, NULL);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
if (xlrec->nupdated > 0)
{
@@ -680,7 +680,7 @@ btree_xlog_delete(XLogReaderState *record)
{
char *ptr = XLogRecGetBlockData(record, 0, NULL);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
if (xlrec->nupdated > 0)
{
@@ -740,7 +740,7 @@ btree_xlog_mark_page_halfdead(uint8 info, XLogReaderState *record)
OffsetNumber nextoffset;
BlockNumber rightsib;
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
pageop = BTPageGetOpaque(page);
poffset = xlrec->poffset;
@@ -769,7 +769,7 @@ btree_xlog_mark_page_halfdead(uint8 info, XLogReaderState *record)
/* Rewrite the leaf page as a halfdead page */
buffer = XLogInitBufferForRedo(record, 0);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
_bt_pageinit(page, BufferGetPageSize(buffer));
pageop = BTPageGetOpaque(page);
@@ -836,7 +836,7 @@ btree_xlog_unlink_page(uint8 info, XLogReaderState *record)
{
if (XLogReadBufferForRedo(record, 1, &leftbuf) == BLK_NEEDS_REDO)
{
- page = (Page) BufferGetPage(leftbuf);
+ page = BufferGetPage(leftbuf);
pageop = BTPageGetOpaque(page);
pageop->btpo_next = rightsib;
@@ -849,7 +849,7 @@ btree_xlog_unlink_page(uint8 info, XLogReaderState *record)
/* Rewrite target page as empty deleted page */
target = XLogInitBufferForRedo(record, 0);
- page = (Page) BufferGetPage(target);
+ page = BufferGetPage(target);
_bt_pageinit(page, BufferGetPageSize(target));
pageop = BTPageGetOpaque(page);
@@ -868,7 +868,7 @@ btree_xlog_unlink_page(uint8 info, XLogReaderState *record)
/* Fix left-link of right sibling */
if (XLogReadBufferForRedo(record, 2, &rightbuf) == BLK_NEEDS_REDO)
{
- page = (Page) BufferGetPage(rightbuf);
+ page = BufferGetPage(rightbuf);
pageop = BTPageGetOpaque(page);
pageop->btpo_prev = leftsib;
@@ -907,7 +907,7 @@ btree_xlog_unlink_page(uint8 info, XLogReaderState *record)
Assert(!isleaf);
leafbuf = XLogInitBufferForRedo(record, 3);
- page = (Page) BufferGetPage(leafbuf);
+ page = BufferGetPage(leafbuf);
_bt_pageinit(page, BufferGetPageSize(leafbuf));
pageop = BTPageGetOpaque(page);
@@ -949,7 +949,7 @@ btree_xlog_newroot(XLogReaderState *record)
Size len;
buffer = XLogInitBufferForRedo(record, 0);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
_bt_pageinit(page, BufferGetPageSize(buffer));
pageop = BTPageGetOpaque(page);
diff --git a/src/backend/access/spgist/spgvacuum.c b/src/backend/access/spgist/spgvacuum.c
index 2678f7ab782..8f8a1ad7796 100644
--- a/src/backend/access/spgist/spgvacuum.c
+++ b/src/backend/access/spgist/spgvacuum.c
@@ -626,7 +626,7 @@ spgvacuumpage(spgBulkDeleteState *bds, Buffer buffer)
Page page;
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
if (PageIsNew(page))
{
@@ -707,7 +707,7 @@ spgprocesspending(spgBulkDeleteState *bds)
buffer = ReadBufferExtended(index, MAIN_FORKNUM, blkno,
RBM_NORMAL, bds->info->strategy);
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
if (PageIsNew(page) || SpGistPageIsDeleted(page))
{
diff --git a/src/backend/access/spgist/spgxlog.c b/src/backend/access/spgist/spgxlog.c
index b7986e6f713..d4620c915d0 100644
--- a/src/backend/access/spgist/spgxlog.c
+++ b/src/backend/access/spgist/spgxlog.c
@@ -576,7 +576,7 @@ spgRedoPickSplit(XLogReaderState *record)
{
/* just re-init the source page */
srcBuffer = XLogInitBufferForRedo(record, 0);
- srcPage = (Page) BufferGetPage(srcBuffer);
+ srcPage = BufferGetPage(srcBuffer);
SpGistInitBuffer(srcBuffer,
SPGIST_LEAF | (xldata->storesNulls ? SPGIST_NULLS : 0));
@@ -629,7 +629,7 @@ spgRedoPickSplit(XLogReaderState *record)
{
/* just re-init the dest page */
destBuffer = XLogInitBufferForRedo(record, 1);
- destPage = (Page) BufferGetPage(destBuffer);
+ destPage = BufferGetPage(destBuffer);
SpGistInitBuffer(destBuffer,
SPGIST_LEAF | (xldata->storesNulls ? SPGIST_NULLS : 0));
@@ -642,7 +642,7 @@ spgRedoPickSplit(XLogReaderState *record)
* full-page-image case, but for safety let's hold it till later.
*/
if (XLogReadBufferForRedo(record, 1, &destBuffer) == BLK_NEEDS_REDO)
- destPage = (Page) BufferGetPage(destBuffer);
+ destPage = BufferGetPage(destBuffer);
else
destPage = NULL; /* don't do any page updates */
}
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 27ea52fdfee..38176d9688e 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -523,7 +523,7 @@ recent_buffer_fast_path:
if (mode == RBM_NORMAL)
{
/* check that page has been initialized */
- Page page = (Page) BufferGetPage(buffer);
+ Page page = BufferGetPage(buffer);
/*
* We assume that PageIsNew is safe without a lock. During recovery,
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index a3c8cff97b0..636d3c3ec73 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1920,7 +1920,7 @@ seq_redo(XLogReaderState *record)
elog(PANIC, "seq_redo: unknown op code %u", info);
buffer = XLogInitBufferForRedo(record, 0);
- page = (Page) BufferGetPage(buffer);
+ page = BufferGetPage(buffer);
/*
* We always reinit the page. However, since this WAL record type is also
--
2.43.0
On 2025-Aug-22, Kirill Reshke wrote:
I am uncertain about the delineation between when we make changes and
when we refrain from doing so.
I think this is natural work after 9c727360bcc7, before which
BufferGetPage() was a macro and strangely enough had its own cast
embedded. As I understand, the less casts we have, the better. There's
some other standardization work going on to remove unnecessary casts
elsewhere, so I'm not sure why we wouldn't do this.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"El que vive para el futuro es un iluso, y el que vive para el pasado,
un imbécil" (Luis Adler, "Los tripulantes de la noche")
On Aug 22, 2025, at 13:36, Kirill Reshke <reshkekirill@gmail.com> wrote:
I do not insist on this modification. I just spotted two completely
same codes in [0] & [1], which only differ in BufferGetPage cast. And
I merely tried to do something with it.v2 attached with all 69 casts removed, but I see there is a little
chance of this committed.[0] https://github.com/postgres/postgres/blob/13b935c/src/backend/access/heap/heapam_xlog.c#L1007-L1050
[1] https://github.com/postgres/postgres/blob/13b935c/src/backend/access/heap/heapam_xlog.c#L1083-L1121--
Best regards,
Kirill Reshke
<v2-0001-Remove-unneeded-cast-in-BufferGetPage.patch>
As there are only 76 occurrences, I think using one commit to clean up the unnecessary cast is worthwhile.
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Fri, Aug 22, 2025 at 6:59 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Aug-22, Kirill Reshke wrote:
I am uncertain about the delineation between when we make changes and
when we refrain from doing so.
I think this is natural work after 9c727360bcc7, before which
BufferGetPage() was a macro and strangely enough had its own cast
embedded. As I understand, the less casts we have, the better. There's
some other standardization work going on to remove unnecessary casts
elsewhere, so I'm not sure why we wouldn't do this.
I don't have a strong opinion on whether we should do this cleanup or
not. I'm a bit concerned about the code churn, given that there are
69 instances spread across 22 files. But maybe I'm worrying over
nothing, as we've done similar cleanups before to remove unnecessary
casts.
Thanks
Richard
On 22.08.25 11:59, Álvaro Herrera wrote:
On 2025-Aug-22, Kirill Reshke wrote:
I am uncertain about the delineation between when we make changes and
when we refrain from doing so.I think this is natural work after 9c727360bcc7, before which
BufferGetPage() was a macro and strangely enough had its own cast
embedded. As I understand, the less casts we have, the better. There's
some other standardization work going on to remove unnecessary casts
elsewhere, so I'm not sure why we wouldn't do this.
In the very first code import, BufferGetPage() was a regular function
that returned Page. (I suppose it was then turned into a macro, and
then back into an inline function.) Even in that first code import,
some callers cast the return to (Page), and some not. So I suppose this
style just crept in for some random and ancient reason and then got
copied around inconsistently. We should clean it up. Casts are bad.
On Sat, 23 Aug 2025 at 19:57, Peter Eisentraut <peter@eisentraut.org> wrote:
On 22.08.25 11:59, Álvaro Herrera wrote:
On 2025-Aug-22, Kirill Reshke wrote:
I am uncertain about the delineation between when we make changes and
when we refrain from doing so.I think this is natural work after 9c727360bcc7, before which
BufferGetPage() was a macro and strangely enough had its own cast
embedded. As I understand, the less casts we have, the better. There's
some other standardization work going on to remove unnecessary casts
elsewhere, so I'm not sure why we wouldn't do this.In the very first code import, BufferGetPage() was a regular function
that returned Page. (I suppose it was then turned into a macro, and
then back into an inline function.) Even in that first code import,
some callers cast the return to (Page), and some not. So I suppose this
style just crept in for some random and ancient reason and then got
copied around inconsistently.
Thank you for clarifications.
We should clean it up. Casts are bad.
I created CF [0]https://commitfest.postgresql.org/patch/6006/ for this.
[0]: https://commitfest.postgresql.org/patch/6006/
--
Best regards,
Kirill Reshke
On 28.08.25 10:08, Kirill Reshke wrote:
On Sat, 23 Aug 2025 at 19:57, Peter Eisentraut <peter@eisentraut.org> wrote:
On 22.08.25 11:59, Álvaro Herrera wrote:
On 2025-Aug-22, Kirill Reshke wrote:
I am uncertain about the delineation between when we make changes and
when we refrain from doing so.I think this is natural work after 9c727360bcc7, before which
BufferGetPage() was a macro and strangely enough had its own cast
embedded. As I understand, the less casts we have, the better. There's
some other standardization work going on to remove unnecessary casts
elsewhere, so I'm not sure why we wouldn't do this.In the very first code import, BufferGetPage() was a regular function
that returned Page. (I suppose it was then turned into a macro, and
then back into an inline function.) Even in that first code import,
some callers cast the return to (Page), and some not. So I suppose this
style just crept in for some random and ancient reason and then got
copied around inconsistently.Thank you for clarifications.
We should clean it up. Casts are bad.
I created CF [0] for this.
committed