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+2-2
0002-Add-const-qualifiers-to-bufpage.h.patchtext/plain; charset=UTF-8; name=0002-Add-const-qualifiers-to-bufpage.h.patchDownload+54-55
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+12-12
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+2-3
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