some Page/PageData const stuff
In [0]/messages/by-id/001d457e-c118-4219-8132-e1846c2ae3c9@eisentraut.org 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]: /messages/by-id/001d457e-c118-4219-8132-e1846c2ae3c9@eisentraut.org
/messages/by-id/001d457e-c118-4219-8132-e1846c2ae3c9@eisentraut.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.
Attachments:
0001-Add-PageData.patchtext/plain; charset=UTF-8; name=0001-Add-PageData.patchDownload
From c2173c42c43488b9da56df114b9ff3032f7e9222 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@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
0002-Add-const-qualifiers-to-bufpage.h.patchtext/plain; charset=UTF-8; name=0002-Add-const-qualifiers-to-bufpage.h.patchDownload
From 0864d770d44d15dda5f8a506bead12f8da5a5a26 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@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
0003-Add-more-use-of-Page-PageData-rather-than-char.patchtext/plain; charset=UTF-8; name=0003-Add-more-use-of-Page-PageData-rather-than-char.patchDownload
From d12cd064b0d5d81d254516ab18f9c91bcc21569c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@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
This has been committed.
Show quoted text
On 09.12.24 16:44, Peter Eisentraut wrote:
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]: /messages/by-id/001d457e-c118-4219-8132-
e1846c2ae3c9%40eisentraut.orgThis 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.
Hi,
On 2025-01-20 15:01:08 +0100, Peter Eisentraut wrote:
This has been committed.
I don't like the const markings in PageGetItem():
/*
* PageGetItem
* Retrieves an item on the given page.
*
* Note:
* This does not change the status of any of the resources passed.
* The semantics may change in the future.
*/
static inline void *
PageGetItem(const PageData *page, const ItemIdData *itemId)
{
Assert(page);
Assert(ItemIdHasStorage(itemId));
return (void *) (((const char *) page) + ItemIdGetOffset(itemId));
}
The const for PageData seems like a lie to me, because we cast it away. And
indeed, we often then use the returned value to set hint bits etc.
Greetings,
Andres Freund
On 02.01.26 20:17, Andres Freund wrote:
On 2025-01-20 15:01:08 +0100, Peter Eisentraut wrote:
This has been committed.
I don't like the const markings in PageGetItem():
/*
* PageGetItem
* Retrieves an item on the given page.
*
* Note:
* This does not change the status of any of the resources passed.
* The semantics may change in the future.
*/
static inline void *
PageGetItem(const PageData *page, const ItemIdData *itemId)
{
Assert(page);
Assert(ItemIdHasStorage(itemId));return (void *) (((const char *) page) + ItemIdGetOffset(itemId));
}The const for PageData seems like a lie to me, because we cast it away. And
indeed, we often then use the returned value to set hint bits etc.
I agree this is incorrect. Since no callers appear to rely on the const
qualification of the argument, the easiest solution would be to just
remove it. See attached patch.
(In the future, this might be a candidate for a qualifier-preserving
polymorphic function like the C23 string functions, but that's for
another day.)
Attachments:
0001-Remove-bogus-const-qualifier-on-PageGetItem-argument.patchtext/plain; charset=UTF-8; name=0001-Remove-bogus-const-qualifier-on-PageGetItem-argument.patchDownload
From 585627bc317926eb1b690f4647566b4ae26a1453 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 3 Jan 2026 17:58:26 +0100
Subject: [PATCH] Remove bogus const qualifier on PageGetItem() argument
The function ends up casting away the const qualifier, so it was a
lie. No callers appear to rely on the const qualifier on the
argument, so the simplest solution is to just remove it.
---
src/include/storage/bufpage.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 52989e5e535..ae3725b3b81 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -351,12 +351,12 @@ PageValidateSpecialPointer(const PageData *page)
* The semantics may change in the future.
*/
static inline void *
-PageGetItem(const PageData *page, const ItemIdData *itemId)
+PageGetItem(PageData *page, const ItemIdData *itemId)
{
Assert(page);
Assert(ItemIdHasStorage(itemId));
- return (void *) (((const char *) page) + ItemIdGetOffset(itemId));
+ return (char *) page + ItemIdGetOffset(itemId);
}
/*
--
2.52.0
Hi,
On 2026-01-03 18:05:19 +0100, Peter Eisentraut wrote:
On 02.01.26 20:17, Andres Freund wrote:
On 2025-01-20 15:01:08 +0100, Peter Eisentraut wrote:
This has been committed.
I don't like the const markings in PageGetItem():
/*
* PageGetItem
* Retrieves an item on the given page.
*
* Note:
* This does not change the status of any of the resources passed.
* The semantics may change in the future.
*/
static inline void *
PageGetItem(const PageData *page, const ItemIdData *itemId)
{
Assert(page);
Assert(ItemIdHasStorage(itemId));return (void *) (((const char *) page) + ItemIdGetOffset(itemId));
}The const for PageData seems like a lie to me, because we cast it away. And
indeed, we often then use the returned value to set hint bits etc.I agree this is incorrect. Since no callers appear to rely on the const
qualification of the argument, the easiest solution would be to just remove
it. See attached patch.
LGTM
(In the future, this might be a candidate for a qualifier-preserving
polymorphic function like the C23 string functions, but that's for another
day.)
Makes sense. I doubt this is the place I would start with that though, the
amount of effort that'd take around all the functions dealing with heap tuples
seems too large...
Greetings,
Andres Freund
On 03.01.26 18:23, Andres Freund wrote:
On 2026-01-03 18:05:19 +0100, Peter Eisentraut wrote:
On 02.01.26 20:17, Andres Freund wrote:
On 2025-01-20 15:01:08 +0100, Peter Eisentraut wrote:
This has been committed.
I don't like the const markings in PageGetItem():
/*
* PageGetItem
* Retrieves an item on the given page.
*
* Note:
* This does not change the status of any of the resources passed.
* The semantics may change in the future.
*/
static inline void *
PageGetItem(const PageData *page, const ItemIdData *itemId)
{
Assert(page);
Assert(ItemIdHasStorage(itemId));return (void *) (((const char *) page) + ItemIdGetOffset(itemId));
}The const for PageData seems like a lie to me, because we cast it away. And
indeed, we often then use the returned value to set hint bits etc.I agree this is incorrect. Since no callers appear to rely on the const
qualification of the argument, the easiest solution would be to just remove
it. See attached patch.LGTM
committed