In [0] I wrote:
"""
I was fiddling a bit with making some Page-related APIs const-proof,
which might involve changing something like "Page p" to "const PageData
*p", but I was surprised that a type PageData exists but it's an
unrelated type local to generic_xlog.c.
This patch renames that type to a more specific name
[GenericXLogPageData]. This makes room for possibly adding another
PageData type with the earlier meaning, but that's not done here.
"""
[0]:
https://www.postgresql.org/message-id/flat/001d457e-c118-4219-8132-e1846c2ae3c9%40eisentraut.org
This is now the follow-up that adds back PageData with the earlier
meaning and updates a few of the Page-related APIs to be const-proof.
That is all pretty straightforward, except one inline function that had
to be changed back to a macro, because it is used in a way that
sometimes it takes const and returns const and sometimes takes non-const
and returns non-const. (We might be able to do that kind of thing
better with C23 in N years. ;-) )
Just a thought, I've been thinking it might be neat if PageData were
actually defined something like this:
typedef struct PageData
{
union
{
PageHeaderData phdr;
PGAlignedBlock data;
};
} PageData;
Then you could write all those (PageHeader) casts in a more elegant way,
and you don't get to randomly mix char * and Page, which has very weak
type safety. But this currently totally breaks, because many places
assume you can do char-based pointer arithmetic with Page values. So
this would need further analysis.From c2173c42c43488b9da56df114b9ff3032f7e9222 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 9 Dec 2024 16:35:23 +0100
Subject: [PATCH 1/3] Add PageData
---
src/include/storage/bufpage.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 6222d46e535..9f3ed976e43 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -78,7 +78,8 @@ extern PGDLLIMPORT bool ignore_checksum_failure;
* fields.
*/
-typedef Pointer Page;
+typedef char PageData;
+typedef PageData *Page;
/*
base-commit: 001a537b83ec6e2ab8fa8af44458b0502c94dd5e
--
2.47.1
From 0864d770d44d15dda5f8a506bead12f8da5a5a26 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 9 Dec 2024 16:35:23 +0100
Subject: [PATCH 2/3] Add const qualifiers to bufpage.h
---
src/backend/storage/page/bufpage.c | 32 ++++++-------
src/include/storage/bufpage.h | 75 +++++++++++++++---------------
src/tools/pgindent/typedefs.list | 1 +
3 files changed, 54 insertions(+), 54 deletions(-)
diff --git a/src/backend/storage/page/bufpage.c
b/src/backend/storage/page/bufpage.c
index aa264f61b9c..b32c5c0de45 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -85,9 +85,9 @@ PageInit(Page page, Size pageSize, Size specialSize)
* to pgstat.
*/
bool
-PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags)
+PageIsVerifiedExtended(const PageData *page, BlockNumber blkno, int flags)
{
- PageHeader p = (PageHeader) page;
+ const PageHeaderData *p = (PageHeaderData *) page;
size_t *pagebytes;
bool checksum_failure = false;
bool header_sane = false;
@@ -351,7 +351,7 @@ PageAddItemExtended(Page page,
* The returned page is not initialized at all; caller must do
that.
*/
Page
-PageGetTempPage(Page page)
+PageGetTempPage(const PageData *page)
{
Size pageSize;
Page temp;
@@ -368,7 +368,7 @@ PageGetTempPage(Page page)
* The page is initialized by copying the contents of the given
page.
*/
Page
-PageGetTempPageCopy(Page page)
+PageGetTempPageCopy(const PageData *page)
{
Size pageSize;
Page temp;
@@ -388,7 +388,7 @@ PageGetTempPageCopy(Page page)
* given page, and the special space is copied from the given page.
*/
Page
-PageGetTempPageCopySpecial(Page page)
+PageGetTempPageCopySpecial(const PageData *page)
{
Size pageSize;
Page temp;
@@ -893,16 +893,16 @@ PageTruncateLinePointerArray(Page page)
* PageGetHeapFreeSpace on heap pages.
*/
Size
-PageGetFreeSpace(Page page)
+PageGetFreeSpace(const PageData *page)
{
+ const PageHeaderData *phdr = (const PageHeaderData *) page;
int space;
/*
* Use signed arithmetic here so that we behave sensibly if pd_lower >
* pd_upper.
*/
- space = (int) ((PageHeader) page)->pd_upper -
- (int) ((PageHeader) page)->pd_lower;
+ space = (int) phdr->pd_upper - (int) phdr->pd_lower;
if (space < (int) sizeof(ItemIdData))
return 0;
@@ -920,16 +920,16 @@ PageGetFreeSpace(Page page)
* PageGetHeapFreeSpace on heap pages.
*/
Size
-PageGetFreeSpaceForMultipleTuples(Page page, int ntups)
+PageGetFreeSpaceForMultipleTuples(const PageData *page, int ntups)
{
+ const PageHeaderData *phdr = (const PageHeaderData *) page;
int space;
/*
* Use signed arithmetic here so that we behave sensibly if pd_lower >
* pd_upper.
*/
- space = (int) ((PageHeader) page)->pd_upper -
- (int) ((PageHeader) page)->pd_lower;
+ space = (int) phdr->pd_upper - (int) phdr->pd_lower;
if (space < (int) (ntups * sizeof(ItemIdData)))
return 0;
@@ -944,16 +944,16 @@ PageGetFreeSpaceForMultipleTuples(Page page, int ntups)
* without any consideration for adding/removing line pointers.
*/
Size
-PageGetExactFreeSpace(Page page)
+PageGetExactFreeSpace(const PageData *page)
{
+ const PageHeaderData *phdr = (const PageHeaderData *) page;
int space;
/*
* Use signed arithmetic here so that we behave sensibly if pd_lower >
* pd_upper.
*/
- space = (int) ((PageHeader) page)->pd_upper -
- (int) ((PageHeader) page)->pd_lower;
+ space = (int) phdr->pd_upper - (int) phdr->pd_lower;
if (space < 0)
return 0;
@@ -977,7 +977,7 @@ PageGetExactFreeSpace(Page page)
* on the number of line pointers, we make this extra check.)
*/
Size
-PageGetHeapFreeSpace(Page page)
+PageGetHeapFreeSpace(const PageData *page)
{
Size space;
@@ -1001,7 +1001,7 @@ PageGetHeapFreeSpace(Page page)
*/
for (offnum = FirstOffsetNumber; offnum <=
nline; offnum = OffsetNumberNext(offnum))
{
- ItemId lp =
PageGetItemId(page, offnum);
+ ItemId lp =
PageGetItemId(unconstify(PageData *, page), offnum);
if (!ItemIdIsUsed(lp))
break;
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 9f3ed976e43..0ee0ed66e72 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -222,9 +222,9 @@ typedef PageHeaderData *PageHeader;
* returns true iff no itemid has been allocated on the page
*/
static inline bool
-PageIsEmpty(Page page)
+PageIsEmpty(const PageData *page)
{
- return ((PageHeader) page)->pd_lower <= SizeOfPageHeaderData;
+ return ((const PageHeaderData *) page)->pd_lower <=
SizeOfPageHeaderData;
}
/*
@@ -232,9 +232,9 @@ PageIsEmpty(Page page)
* returns true iff page has not been initialized (by PageInit)
*/
static inline bool
-PageIsNew(Page page)
+PageIsNew(const PageData *page)
{
- return ((PageHeader) page)->pd_upper == 0;
+ return ((const PageHeaderData *) page)->pd_upper == 0;
}
/*
@@ -275,9 +275,9 @@ PageGetContents(Page page)
* however, it can be called on a page that is not stored in a buffer.
*/
static inline Size
-PageGetPageSize(Page page)
+PageGetPageSize(const PageData *page)
{
- return (Size) (((PageHeader) page)->pd_pagesize_version & (uint16)
0xFF00);
+ return (Size) (((const PageHeaderData *) page)->pd_pagesize_version &
(uint16) 0xFF00);
}
/*
@@ -285,9 +285,9 @@ PageGetPageSize(Page page)
* Returns the page layout version of a page.
*/
static inline uint8
-PageGetPageLayoutVersion(Page page)
+PageGetPageLayoutVersion(const PageData *page)
{
- return (((PageHeader) page)->pd_pagesize_version & 0x00FF);
+ return (((const PageHeaderData *) page)->pd_pagesize_version & 0x00FF);
}
/*
@@ -315,9 +315,9 @@ PageSetPageSizeAndVersion(Page page, Size size, uint8
version)
* Returns size of special space on a page.
*/
static inline uint16
-PageGetSpecialSize(Page page)
+PageGetSpecialSize(const PageData *page)
{
- return (PageGetPageSize(page) - ((PageHeader) page)->pd_special);
+ return (PageGetPageSize(page) - ((const PageHeaderData *)
page)->pd_special);
}
/*
@@ -326,23 +326,22 @@ PageGetSpecialSize(Page page)
* This is intended to catch use of the pointer before page initialization.
*/
static inline void
-PageValidateSpecialPointer(Page page)
+PageValidateSpecialPointer(const PageData *page)
{
Assert(page);
- Assert(((PageHeader) page)->pd_special <= BLCKSZ);
- Assert(((PageHeader) page)->pd_special >= SizeOfPageHeaderData);
+ Assert(((const PageHeaderData *) page)->pd_special <= BLCKSZ);
+ Assert(((const PageHeaderData *) page)->pd_special >=
SizeOfPageHeaderData);
}
/*
* PageGetSpecialPointer
* Returns pointer to special space on a page.
*/
-static inline char *
-PageGetSpecialPointer(Page page)
-{
- PageValidateSpecialPointer(page);
- return (char *) page + ((PageHeader) page)->pd_special;
-}
+#define PageGetSpecialPointer(page) \
+( \
+ PageValidateSpecialPointer(page), \
+ ((page) + ((PageHeader) (page))->pd_special) \
+)
/*
* PageGetItem
@@ -353,12 +352,12 @@ PageGetSpecialPointer(Page page)
* The semantics may change in the future.
*/
static inline Item
-PageGetItem(Page page, ItemId itemId)
+PageGetItem(const PageData *page, const ItemIdData *itemId)
{
Assert(page);
Assert(ItemIdHasStorage(itemId));
- return (Item) (((char *) page) + ItemIdGetOffset(itemId));
+ return (Item) (((const char *) page) + ItemIdGetOffset(itemId));
}
/*
@@ -371,9 +370,9 @@ PageGetItem(Page page, ItemId itemId)
* return zero to ensure sane behavior.
*/
static inline OffsetNumber
-PageGetMaxOffsetNumber(Page page)
+PageGetMaxOffsetNumber(const PageData *page)
{
- PageHeader pageheader = (PageHeader) page;
+ const PageHeaderData *pageheader = (const PageHeaderData *) page;
if (pageheader->pd_lower <= SizeOfPageHeaderData)
return 0;
@@ -385,7 +384,7 @@ PageGetMaxOffsetNumber(Page page)
* Additional functions for access to page headers.
*/
static inline XLogRecPtr
-PageGetLSN(const char *page)
+PageGetLSN(const PageData *page)
{
return PageXLogRecPtrGet(((const PageHeaderData *) page)->pd_lsn);
}
@@ -396,9 +395,9 @@ PageSetLSN(Page page, XLogRecPtr lsn)
}
static inline bool
-PageHasFreeLinePointers(Page page)
+PageHasFreeLinePointers(const PageData *page)
{
- return ((PageHeader) page)->pd_flags & PD_HAS_FREE_LINES;
+ return ((const PageHeaderData *) page)->pd_flags & PD_HAS_FREE_LINES;
}
static inline void
PageSetHasFreeLinePointers(Page page)
@@ -412,9 +411,9 @@ PageClearHasFreeLinePointers(Page page)
}
static inline bool
-PageIsFull(Page page)
+PageIsFull(const PageData *page)
{
- return ((PageHeader) page)->pd_flags & PD_PAGE_FULL;
+ return ((const PageHeaderData *) page)->pd_flags & PD_PAGE_FULL;
}
static inline void
PageSetFull(Page page)
@@ -428,9 +427,9 @@ PageClearFull(Page page)
}
static inline bool
-PageIsAllVisible(Page page)
+PageIsAllVisible(const PageData *page)
{
- return ((PageHeader) page)->pd_flags & PD_ALL_VISIBLE;
+ return ((const PageHeaderData *) page)->pd_flags & PD_ALL_VISIBLE;
}
static inline void
PageSetAllVisible(Page page)
@@ -490,19 +489,19 @@ StaticAssertDecl(BLCKSZ == ((BLCKSZ / sizeof(size_t)) *
sizeof(size_t)),
"BLCKSZ has to be a multiple of
sizeof(size_t)");
extern void PageInit(Page page, Size pageSize, Size specialSize);
-extern bool PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags);
+extern bool PageIsVerifiedExtended(const PageData *page, BlockNumber blkno,
int flags);
extern OffsetNumber PageAddItemExtended(Page page, Item item, Size size,
OffsetNumber offsetNumber, int flags);
-extern Page PageGetTempPage(Page page);
-extern Page PageGetTempPageCopy(Page page);
-extern Page PageGetTempPageCopySpecial(Page page);
+extern Page PageGetTempPage(const PageData *page);
+extern Page PageGetTempPageCopy(const PageData *page);
+extern Page PageGetTempPageCopySpecial(const PageData *page);
extern void PageRestoreTempPage(Page tempPage, Page oldPage);
extern void PageRepairFragmentation(Page page);
extern void PageTruncateLinePointerArray(Page page);
-extern Size PageGetFreeSpace(Page page);
-extern Size PageGetFreeSpaceForMultipleTuples(Page page, int ntups);
-extern Size PageGetExactFreeSpace(Page page);
-extern Size PageGetHeapFreeSpace(Page page);
+extern Size PageGetFreeSpace(const PageData *page);
+extern Size PageGetFreeSpaceForMultipleTuples(const PageData *page, int ntups);
+extern Size PageGetExactFreeSpace(const PageData *page);
+extern Size PageGetHeapFreeSpace(const PageData *page);
extern void PageIndexTupleDelete(Page page, OffsetNumber offnum);
extern void PageIndexMultiDelete(Page page, OffsetNumber *itemnos, int nitems);
extern void PageIndexTupleDeleteNoCompact(Page page, OffsetNumber offnum);
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index ce33e55bf1d..0b23985e90f 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1981,6 +1981,7 @@ PX_Combo
PX_HMAC
PX_MD
Page
+PageData
PageGistNSN
PageHeader
PageHeaderData
--
2.47.1
From d12cd064b0d5d81d254516ab18f9c91bcc21569c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 9 Dec 2024 16:35:23 +0100
Subject: [PATCH 3/3] Add more use of Page/PageData rather than char *
---
src/backend/access/transam/xloginsert.c | 16 ++++++++--------
src/include/access/xloginsert.h | 7 ++++---
2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/src/backend/access/transam/xloginsert.c
b/src/backend/access/transam/xloginsert.c
index f92d0626082..1151b68470e 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -72,7 +72,7 @@ typedef struct
RelFileLocator rlocator; /* identifies the relation and block */
ForkNumber forkno;
BlockNumber block;
- const char *page; /* page content */
+ const PageData *page; /* page content */
uint32 rdata_len; /* total length of data in
rdata chain */
XLogRecData *rdata_head; /* head of the chain of data registered
with
* this block */
@@ -138,8 +138,8 @@ static XLogRecData *XLogRecordAssemble(RmgrId rmid, uint8
info,
XLogRecPtr RedoRecPtr, bool doPageWrites,
XLogRecPtr *fpw_lsn, int *num_fpi,
bool
*topxid_included);
-static bool XLogCompressBackupBlock(const char *page, uint16 hole_offset,
- uint16
hole_length, char *dest, uint16 *dlen);
+static bool XLogCompressBackupBlock(const PageData *page, uint16 hole_offset,
+ uint16
hole_length, void *dest, uint16 *dlen);
/*
* Begin constructing a WAL record. This must be called before the
@@ -307,7 +307,7 @@ XLogRegisterBuffer(uint8 block_id, Buffer buffer, uint8
flags)
*/
void
XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator, ForkNumber forknum,
- BlockNumber blknum, const char *page, uint8
flags)
+ BlockNumber blknum, const PageData *page,
uint8 flags)
{
registered_buffer *regbuf;
@@ -648,7 +648,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
if (include_image)
{
- const char *page = regbuf->page;
+ const PageData *page = regbuf->page;
uint16 compressed_len = 0;
/*
@@ -941,13 +941,13 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
* the length of compressed block image.
*/
static bool
-XLogCompressBackupBlock(const char *page, uint16 hole_offset, uint16
hole_length,
- char *dest, uint16 *dlen)
+XLogCompressBackupBlock(const PageData *page, uint16 hole_offset, uint16
hole_length,
+ void *dest, uint16 *dlen)
{
int32 orig_len = BLCKSZ - hole_length;
int32 len = -1;
int32 extra_bytes = 0;
- const char *source;
+ const void *source;
PGAlignedBlock tmp;
if (hole_length != 0)
diff --git a/src/include/access/xloginsert.h b/src/include/access/xloginsert.h
index 652f7bc9bd1..09cf07992ca 100644
--- a/src/include/access/xloginsert.h
+++ b/src/include/access/xloginsert.h
@@ -15,6 +15,7 @@
#include "access/xlogdefs.h"
#include "storage/block.h"
#include "storage/buf.h"
+#include "storage/bufpage.h"
#include "storage/relfilelocator.h"
#include "utils/relcache.h"
@@ -47,16 +48,16 @@ extern void XLogEnsureRecordSpace(int max_block_id, int
ndatas);
extern void XLogRegisterData(const char *data, uint32 len);
extern void XLogRegisterBuffer(uint8 block_id, Buffer buffer, uint8 flags);
extern void XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator,
- ForkNumber forknum,
BlockNumber blknum, const char *page,
+ ForkNumber forknum,
BlockNumber blknum, const PageData *page,
uint8 flags);
extern void XLogRegisterBufData(uint8 block_id, const char *data, uint32 len);
extern void XLogResetInsertion(void);
extern bool XLogCheckBufferNeedsBackup(Buffer buffer);
extern XLogRecPtr log_newpage(RelFileLocator *rlocator, ForkNumber forknum,
- BlockNumber blkno,
char *page, bool page_std);
+ BlockNumber blkno,
Page page, bool page_std);
extern void log_newpages(RelFileLocator *rlocator, ForkNumber forknum, int
num_pages,
- BlockNumber *blknos, char
**pages, bool page_std);
+ BlockNumber *blknos, Page
*pages, bool page_std);
extern XLogRecPtr log_newpage_buffer(Buffer buffer, bool page_std);
extern void log_newpage_range(Relation rel, ForkNumber forknum,
BlockNumber startblk,
BlockNumber endblk, bool page_std);
--
2.47.1
